SM bus ioctls incorrect in FreeBSD 11
Michael Gmelin
grembo at freebsd.org
Fri Oct 14 15:18:11 UTC 2016
On Fri, 14 Oct 2016 17:50:40 +0300
Andriy Gapon <avg at FreeBSD.org> wrote:
> On 14/10/2016 00:39, Lewis Donzis wrote:
> > After upgrading to FreeBSD 11.0 and changing source code to use the
> > new version of “struct smbcmd”, some commands are not working as
> > documented, specifically those that read data.
> >
> > As an example, SMB_READW is documented as returning the word read
> > from the device in rdata.word. However, this doesn’t happen, I
> > think because the ioctl request value is defined using _IOW(), so
> > the kernel doesn’t copy the data it read back out.
> >
> > In prior versions, the structure had only a pointer to the data,
> > and the smb.c code used copyout() to transfer the data back to
> > userland.
> >
> > As a temporary work-around, we added code to set rbuf to point to
> > rdata.word and rcount to two.
>
> Lewis,
>
> thank you for the report. This is a bug indeed and your analysis is
> correct. Could you please open a bugzilla issue for the bug?
> https://bugs.freebsd.org/bugzilla/
>
> Looking at ports commit 385155
> https://svnweb.freebsd.org/ports/head/sysutils/xmbmon/files/patch-getMB-smb_ioctl.c?r1=385155&r2=385154&pathrev=385155
> I see that it also used the approach that you use as a workaround.
> And that port commit is by Michael Gmelin who made the change to
> smb.h in r281985
> https://svnweb.freebsd.org/base?view=revision&revision=281985 So, I
> am not sure if the documented approach was known to not work.
>
> The src change is described as "Expand SMBUS API ...", but in fact it
> also _changed_ the existing ioctls. And both binary compatibility
> and programming compatibility were broken because of how struct
> smbcmd was changed. In FreeBSD we try to not do that without a very
> strong reason, but alas. And, as you report, the change was not done
> entirely correctly.
>
> I see several possibilities now.
>
> Option 1. Change the documentation to reflect the actual behavior.
> In this case data.rdata will remain unusable and unused. No
> interface changes.
>
> Option 2. Redefine SMB_READB, SMB_READW and SMB_PCALL ioctls using
> _IOWR, so that data.rdata could be returned from kernel. This seems
> like a proper fix, but it is another binary level incompatibility.
>
> Option 3. Use a horrible hack to discover a userland address of
> smbcmd and explicitly copyout to data.rdata. No interface
> incompatibilities, but it will be a horrible hack. Besides, not sure
> how feasible it is.
>
> Option 4. Revert smb ioctl changes to what they used to be before
> r281985. Personally, I would prefer this approach. But now that the
> new interface is in 11.0, it means another interface change just like
> Option 2.
>
> I would like to hear other developers' opinions about this situation.
>
> P.S.
> Two changes that I want to do no matter which course of action we
> select are:
> - revert SMB_MAXBLOCKSIZE to 32
> - remove SMB_TRANS as it does not map to anything defined by the SMBus
> specification and it can not be implemented for most, if not all,
> SMBus controllers
>
For some history on these changes, please see also [1] and [2] (there
were a few discussions and the revision was bumped, I also tried to
get some attention, but not enough it seems).
Given your recent changes to iicbus in HEAD, I think it would be best to
MFC those and go with Option 4 or, if that's to drastic, go with
Option 1. Thanks for cleaning after me.
- Michael
[1]https://lists.freebsd.org/pipermail/freebsd-arch/2015-March/016972.html
[2]https://lists.freebsd.org/pipermail/freebsd-arch/2015-May/017157.html
--
Michael Gmelin
More information about the freebsd-current
mailing list