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