On Thursday 25 August 2005 22.21, Alan Stern wrote:
> On Thu, 25 Aug 2005, Daniel Ritz wrote:
>
> > > The remark in parenthesis is wrong. dev->driver can be set when calling
> > > device_add, at which time the device obviously is not already bound to a
> > > driver. When this is done, device_attach will automatically bind the
> > > device to dev->driver.
> >
> > yeah, i noticed that too. see my answer in:
> > http://marc.theaimsgroup.com/?l=linux-kernel&m=112481438512222&w=2
> >
> > it's the easiest way to 'fix' the race by allowing a call to
> > device_bind_driver()
> > even if the device is already bound. fixes my rmmod hang and shoudln't mess
> > with USB. comment is updated too :)
> > what do you think about that as a short term fix?
>
> It looks fine for now. (Although I might have left the test in
> bus_rescan_devices_helper as a simple optimization.) For the long run, we
> do need to switch to a sane API.
>
> I was going to fix things by passing the driver as an explicit argument to
> device_bind_driver and device_release_driver. That way the subroutines
> can test the current value of dev->driver while holding the semaphore,
> avoiding a race.
an extra argument to device_bind_driver() is fine but i don't quite see the
point for an extra argument in device_release_driver().
device_bind_driver() with a driver argument should check:
- if it's already bound, return success if bound to the requested driver
- if dev->driver is already set, but not bound, bind and return success
- if dev->driver not set, bind and return success
but look what USB does in usb_driver_claim_interface(). device_bind_driver()
is only called if the device is already bound to the bus.
swapping these two lines in bus_add_device() would make that check go away, no?
device_attach(dev);
klist_add_tail(&bus->klist_devices, &dev->knode_bus);
BTW i find that usb_driver_claim_interface() is strange anyway. i mean an usb
driver is per inteface yet it sets the driver to the device. so if a usb device
has two (or more) interfaces this will lead to troubles if different drivers
for the different interfaces are required. (usbaudio for example binds the
same driver for both, the input and the ouput interface. it would lead to
double registration if the knode_bus check wouldn't be there).
so i think the .driver member should go back to struct usb_interface were it
has been before (btw. the comment in usb.h still describes the .driver member
even when it was removed). i think either abstract that interface thingy
in the driver core or create a dummy driver for usb that binds to the device
and does the handling of the driver per interface right. no?
>
> Calls to device_add with dev->driver already set can be handled by making
> the initial driver an explicit argument to device_attach. There's still
> the possibility of a race (another driver manages to get there first), but
> at least it won't cause an oops.
bus_add_device() also? and device_release_driver() should also check if the
knode_driver is attached before messing with klist_remove().
>
> What do you think? Complications arise from places where these routines
> are called with no explicit synchronization other than the subsystem
> rwsem. Fortunately some of those are going to go away.
>
i think we need to make a list of requirements to API, locking, etc.
that shows the needs of the different subsystems like USB, serio, pcmcia/CB,
etc. and then make a race-free locking design from that. currently some
subsystems work around limitations in the driver core...let's get rid of these
workarounds (and to make hch happy kill the klists :).
> Alan Stern
>
rgds
-daniel
-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
