Date:	Thu, 12 Oct 2006 17:41:46 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Cornelia Huck <cornelia.huck@...ibm.com>
cc:	Jaroslav Kysela <perex@...e.cz>, Andrew Morton <akpm@...l.org>,
	ALSA development <alsa-devel@...a-project.org>,
	Takashi Iwai <tiwai@...e.de>, Greg KH <gregkh@...e.de>,
	LKML <linux-kernel@...r.kernel.org>,
	Jiri Kosina <jikos@...os.cz>,
	Castet Matthieu <castet.matthieu@...e.fr>,
	Akinobu Mita <akinobu.mita@...il.com>
Subject: Re: [PATCH] Driver core: Don't ignore bus_attach_device() retval

On Thu, 12 Oct 2006, Cornelia Huck wrote:

> On Thu, 12 Oct 2006 11:59:45 -0400 (EDT),
> Alan Stern <stern@...land.harvard.edu> wrote:
> 
> > > * 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.
> 
> In really_probe(), it is called after ->probe has been called (though
> the code does nothing but complain in the failure case).

??  In really_probe(), driver_sysfs_add() is called _before_ ->probe.  So
it's okay to fail there.  And in device_bind_driver(), which is what you
were talking about above, ->probe doesn't get called anywhere.

At least, in my copy of Greg KH's development tree that's what happens.


> > > * 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.
> 
> Maybe ignoring all probe errors would be best here? (Calling
> device_bind_driver() only on success, of course.)

I agree.


> > 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.
> 
> Currently, it is the device driver which specifies whether multithreaded
> probe should be done. Maybe this should rather be specified per
> subsystem? Having several bus_for_each_drv(..., dev, __device_attach)
> run in parallel makes more sense to me than the current approach.

Doing it per subsystem sounds right.  It looks like the multithread
probing code was added without sufficient thought beforehand.


> > > 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.
> 
> There's already been a fix:
> http://marc.theaimsgroup.com/?l=linux-kernel&m=116062971226779&w=2

Oh, okay. (Hmmm, that message was posted earlier today...)

> > Clearly
> > device_bind_driver() should call driver_sysfs_add() before driver_bound(),
> > not after.  Or else it should never fail.
> 
> Or that. I'm currently a bit in favour of ignoring symlink errors.

I don't care too much one way or another.  Although missing symlinks might 
cause problems for certain user programs.

> > It's a lot safer and easier just to switch the order of the calls in 
> > device_bind_driver().
> 
> ?? Did you mean really_probe() here?

No, I meant device_bind_driver().  Try to create the symlinks first, and 
if that succeeds (or if you don't care when it fails) then call 
driver_bound().  This confusion may be a result of looking at different 
source trees.

> >  I 
> > think in all cases, bus_attach_device() really should ignore the return 
> > code from device_attach().
> 
> This would be the only sensible approach if we had one probing thread
> per device. And device_bind_driver() should probably always succeed.

Other people may disagree about device_bind_driver().  Best to let it fail 
for now and fix these other problems first.  It can always be changed 
later.

Do you want to write another patch?

Alan Stern

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

Powered by blists - more mailing lists