On Sat, 19 Jan 2013, Sergei Shtylyov wrote: > > Furthermore, the locking in usbfs doesn't look right. A process should > > be able to submit as many URBs as it wants, of whatever type, at any > > time. > > You mean USBDEVFS_SUBMITURB ioctl()? That's indeed an issue with 2.4 patch > (and mine, of course) -- it still accounts this ioctl() as exclusive and so > only permits one instance of URB. I should have documented it internally as a > side effect of the v1 patch but I'm not really familiar with usbfs, and so > documented only what was fixed by Pete back in 2006 (USBDEVFS_BULK). I also meant USBDEVFS_BULK and USBDEVFS_CONTROL. Now that I look more closely at the patch, I wonder why it adds a lock in usbdev_do_ioctl? Won't that lock still be held when proc_bulk is called and tries to acquire its own lock? Won't that cause a deadlock? Also, why add a lock to usb_dump_desc in devices.c? None of the routines in that file try to communicate with the device. > > The locking should be by file handle, not by "read" vs. "write". > > You mean we still don't allow more than one URB per file handle? That > would probably require a lock in the 'struct dev_state'... No, this is what I mean: There's no point preventing usbfs from sending bulk URBs while usb-storage is using the device. usbfs already prevents that. What you want to do is prevent usbfs from sending control URBs at the wrong time. But when usb-storage isn't using the device, there's no reason to restrict how many URBs usbfs can send. Therefore you probably should use an rwsem. Have usb-storage lock it for writing, and have usbfs lock it for reading in proc_control and proc_submiturb (with matching unlocks at the right places). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
