Re: SM bus ioctls incorrect in FreeBSD 11

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