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