Re: SM bus ioctls incorrect in FreeBSD 11

2016-11-10 Thread Adrian Chadd
+10 on option 2. Hate to say it, but I'd rather this little corner of
freebsd be "right" before it gets more heavily used.


-a


On 10 November 2016 at 03:21, Andriy Gapon  wrote:
> On 14/10/2016 18:51, Lewis Donzis wrote:
>> Our opinion doesn’t count for much, but I like 2 or 4.  Option 1 would
>> essentially obviate the entire purpose of changing the structure.  Option 2
>> basically finishes the job and makes it work properly.  Option 3 is, as you
>> say, unappealing.  I have no problem with Option 4, obviously we can change
>> our code back to the old way, but assuming there was a good reason for this
>> change in the first place, Option 2 seems more logical.
>>
>> But whatever y’all decide is fine with us, we’ll just change code to match at
>> the appropriate time.
>
> Anyone interested in the issue, could you please take a look at this review?
> https://reviews.freebsd.org/D8430
>
> Thank you.
>
> --
> Andriy Gapon
> ___
> freebsd-a...@freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arch
> To unsubscribe, send any mail to "freebsd-arch-unsubscr...@freebsd.org"
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: SM bus ioctls incorrect in FreeBSD 11

2016-11-10 Thread Andriy Gapon
On 14/10/2016 18:51, Lewis Donzis wrote:
> Our opinion doesn’t count for much, but I like 2 or 4.  Option 1 would
> essentially obviate the entire purpose of changing the structure.  Option 2
> basically finishes the job and makes it work properly.  Option 3 is, as you
> say, unappealing.  I have no problem with Option 4, obviously we can change
> our code back to the old way, but assuming there was a good reason for this
> change in the first place, Option 2 seems more logical.
> 
> But whatever y’all decide is fine with us, we’ll just change code to match at
> the appropriate time.

Anyone interested in the issue, could you please take a look at this review?
https://reviews.freebsd.org/D8430

Thank you.

-- 
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: SM bus ioctls incorrect in FreeBSD 11

2016-10-14 Thread John Baldwin
On Friday, October 14, 2016 05:50:40 PM Andriy Gapon 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=385154=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=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

During the review the assumption was that breaking the ABI wasn't great, but 
that
we could always fix it by adding compat handlers for the old ioctl values.  If
those are needed then they need to be restored.  If the new API is useful then 
it
needs to be fixed which I think is option 2.  I think it is new enough to just
fix without support compat shims for the broken version of it.

Unfortunately I don't know SMBus well enough to comment on your latter changes,
so I will defer to your call on those.

-- 
John Baldwin
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: SM bus ioctls incorrect in FreeBSD 11

2016-10-14 Thread Lewis Donzis

> On Oct 14, 2016, at 9:50 AM, Andriy Gapon  wrote:
> Could you please open a bugzilla issue for the bug?
> https://bugs.freebsd.org/bugzilla/

Sure, will do.  I was unsure of the effectiveness of that, because I filed a 
much more serious report about a different issue a couple of weeks ago and have 
seen no activity or even acknowledgement.

> 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 was also surprised, but it wasn’t really a big deal, and the new structure 
might be slightly easier to use.  There have been other similar things in 11.0, 
like __mq_oshandle() disappearing, and more .so files needing to be added to 
our embedded system, so all things considered, it’s been reasonably smooth 
moving from 10.3 to 11.0.


> 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.

Our opinion doesn’t count for much, but I like 2 or 4.  Option 1 would 
essentially obviate the entire purpose of changing the structure.  Option 2 
basically finishes the job and makes it work properly.  Option 3 is, as you 
say, unappealing.  I have no problem with Option 4, obviously we can change our 
code back to the old way, but assuming there was a good reason for this change 
in the first place, Option 2 seems more logical.

But whatever y’all decide is fine with us, we’ll just change code to match at 
the appropriate time.

Thanks,
lew

___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: SM bus ioctls incorrect in FreeBSD 11

2016-10-14 Thread Andriy Gapon
On 14/10/2016 18:11, Michael Gmelin wrote:
> 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.

I am leaning towards this approach as well.

> Thanks for cleaning after me.

You asked for a discussion and reviews.
I can not recall what I was doing at that time, but I completely ignored the
development and for that I can only blame myself.

> [1]https://lists.freebsd.org/pipermail/freebsd-arch/2015-March/016972.html
> [2]https://lists.freebsd.org/pipermail/freebsd-arch/2015-May/017157.html

I also agree that having a thin library on top of the ioctl would be a 
convenience.

-- 
Andriy Gapon
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"


Re: SM bus ioctls incorrect in FreeBSD 11

2016-10-14 Thread Michael Gmelin


On Fri, 14 Oct 2016 17:50:40 +0300
Andriy Gapon  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=385154=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=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
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: SM bus ioctls incorrect in FreeBSD 11

2016-10-14 Thread Andriy Gapon
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=385154=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=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
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"