On Monday 29 August 2005 17.09, Alan Stern wrote: > [CC: list pared down] > > On Sun, 28 Aug 2005, Daniel Ritz wrote: > > > > 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(). > > The reason is simple: When device_release_driver runs, the current > driver may no longer be what the caller thinks it is! >
yeah, makes sense... > > 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 > > There's no point worrying about this case. With a proper API it will > never exist. For registered devices, dev->driver will _always_ be equal > to the bound driver (except at times like during probe, while dev->sem is > held). yes, when device_bind_driver() itself owns the semaphore like in your patch. (i read it now :) > > > - 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. > > Of course. We don't want to bind a driver to a device if the device > itself hasn't been registered yet. Trying to do that would mess up sysfs. i looked at the code again...i see now two reason for that check. one is the not yet registered device, the other is described below. > > > 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); > > No it wouldn't. The problem isn't that the code in bus_add_device needs > to be reordered; the problem is that we want to avoid calling > device_bind_driver before calling device_add. no reorderig wouldn't help at all. looking at the code and seeing where usb_driver_claim_interface() is called from. always from a drivers .probe() callback. a usb_driver's .probe() callback is only called from usb_probe_interface(). usb_probe_interface() is the usb's device_driver .probe callback which is called in driver_probe_device(). right after calling the .probe callback device_bind_driver() is called (of course only if .probe was successful). if i didn't miss anything usb's check for knode_bus is also stupid workaround to avoid a double device_bind_driver()... > > > > 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. > > ?? What on earth are you talking about? nada. forget it. i was just confused about the removed usb_interface.driver (-ENOCOFFE) > [ snip some complete crap i wrote ] > Have I missed something here? Since when has the .driver member been > removed from usb_interface? > 18 months ago. see: http://linux.bkbits.net:8080/linux-2.6/diffs/drivers/usb/core/usb.c%401.157?nav=index.html|src/.|src/drivers|src/drivers/usb|src/drivers/usb/core|hist/drivers/usb/core/usb.c but i still think it's damn ugly to check for knode_bus (layering violation?) besides: usbaudio (and others) calls usb_driver_claim_interface() for the second interface but does not own intf2->dev.sem a nice API? it should be something like - lock device - check if registered - if not, set dev->driver for later bind in device_attach() - if yes, bind - and if already bound fail - unlock device but it looks quite harmless from the places it's called from. > > > > 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? > > bus_add_device can continue to receive the initial driver in dev->driver, > but it should store that value in a local variable and set dev->driver to > NULL. Then it can pass the local variable to device_attach. that's a matter of taste i guess... > > > and device_release_driver() should also check if the > > knode_driver is attached before messing with klist_remove(). > > The patch I sent in before (which you said you hadn't read yet) already > takes care of that. i read it now but didn't test it yet (knowing it still has the race i run into :) > > > 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 :). > > My patch already gets rid of the driver's klist of devices. The other two which is fine :) > (the bus's klist of devices and the device's klist of children) seem > harmless, since a node can never be removed from one klist and then added > to another. didn't check if they're harmless, but i guess so. it's just because i kinda agree with christoph hellwig about the klists. they don't buy us anything but give overhead for nothing. each klist_node points to the list head but for nothing. we already have the refrerence to the parent device, bus, driver or whatever. it's just ugly if you ask me. (with almost no users, one less with your patch) > > I know the requirements for USB but not for those other subsystems. > Talking with their maintainers may help. In some cases the interfaces can > be simplified now that manual driver binding is available through sysfs. > i guess the most important is the one from dimitry. not deadlocking. so i think your patch is a good start to work from. 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
