Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices

[Alan Stern]

On Fri, 26 Jun 2015, Tom Gundersen wrote:

> This policy used to be unconditionally applied by udev, but there
> is no reason to make userspace be involved in this and in the future
> udev will not be doing it by default.
> 
> See: <https://github.com/systemd/systemd/pull/353>.
> 
> Signed-off-by: Tom Gundersen <teg@xxxxxxx>
> Cc: Jiri Kosina <jkosina@xxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
> 
> Hi,
> 
> I don't have the right hardware for this, so it has only been compile-tested.
> I'm therefore sending it as an RFC only. Mainly I want to bring it to people's
> attention that it would be great to get this feature into the kernel as we want
> to drop it from udev.
> 
> Cheers,
> 
> Tom
> 
>  drivers/hid/usbhid/hid-core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index bfbe1be..af80700 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1358,6 +1358,9 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
>  	setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
>  	spin_lock_init(&usbhid->lock);
>  
> +	if (dev->removable == USB_DEVICE_FIXED)
> +		usb_enable_autosuspend(dev);

This doesn't do what the patch title says.  USB_DEVICE_FIXED means that
the device can't be unplugged from its upstream port.  It doesn't mean
the device is internal to the computer.

As an example, consider a composite Apple keyboard, which has an
internal 3-port USB hub where two of the hub's ports are exposed on the
edge of the keyboard case and the keyboard controller is permanently
attached to the third hub port.  Then the controller device would be
marked USB_DEVICE_FIXED, even though the whole thing is external to
the computer and can be unplugged.

A reasonable compromise might be

	/*
	 * Enable autosuspend for devices permanently attached
	 * to the root hub.
	 */
	if (!dev->parent->parent && dev->removable == USB_DEVICE_FIXED)
		usb_enable_autosuspend(dev);

But this doesn't work if there's a permanently attached hub and a
device permanently attached to that hub.  To do this thoroughly, you
have to iterate up the dev->parent chain, making sure at each step that 
the ->removable value is USB_DEVICE_FIXED.

Also, are you really certain this is safe?  Aren't there a number of 
built-in keyboards that will work badly if you allow them to 
autosuspend?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/