Re: SM bus ioctls incorrect in FreeBSD 11

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

-- 
Andriy Gapon