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
