Re: [linux-usb-devel] Re: [OOPS, usbcore, releaseintf] 2.6.0-test10-mm1

[David Brownell <david-b@pacbell.net>]

Oliver Neukum wrote:
>>(1) If both subsys.rwsem and dev->serialize are taken, then
>>subsys.rwsem must be taken first.
> 
> 
> Yes.

Erm, no.  As I pointed out the other day, it's a locking
hierarchy.  It must always go the other way:

  - dev->serialize first.  That controls the existence
    of the devices representing each interface ... which
    are different depending on device configuration.
    (Example, one config might have three interfaces,
    another one might just have one.)

    Needs to be grabbed during normal enumeration paths
    and by paths that can re-enumerate -- reset_device().
    The critical step is setting the configuration.

  - subsys.rwsem is grabbed automatically by the driver
    model core when it probes to see which driver will
    bind to the per-interface devices, or unbinds them
    (disconnect callbacks).

    It needs to be manually grabbed during claim() and
    release() style driver binding ... usbfs was doing
    this wrong, and I recently posted a patch that should
    fix it (same thread as the other binding fixups).

If you get the lock sequence wrong you'll eventually
deadlock with something that's using the correct
lock sequence.  Like khubd -- wedging much of USB.


>>(2) dev->serialize atomizes changes to the struct usb_device.
>>
>>Why then is dev->serialize not taken in usb_reset_device
>>(except in a dud code path)?

It IS taken in usb_reset_device(), and that "dud" path
is broken because that needs to be done in some other
task context.  (Because the reset must fail, and yet
the device is still there -- it needs to re-enumerate,
which that thread can't do.)

That "physical" reset_device doesn't grab it, since
its caller is guaranteed to already have the lock.


>>Also, why isn't dev->serialize enough to protect against
>>probe() during usb_reset_device?  After all, won't
>>dev->serialize be held during the probe calls (I didn't
>>check this and I'm in need of coffee - I hope I'm on the
>>right planet...)

Why?  See above about lock hierarchy.  When devices are
configured -- during enumeration, or eventually during
the re-enumeration on that "dud" codepath) -- devices
are created for each interface.


> In the current code definitely not.
> You must make sure that the configuration is still available.
> Guarding against probe() during reset is not enough.
> AFAIK David is currently rewriting this.

I have some unfinished code, but it's got to wait utnil
those two "fix driver binding" patches get merged:
the one for usbcore (everyone seemed OK with that one),
and the other for usbfs (no feedback yet).

There are entirely too many locks involved in all this,
and many of them will cause problems the minute any
very "interesting" things start getting done ... which
is why locking cleanups need to be done first.

- Dave



> 
> 	Regards
> 		Oliver
> 
>