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

On Thu, 12 Oct 2006, Cornelia Huck wrote:

On Wed, 11 Oct 2006 10:49:36 -0400 (EDT),
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: BDA driver, question about get properties from node
    ... Can you put the breakpoint in the driver ... Do they fail? ... > // Input pin to Output pin Topology Joints ... >>> IN PKSPROPERTY pKSProperty, ...
    (microsoft.public.development.device.drivers)
  • Re: ADM851X USB-LAN-Adapter driver
    ... no instrument no spam DOT com> wrote: ... Windows CE 4.2 driver worked with, then there's something wrong with your ... LQRP: LM_ReceiveCompleteCE fail! ...
    (microsoft.public.windowsce.platbuilder)
  • Re: [PATCH 1/2] [PCI] Check that MWI bit really did get set
    ... report it, recover from it, fail to load, etc. ... driver fails to do any of this then it's a buggy driver. ... Let me restore the words from my earlier email which you removed so that ... reasons. ...
    (Linux-Kernel)
  • Re: [PATCH] Driver core: Dont ignore bus_attach_device() retval
    ... It would be okay to fail in this case, because the driver would not have ... Having an unbound device is okay. ... Maybe ignoring all probe errors would be best here? ...
    (Linux-Kernel)
  • Re: Please revert 5adc55da4a7758021bcc374904b0f8b076508a11 (PCI_MULTITHREAD_PROBE)
    ... Threading at a driver level still does that (ie individual disks may be ... the device driver with multithreaded probing (on s390 ccw, ... I have used userspace tools for managing multiple machines that spawn tasks in parallel on multiple machines and then buffers them per-machine to display the output of the commands in order, no matter which one finished first. ... why can't the detection be done in parallel, but the regestration of detected devices be done in order? ...
    (Linux-Kernel)