Re: autosuspend for hid

Am Montag, 21. Mai 2007 17:32 schrieb Alan Stern:
> On Mon, 21 May 2007, Oliver Neukum wrote:
> 
> > Am Mittwoch, 16. Mai 2007 17:20 schrieb Alan Stern:

> > > Don't you need to do usb_autopm_get_interface() before hid_start_in()?
> > 
> > hid_start_in() takes the spinlock and checks for a suspension. In the
> > unlikely case that the device already has been suspended, remote
> > wakeup must be active and resume() will call hid_start_in() again.
> 
> But this code runs whenever a process opens the device file.  If the
> device is suspended at that time, there might not be a remote wakeup
> request pending.  So you'd run into trouble unless you did an 
> autoresume before calling hid_start_in().

Which trouble? Remote wakeup must be activated as it is requested
with a bumped pm count. If there's no wakeup, what's the point in
pumping data in? There won't be any data.


> > > This might be simpler if you inverted the order of the tests.  That is, 
> > > first get the spinlock, then test for
> > > 
> > >         udev->auto_pm && (test_bit() || test_bit() || ...)
> > 
> > But that would put code needlessly under lock.
> 
> A single test of udev->auto_pm!  I think it would be worthwhile for the 
> improvements in comprehensibility and code size.

Code size yes. Comprehensibility, no. It might lead to the conclusions
that udev->auto_pm needs to be locked.

> > > One last thing, about the race between a last-minute URB completion and 
> > > autosuspend.  The USB autosuspend core routine doesn't check the 
> > > last_busy value after suspending the interface drivers and before 
> > > suspending the device.  Do we need to?
> > 
> > As the lock is per device, the resume() call would wait and proceed
> > immediately after suspend() drops the lock. I see no problem.
> 
> I guess it's just a theoretical problem.  The whole point of the
> last_busy field is to prevent autosuspend too soon after any I/O.  So
> if an URB does complete and nevertheless the device is suspended a few
> milliseconds later, then last_busy hasn't fulfilled its role.  Like
> this:
> 
> 	The autosuspend timer expires.
> 	last_busy is sufficiently old.
> 	hid_suspend() is called.
> 					The interrupt URB completes.
> 					last_busy is set to jiffies.
> 	hid_suspend() kills the
> 		interrupt URB and returns.
> 	usbcore doesn't notice that
> 		last_busy has been changed.
> 	usbcore suspends the device.
> 
> It's not actually _wrong_, but it isn't optimal.

Yes, the race exists. The original version scheduled work to reawake
the device in this case. Pete spotted the last vestige of that. Should I
put it back in? It seemed a pointless waste of perfection on a heuristics
that can't be perfect.

	Regards
		Oliver

-------------------------------------------------------------------------
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