From owner-freebsd-arch@freebsd.org Fri Oct 14 15:11:37 2016 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 01DFFC1128E for ; Fri, 14 Oct 2016 15:11:37 +0000 (UTC) (envelope-from grembo@freebsd.org) Received: from mail.grem.de (outcast.grem.de [213.239.217.27]) by mx1.freebsd.org (Postfix) with SMTP id 2AD6ED53 for ; Fri, 14 Oct 2016 15:11:35 +0000 (UTC) (envelope-from grembo@freebsd.org) Received: (qmail 23590 invoked by uid 89); 14 Oct 2016 15:11:28 -0000 Received: from unknown (HELO bsd64.grem.de) (mg@grem.de@194.97.158.70) by mail.grem.de with ESMTPA; 14 Oct 2016 15:11:28 -0000 Date: Fri, 14 Oct 2016 17:11:26 +0200 From: Michael Gmelin To: Andriy Gapon Cc: Lewis Donzis , freebsd-current@FreeBSD.org, "freebsd-arch@freebsd.org" Subject: Re: SM bus ioctls incorrect in FreeBSD 11 Message-ID: <20161014171126.74e6e2fc@bsd64.grem.de> In-Reply-To: References: <06929AC5-D350-4236-A813-56C862B58174@perftech.com> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.29; amd64-portbld-freebsd10.2) X-Face: $wrgCtfdVw_H9WAY?S&9+/F"!41z'L$uo*WzT8miX?kZ~W~Lr5W7v?j0Sde\mwB&/ypo^}> +a'4xMc^^KroE~+v^&^#[B">soBo1y6(TW6#UZiC]o>C6`ej+i Face: iVBORw0KGgoAAAANSUhEUgAAADAAAAAwBAMAAAClLOS0AAAAJFBMVEWJBwe5BQDl LASZU0/LTEWEfHbyj0Txi32+sKrp1Mv944X8/fm1rS+cAAAACXBIWXMAAAsTAAAL EwEAmpwYAAAAB3RJTUUH3wESCxwC7OBhbgAAACFpVFh0Q29tbWVudAAAAAAAQ3Jl YXRlZCB3aXRoIFRoZSBHSU1QbbCXAAAAAghJREFUOMu11DFvEzEUAGCfEhBVFzuq AKkLd0O6VrIQsLXVSZXoWE5N1K3DobBBA9fQpRWc8OkWouaIjedWKiyREOKs+3PY fvalCNjgLVHeF7/3bMtBzV8C/VsQ8tecEgCcDgrzjekwKZ7TwsJZd/ywEKwwP+ZM 8P3drTsAwWn2mpWuDDuYiK1bFs6De0KUUFw0tWxm+D4AIhuuvZqtyWYeO7jQ4Aea 7jUqI+ixhQoHex4WshEvSXdood7stlv4oSuFOC4tqGcr0NjEqXgV4mMJO38nld4+ xKNxRDon7khyKVqY7YR4d+Cg0OMrkWXZOM7YDkEfKiilCn1qYv4mighZiynuHHOA Wq9QJq+BIES7lMFUtcikMnkDGHUoncA+uHgrP0ctIEqfwLHzeSo+eUA66AqzwN6n 2ZHJhw6Qh/PoyC/QENyEyC/AyNjq74Bs+3UH0xYwzDUC4B97HgLocg1QLYgDDO1v f3UX9Y307Ew4AHh67YAFFsxEpkXwpXY3eIgMhAAE3R19L919nNnuD2wlPcDE3UeT L2ytEICQib9BXgS2fU8PrD82ToYO1OEmMSnYTjSqSv9wdC0tPYC+rQRQD9ESnldF CyqfmiYW+tlALt8gH2xrMdC/youbjzPXEun+/ReXsMCDyve3dZc09fn2Oas8oXGc Jj6/fOeK5UmSMPmf/jL+GD8BEj0k/Fn6IO4AAAAASUVORK5CYII= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Oct 2016 15:11:37 -0000 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 =E2=80=9Cstruct smbcmd=E2=80=9D, some commands are not w= orking as > > documented, specifically those that read data. > >=20 > > As an example, SMB_READW is documented as returning the word read > > from the device in rdata.word. However, this doesn=E2=80=99t happen, I > > think because the ioctl request value is defined using _IOW(), so > > the kernel doesn=E2=80=99t copy the data it read back out. > >=20 > > 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. > >=20 > > As a temporary work-around, we added code to set rbuf to point to > > rdata.word and rcount to two. =20 >=20 > Lewis, >=20 > 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/ >=20 > Looking at ports commit 385155 > https://svnweb.freebsd.org/ports/head/sysutils/xmbmon/files/patch-getMB-s= mb_ioctl.c?r1=3D385155&r2=3D385154&pathrev=3D385155 > 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=3Drevision&revision=3D281985 So, I > am not sure if the documented approach was known to not work. >=20 > 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. >=20 > I see several possibilities now. >=20 > Option 1. Change the documentation to reflect the actual behavior. > In this case data.rdata will remain unusable and unused. No > interface changes. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > I would like to hear other developers' opinions about this situation. >=20 > 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 >=20 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 --=20 Michael Gmelin