[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-usb-devel
Subject:    Re: [linux-usb-devel] [PATCH 2.6.12-rc2] drivers/base/bus.c: fix
From:       Alan Stern <stern () rowland ! harvard ! edu>
Date:       2005-04-29 18:34:59
Message-ID: Pine.LNX.4.44L0.0504291410340.7108-100000 () iolanthe ! rowland ! org
[Download RAW message or body]

On Fri, 29 Apr 2005, Roman Kagan wrote:

> On Tue, Apr 26, 2005 at 11:57:34AM -0400, Alan Stern wrote:
> > On Tue, 26 Apr 2005, Roman Kagan wrote:
> > 
> > > On Mon, Apr 25, 2005 at 04:01:07PM -0400, Alan Stern wrote:
> > > > I think usb_driver_claim_interface is correct as it stands.  It was a 
> > > > mistake to leave out from usb_driver_release_interface originally the line 
> > > > setting iface->condition to USB_INTERFACE_UNBINDING.
> > > 
> > > But it would be asymmetric then.
> > 
> > In what sense?  You mean because we don't also protect against a driver
> > calling usb_driver_claim_interface from within that interface's probe?  
> > We don't need to check for that, do we?
> 
> Well, I'm not sure, but we do it now (from 2.6.12-rc2):

Yes, we do.  Okay, are you still concerned about the new code being
asymmetric somehow?

> > There are other reasons for keeping usb_interface.condition.  For one 
> > thing, the driver-model data doesn't tell when a bind or unbind is in 
> > progress.  That information is used elsewhere in usbcore.
> 
> AFAICT this "elsewhere" is only usb_lock_device_for_reset().

That's right.

>  And it
> looks like all its users (scsi reset in storage/scsiglue.c and
> image/microtek.c) don't really care if it is transitioning or already in
> an established bound state.

They care if it is in the UNBINDING transition.  This is because of the
potential for a strange deadlock between disconnect() and the SCSI layer:
If the error handler is running then scsi_remove_host won't return until
it's done, and the error handler might very well be waiting to lock the
device for a USB reset.

(Also usb-storage used to care if it was in the BINDING transition, but it 
doesn't care about that case any more.)

> > Here is a fix for driver_detach().  It's a little ugly because it avoids
> > using the klist iterator, but there's no way around it -- the iterator
> > simply can't be made to work here.  Not only does it prevent
> > device_release_driver() from calling klist_remove(), but also it prevents
> > doing get_device() while holding the klist spinlock.
>
> I was about to cook up something very much like this :)

Does the patch look good?  If you like it, I'll submit it to Greg.  I
don't know what has happened to Pat Mochel -- he hasn't posted anything to
the public mailing lists since April 7.

> > With this patch in place, the code in usb_driver_release_interface()
> > doesn't really need to be changed.
>
> Well, except for the inversion of the test, as given in my original
> post.

Yes.  :-)

> > However I think it should be.  As it
> > is now, it relies on internal knowledge of how the driver-model core
> > works,
>
> Agreed.  IMHO if the driver model strives to take care of everything on
> caller's behalf, it also has to gracefully handle recursive calls of one
> of its exported interfaces, device_release_driver().

A simple way to do it would be for __device_release_driver to set 
dev->driver to NULL _before_ calling dev->remove instead of after.  But 
would this be safe?  Can you think of a better way?

Alan Stern



-------------------------------------------------------
This SF.Net email is sponsored by: NEC IT Guy Games.
Get your fingers limbered up and give it your best shot. 4 great events, 4
opportunities to win big! Highest score wins.NEC IT Guy Games. Play to
win an NEC 61 plasma display. Visit http://www.necitguy.com/?r=20
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
[prev in list] [next in list] [prev in thread] [next in thread]