Re: [PATCH] Driver core: Don't ignore bus_attach_device() retval

[Alan Stern <stern@rowland.harvard.edu>]

On Thu, 12 Oct 2006, Cornelia Huck wrote:

> On Wed, 11 Oct 2006 10:49:36 -0400 (EDT),
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > You know, I'm not so sure device registration should fail when 
> > bus_attach_device() returns an error.
> 
> Hm, let's see why bus_attach_device() might fail:
> 
> * device_bind_driver() failed to create some symlinks. We may
> consider not to fail in this case, since sysfs_remove_link() is fine
> even for non-existing links.

It would be okay to fail in this case, because the driver would not have 
been probed at all.

> * probing failed for one possible driver with something other than
> -ENODEV or -ENXIO. Not sure if we really should abort in this case.
> We'd just end up with an unbound device, and a driver returning (for
> example) -ENOMEM for probing may just be a really dumb driver trying to
> allocate an insane amount of memory (and the next driver might just be
> fine).

Yes, this is exactly what I meant.  Having an unbound device is okay.

Note also that when multithreaded probing is used, there is no way for 
driver_probe_device() to return an error from really_probe().  We may as 
well act the same way when multithreaded probing isn't used.

I haven't looked at the driver core much since multithreaded probing was
added.  The multithreading part has a bad locking bug: the new thread
doesn't acquire the necessary semaphores.  Also it probes multiple drivers
for the same device in parallel, which seems wrong.  Since multiple probes 
can't run concurrently there's no reason to have a separate thread for 
each one.  There should be only one new thread per device.


> > Furthermore there are subtle problems that can arise.  In effect, the
> > device is registered for a brief time (while the driver is probed) and
> > then unregistered without giving the bus subsystem a chance to prepare for
> > the removal.  With USB this can lead to problems; if the driver called
> > usb_set_interface() then child devices would be created below the one
> > being probed -- and they would never get removed.
> 
> One way to fix this would be to make device_bind_driver() always
> succeed (even without symlinks),

Hmm... If device_bind_driver() fails -- because of the symlinks -- then
the device is still on the driver's klist, because the klist_add_tail() in
driver_bound() never gets undone.  Another bug.  Clearly
device_bind_driver() should call driver_sysfs_add() before driver_bound(),
not after.  Or else it should never fail.

>  the other to call the ->remove
> function if device_bind_driver() fails (assuming that the ->remove
> method should undo the stuff done in ->probe).

It's a lot safer and easier just to switch the order of the calls in 
device_bind_driver().

Note that it's also possible for device_attach() to fail through the 
__device_attach() -> driver_probe_device() -> really_probe() pathway.  I 
think in all cases, bus_attach_device() really should ignore the return 
code from device_attach().


> > In fact, we might want to separate driver probing from device_add()  
> > entirely.  That is, make them available as two separate function calls.  
> > That way the subsystem driver will have a chance to create attribute files
> > before a uevent is generated and a driver is loaded.  (That should help
> > udev to work better.)  This would require a larger change, though --
> > probably requiring an alternate version of device_add().
> 
> Shouldn't subsystems that need attributes early just use dev->groups,
> class->dev_attrs or bus->dev_attrs? These attribute groups are added
> before the uevent is generated.

Yes, you're right.  Forget about this part.

Alan Stern

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/