Re: autosuspend for hid

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