From owner-freebsd-arch@freebsd.org Fri Oct 14 14:51:57 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 C395EC11B1B; Fri, 14 Oct 2016 14:51:57 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from citapm.icyb.net.ua (citapm.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 31F45FE1; Fri, 14 Oct 2016 14:51:55 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from porto.starpoint.kiev.ua (porto-e.starpoint.kiev.ua [212.40.38.100]) by citapm.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id RAA09302; Fri, 14 Oct 2016 17:51:52 +0300 (EEST) (envelope-from avg@FreeBSD.org) Received: from localhost ([127.0.0.1]) by porto.starpoint.kiev.ua with esmtp (Exim 4.34 (FreeBSD)) id 1bv3ps-000Kvw-37; Fri, 14 Oct 2016 17:51:52 +0300 Subject: Re: SM bus ioctls incorrect in FreeBSD 11 To: Lewis Donzis , freebsd-current@FreeBSD.org, "freebsd-arch@freebsd.org" References: <06929AC5-D350-4236-A813-56C862B58174@perftech.com> From: Andriy Gapon Message-ID: Date: Fri, 14 Oct 2016 17:50:40 +0300 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <06929AC5-D350-4236-A813-56C862B58174@perftech.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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 14:51:57 -0000 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 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 From owner-freebsd-arch@freebsd.org Fri Oct 14 15:31:15 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 A9162C11838; Fri, 14 Oct 2016 15:31:15 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from citapm.icyb.net.ua (citapm.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id A73EBCE7; Fri, 14 Oct 2016 15:31:13 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from porto.starpoint.kiev.ua (porto-e.starpoint.kiev.ua [212.40.38.100]) by citapm.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id SAA09427; Fri, 14 Oct 2016 18:31:11 +0300 (EEST) (envelope-from avg@FreeBSD.org) Received: from localhost ([127.0.0.1]) by porto.starpoint.kiev.ua with esmtp (Exim 4.34 (FreeBSD)) id 1bv4Rz-000KyQ-Lt; Fri, 14 Oct 2016 18:31:11 +0300 Subject: Re: SM bus ioctls incorrect in FreeBSD 11 To: Michael Gmelin References: <06929AC5-D350-4236-A813-56C862B58174@perftech.com> <20161014171126.74e6e2fc@bsd64.grem.de> Cc: freebsd-current@FreeBSD.org, "freebsd-arch@freebsd.org" From: Andriy Gapon Message-ID: Date: Fri, 14 Oct 2016 18:30:36 +0300 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161014171126.74e6e2fc@bsd64.grem.de> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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:31:15 -0000 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 From owner-freebsd-arch@freebsd.org Fri Oct 14 15:53:18 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 972BCC1021F for ; Fri, 14 Oct 2016 15:53:18 +0000 (UTC) (envelope-from lew@perftech.com) Received: from smtp-gw.pt.net (smtp-gw.pt.net [206.210.194.15]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "smtp-gw.pt.net", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 648B4F9E for ; Fri, 14 Oct 2016 15:53:18 +0000 (UTC) (envelope-from lew@perftech.com) X-ASG-Debug-ID: 1476460306-09411a4d54e70f0001-9koHHG Received: from mail.pt.net (mail.pt.net [206.210.194.11]) by smtp-gw.pt.net with ESMTP id s28qEGlHKHnbI0nj (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Fri, 14 Oct 2016 10:51:46 -0500 (CDT) X-Barracuda-Envelope-From: lew@perftech.com X-Barracuda-Effective-Source-IP: mail.pt.net[206.210.194.11] X-Barracuda-Apparent-Source-IP: 206.210.194.11 Received: from localhost (localhost [IPv6:::1]) by mail.pt.net (Postfix) with ESMTP id 93CC3841DA5; Fri, 14 Oct 2016 10:51:46 -0500 (CDT) Received: from mail.pt.net ([IPv6:::1]) by localhost (mail.pt.net [IPv6:::1]) (amavisd-new, port 10032) with ESMTP id sXjNM9tidETp; Fri, 14 Oct 2016 10:51:46 -0500 (CDT) Received: from localhost (localhost [IPv6:::1]) by mail.pt.net (Postfix) with ESMTP id 58E06842566; Fri, 14 Oct 2016 10:51:46 -0500 (CDT) X-Virus-Scanned: amavisd-new at pt.net Received: from mail.pt.net ([IPv6:::1]) by localhost (mail.pt.net [IPv6:::1]) (amavisd-new, port 10026) with ESMTP id rGGR-kYdDMhc; Fri, 14 Oct 2016 10:51:46 -0500 (CDT) Received: from dhcp-221-110.perftech.com (dhcp-221-110.perftech.com [206.210.221.110]) (Authenticated sender: lew@pt.net) by mail.pt.net (Postfix) with ESMTPSA id 3FCE4841DA5; Fri, 14 Oct 2016 10:51:46 -0500 (CDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) Subject: Re: SM bus ioctls incorrect in FreeBSD 11 From: Lewis Donzis X-ASG-Orig-Subj: Re: SM bus ioctls incorrect in FreeBSD 11 In-Reply-To: Date: Fri, 14 Oct 2016 10:51:46 -0500 Cc: freebsd-current@FreeBSD.org, "freebsd-arch@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: <1AA2BB21-0D6D-42F4-9CB2-3CBB00F389C6@perftech.com> References: <06929AC5-D350-4236-A813-56C862B58174@perftech.com> To: Andriy Gapon X-Mailer: Apple Mail (2.3124) X-Barracuda-Connect: mail.pt.net[206.210.194.11] X-Barracuda-Start-Time: 1476460306 X-Barracuda-Encrypted: ECDHE-RSA-AES256-GCM-SHA384 X-Barracuda-URL: https://smtp-gw.pt.net:443/cgi-mod/mark.cgi X-Barracuda-Scan-Msg-Size: 2592 X-Virus-Scanned: by bsmtpd at pt.net X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.00 X-Barracuda-Spam-Status: No, SCORE=0.00 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=9.0 tests= X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.33713 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 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:53:18 -0000 > 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=E2=80=99t 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=E2=80=99s been reasonably smooth moving from 10.3 to = 11.0. > 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. Our opinion doesn=E2=80=99t 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=E2=80=99all decide is fine with us, we=E2=80=99ll just = change code to match at the appropriate time. Thanks, lew From owner-freebsd-arch@freebsd.org Fri Oct 14 18:35:29 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 09BE9C122F9; Fri, 14 Oct 2016 18:35:29 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from mail.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id D697226B; Fri, 14 Oct 2016 18:35:28 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id 8934410AF90; Fri, 14 Oct 2016 14:35:27 -0400 (EDT) From: John Baldwin To: Andriy Gapon Cc: Lewis Donzis , freebsd-current@freebsd.org, "freebsd-arch@freebsd.org" Subject: Re: SM bus ioctls incorrect in FreeBSD 11 Date: Fri, 14 Oct 2016 11:34:45 -0700 Message-ID: <13130323.4rNJusEcix@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.0-PRERELEASE; KDE/4.14.10; amd64; ; ) In-Reply-To: References: <06929AC5-D350-4236-A813-56C862B58174@perftech.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (mail.baldwin.cx); Fri, 14 Oct 2016 14:35:27 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.99.2 at mail.baldwin.cx X-Virus-Status: Clean 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 18:35:29 -0000 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 =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 f= rom the > > device in rdata.word. However, this doesn=E2=80=99t happen, I thin= k 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, an= d 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 r= data.word > > and rcount to two. >=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-get= MB-smb_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=3D28= 1985 > 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 pro= gramming > compatibility were broken because of how struct smbcmd was changed. > In FreeBSD we try to not do that without a very strong reason, but al= as. > 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 interfac= e changes. >=20 > Option 2. Redefine SMB_READB, SMB_READW and SMB_PCALL ioctls using _I= OWR, so > that data.rdata could be returned from kernel. This seems like a pro= per fix, > but it is another binary level incompatibility. >=20 > Option 3. Use a horrible hack to discover a userland address of smbc= md and > explicitly copyout to data.rdata. No interface incompatibilities, bu= t 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 r2= 81985. > Personally, I would prefer this approach. But now that the new inter= face 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 sel= ect are: > - revert SMB_MAXBLOCKSIZE to 32 > - remove SMB_TRANS as it does not map to anything defined by the SMBu= s > 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 valu= es. If those are needed then they need to be restored. If the new API is usef= ul 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. --=20 John Baldwin