From owner-freebsd-usb@FreeBSD.ORG Sun Nov 9 15:13:20 2008 Return-Path: Delivered-To: usb@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 91DF61065678 for ; Sun, 9 Nov 2008 15:13:20 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe12.swip.net [212.247.155.97]) by mx1.freebsd.org (Postfix) with ESMTP id F090D8FC12 for ; Sun, 9 Nov 2008 15:13:19 +0000 (UTC) (envelope-from hselasky@c2i.net) X-Cloudmark-Score: 0.000000 [] X-Cloudmark-Analysis: v=1.0 c=1 a=MdMrz2CnUUkA:10 a=d6BVkb5LuPPVEe4iNQMLyA==:17 a=1su-2ruMAAAA:8 a=tLYY_Hw5G-20JAV5bo4A:9 a=pBeghD07ZpBDkmSujhgA:7 a=wGUWeYFnk7lRD-dxEgJPurCmvmQA:4 a=50e4U0PicR4A:10 Received: from [62.113.135.6] (account mc467741@c2i.net [62.113.135.6] verified) by mailfe12.swip.net (CommuniGate Pro SMTP 5.2.6) with ESMTPA id 973860208; Sun, 09 Nov 2008 15:13:16 +0100 From: Hans Petter Selasky To: freebsd-usb@freebsd.org Date: Sun, 9 Nov 2008 15:15:25 +0100 User-Agent: KMail/1.9.7 References: <20081109120257.GA78524@rink.nu> In-Reply-To: <20081109120257.GA78524@rink.nu> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200811091515.26123.hselasky@c2i.net> Cc: usb@freebsd.org Subject: Re: Patch to convert usb2 to use cdev X-BeenThere: freebsd-usb@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 09 Nov 2008 15:13:20 -0000 Hi Rick, After going through your patch I have a feeling you quite well understand how USB2 is working with regard to the file system. Some issues: 1) You don't have to create an alias in "usb2_fifo_attach". The alias /dev/usbX.Y.Z.T is mostly for internal usage. + make_dev_alias(f_sc->dev, buf); 2) struct usb2_privdata I would call the structure "usb2_fs_privdata" so that it is clear that this is File-System related private data. There is also a field called "xfer->priv_sc" so it might be confusing? 3) You need to solve the problem about a per-open-call context for /dev/ugenX.Y . This device is supposed to be cloneable, that means multiple processes are allowed to open it and establish independant connections to the USB stack. Here are also some tricky issues with permissions, because I allow trunking of multiple endpoints through the same file-handle, called USB FS, and you have to verify that the current thread has permission to open the endpoint inside an ioctl function. 4) You need to generate dummy /dev/ugenX.Y.0 ... 15 inclusivly, endpoint holders. Typically there are not 15 endpoints, but it is difficult to in-advance figure out this number. 5) Given that you use "destroy_dev_sched_cb" it becomes very easy to end up in a situation with multiple cdev instances having identical names, because the "destroy_dev_sched_cb" does not delete the device until the process which has the device opened closes it. Especially when re-attaching an USB device. Regarding your finding [1], I've assumed that lookup and open of a file is atomic in devfs regard. Else you would have to change the devfs-clone interface to be able to solve the problem passing along the global variables. --HPS On Sunday 09 November 2008, Rink Springer wrote: > Hi everyone, > > I've made a patch, available at http://rink.nu/tmp/usb-cdev.diff, which > removes the custom "/dev/usb " device, associated event handlers and > custom ownership/permissions structures and converts the whole deal to > use make_dev(9) and friends. The end result is that every USB device > will get a /dev entry, which can be chmod(1)-ed, chown(1)-ed etc as > usual - futhermore, possible races between looking up a device name and > opening it are completely removed by this patch [1] > > usbconfig(8) works as before after applying the patch, but obviously, > commands that involve setting permissions or ownership will return an > error as those ioctl's are no longer present; I intend to remove them > completely and from usbconfig itself after this patch has been > committed. > > Feel free to review this patch; I'd like to commit it to HEAD at the end > of the week or so. > > [1] The previous code would set a global variable to determine which > USB device corresponds with the file being looked up, and a > subsequent open call would open this device. I don't know the VFS > well enough to determine if this can be exploited, but it doesn't > look right to me :-) >