Am Sonntag, 21. Oktober 2001 19:46 schrieben Sie:
> > > The simplest answer is (b):
> > >
> > > (b) usb_find_interface_driver() uses the dev->serialize semaphore
> > > for exclusion, but usbdevfs uses the BKL instead.
> >
> > This is not all, it uses driver->serialize() as well.
>
> Ah so it does. But still, usbdevfs uses neither,
> and that's clearly wrong: three locks to cover
> one data item ... wrong, wrong, wrong. (BKL,
> dev->serialize, and driver->serialize).
Yes, locking there is incorrect.
The BKL in usbdevfs is needed at least to prevent concurrent claimings of the
same interface. Taking a semaphore there should probably allow it to be
removed. Two locks are perhaps a little awkward by not in themselves a bug.
> Even if I were to acccept your claim that the
> driver->serialize semaphore should protect
> probe() and disconnect(), it couldn't work for
> usbdevfs doing device claiming since it's not
> using those driver entries.
Does that matter, if it takes dev->serialize ?
> > Using only dev->serialize() confuses drivers that expect
> > a single thread in probe(), and they do need a single thread
> > in proble because they need to initialize their mutual
> > exclusion primitives that they might use to serialize other calls.
>
> This is clearly an undocumented behavior. I think it's also
> a bad API guarantee too; static init of global locks is
> the rule (that's the only kind they "might use to serialize
> other calls"). That's stuff module init should handle (and
> precious little else, beyond registering with usbcore).
Nevertheless, there is no absolute need to change it in a stable kernel
series.
> > driver->serialize is common between probe and disconnect.
>
> Not in usbdevfs. And then there's the curiousity that
> dev->serialize is NOT common. Too many locks
> for handling one data structure.
>
> > Using dev->serialize would not work for the reason I pointed
> > out above: driver may attempt to set its globals in probe().
>
> Which isn't a good API usage policy. Which drivers
> do that? I scanned a bunch, didn't see any expecting
> that to be needed. But I did see a bunch that do static
> init or use module_init() for such stuff.
Have a close look at the probe() of printer.c.
I chose to check this driver at random and saw a potential race in the first
driver I looked at. Relaxing locking in a stable kernel is really not a good
idea.
The following patch should do it.
Regards
Oliver
--- devio.c.alt Mon Oct 1 20:37:40 2001
+++ devio.c Sun Oct 21 23:22:13 2001
@@ -321,13 +321,13 @@
return 0;
iface = &dev->actconfig->interface[intf];
err = -EBUSY;
- lock_kernel();
+ down(&dev->serialize);
if (!usb_interface_claimed(iface)) {
usb_driver_claim_interface(&usbdevfs_driver, iface, ps);
set_bit(intf, &ps->ifclaimed);
err = 0;
}
- unlock_kernel();
+ up(&dev->serialize);
return err;
}
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
