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/ _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel