On Wed, 16 May 2007, Oliver Neukum wrote:
> Here's part #2, the core autosuspend.
> @@ -230,12 +239,14 @@ static void hid_irq_in(struct urb *urb)
>
> switch (urb->status) {
> case 0: /* success */
> + usbhid_mark_busy(usbhid);
> usbhid->retry_delay = 0;
> hid_input_report(urb->context, HID_INPUT_REPORT,
> urb->transfer_buffer,
> urb->actual_length, 1);
> break;
> case -EPIPE: /* stall */
> + usbhid_mark_busy(usbhid);
> clear_bit(HID_IN_RUNNING, &usbhid->iofl);
> set_bit(HID_CLEAR_HALT, &usbhid->iofl);
> schedule_work(&usbhid->reset_work);
> @@ -249,6 +260,7 @@ static void hid_irq_in(struct urb *urb)
> case -EPROTO: /* protocol error or unplug */
> case -ETIME: /* protocol error or unplug */
> case -ETIMEDOUT: /* Should never happen, but... */
> + usbhid_mark_busy(usbhid);
> clear_bit(HID_IN_RUNNING, &usbhid->iofl);
> hid_io_error(hid);
> return;
At first I thought it would be easier to call usbhid_mark_busy() at
every URB completion. But obviously you don't want to do it when an
URB is unlinked.
> @@ -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()?
> @@ -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() || ...)
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()?
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?
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
