Re: [patch]a lockless method to alter pm count

On Wed, 25 Apr 2007, Oliver Neukum wrote:

> > Yes, the problem is clear.  Presumably you want to change things to go
> > like this:
> > 
> > usb_storage_control_thread():
> > usb_pm_lock(us->pusb_dev);
> > mutex_lock(&us->dev_mutex);
> > if (test_bit(US_FLIDX_DISCONNECTING, &us->flags)) ...
> > __usb_autopm_get_interface();
> > usb_pm_unlock(us->pusb_dev);
> > 
> > Right?  I've been thinking about this exact same problem recently.  It
> > affect every USB driver, in principle.  You might prefer the approach I 
> > thought up; it involves adding a new mutex to struct us_data like so:
> > 
> > storage_suspend():
> > mutex_lock(&us->suspend_mutex);
> > 
> > usb_stor_control_thread():
> > mutex_lock(&us->dev_mutex);
> > if (test_bit(US_FLIDX_DISCONNECTING, &us->flags)) ...
> > usb_autopm_get_interface();
> > mutex_lock(&us->suspend_mutex);
> > 
> > Either way is awkward, but mine is a little cleaner.  What do you think?
> 
> If you want me to think I can do that. If both solutions we come up
> are awkward, we are doing something wrong. It seems to me we should
> reduce the number of locks, not add new locks. Can we get rid of the
> special pm lock, use a general lock and export that to the drivers?

No, that's impossible.  It's because the locking rules for the pm_mutex
are opposite those for dev->sem: The order of acquisition of the dev->sem
locks is _down_ the device tree, but the order of acquisition of the
udev->pm_mutex locks is _up_ the device tree.

However in this particular case we may indeed be able to simplify things.  
Just do this:

usb_stor_control_thread():
usb_autopm_get_interface();
mutex_lock(&us->dev_mutex);
if (test_bit(US_FLIDX_DISCONNECTING, &us->flags)) ...

The only possible problem here is that the code does PM-related stuff at a
time when we aren't certain the driver is still bound to the interface.  
So change the way storage_disconnect() works; make it wait until the
control thread has exited.  If we convert over to using kthread_stop()
this will happen naturally.

I'll work on a patch to do that conversion...

Alan Stern


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel