Re: autosuspend for hid

On Mon, 21 May 2007, Oliver Neukum wrote:

> Am Mittwoch, 16. Mai 2007 17:20 schrieb Alan Stern:
> > > @@ -592,8 +604,21 @@ static int hid_get_class_descriptor(stru
> > >  int usbhid_open(struct hid_device *hid)
> > >  {
> > >       struct usbhid_device *usbhid = hid->driver_data;
> > > +     int res;
> > >  
> > > -     hid->open++;
> > > +     mutex_lock(&hid_open_mut);
> > > +     if (!hid->open++) {
> > > +             res = usb_autopm_get_interface(usbhid->intf);
> > > +             /* the device must be awake to reliable request remote wakeup */
> > > +             if (res < 0) {
> > > +                     hid->open--;
> > > +                     mutex_unlock(&hid_open_mut);
> > > +                     return -EIO;
> > > +             }
> > > +             usbhid->intf->needs_remote_wakeup = 1;
> > > +             usb_autopm_put_interface(usbhid->intf);
> > > +     }
> > > +     mutex_unlock(&hid_open_mut);
> > >       if (hid_start_in(hid))
> > >               hid_io_error(hid);
> > >       return 0;
> > 
> > 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().


> > > @@ -1154,13 +1184,26 @@ static int hid_suspend(struct usb_interf
> > >       struct usbhid_device *usbhid = hid->driver_data;
> > >       struct usb_device *udev = interface_to_usbdev(intf);
> > >  
> > > -
> > > -     spin_lock_irq(&usbhid->inlock);
> > > -     set_bit(HID_REPORTED_IDLE, &usbhid->iofl);
> > > -     spin_unlock_irq(&usbhid->inlock);
> > > -     if (usbhid_wait_io(hid) < 0)
> > > -             return -EIO;
> > > -
> > > +     if (udev->auto_pm) {
> > > +             spin_lock_irq(&usbhid->inlock); /* Sync with error handler */
> > > +             if (!test_bit(HID_RESET_PENDING, &usbhid->iofl)
> > > +                 && !test_bit(HID_CLEAR_HALT, &usbhid->iofl)
> > > +                 && !test_bit(HID_OUT_RUNNING, &usbhid->iofl)
> > > +                 && !test_bit(HID_CTRL_RUNNING, &usbhid->iofl))
> > > +             {
> > > +                     set_bit(HID_REPORTED_IDLE, &usbhid->iofl);
> > > +                     spin_unlock_irq(&usbhid->inlock);
> > > +             } else {
> > > +                     spin_unlock_irq(&usbhid->inlock);
> > > +                     return -EBUSY;
> > > +             }
> > > +     } else {
> > > +             spin_lock_irq(&usbhid->inlock);
> > > +             set_bit(HID_REPORTED_IDLE, &usbhid->iofl);
> > > +             spin_unlock_irq(&usbhid->inlock);
> > > +             if (usbhid_wait_io(hid) < 0)
> > > +                     return -EIO;
> > > +     }
> > >       del_timer(&usbhid->io_retry);
> > >       usb_kill_urb(usbhid->urbin);
> > >       flush_scheduled_work();
> > 
> > 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.


> > to see if you need to fail right away.  You probably don't even need to
> > check udev->auto_pm again before calling usbhid_wait_io.
> > 
> > > @@ -1175,6 +1218,7 @@ static int hid_resume(struct usb_interfa
> > >       int status;
> > >  
> > >       clear_bit(HID_REPORTED_IDLE, &usbhid->iofl);
> > > +     usbhid_mark_busy(usbhid);
> > >       usbhid->retry_delay = 0;
> > >       status = hid_start_in(hid);
> > >       usbhid_restart_queues(usbhid);
> > 
> > Again, don't you need to call usb_autopm_get_interface() before 
> > hid_start_in()?
> 
> resume() and suspend() have mutual exclusion, don't they? So
> the device must be awake while resume() is running.

Oh, yes.  My mistake.

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

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