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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1506261622500.1566-100000@iolanthe.rowland.org>
Date:	Fri, 26 Jun 2015 16:28:20 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Tom Gundersen <teg@...m.no>
cc:	linux-usb@...r.kernel.org, <linux-input@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Jiri Kosina <jkosina@...e.cz>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices

On Fri, 26 Jun 2015, Alan Stern wrote:

> 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@...m.no>
> > Cc: Jiri Kosina <jkosina@...e.cz>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > ---
> > 
> > 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

Oops -- I meant "compound", not "composite".  A compound USB device
actually has multiple devices (one of which is a hub) in a single 
package.

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

Another problem is that the set_usb_port_removable() routine in
drivers/usb/core/hub.c (the routine that sets dev->removable in the
first place) contains this code:

	hub = usb_hub_to_struct_hub(udev->parent);

	wHubCharacteristics = le16_to_cpu(hub->descriptor->wHubCharacteristics);

	if (!(wHubCharacteristics & HUB_CHAR_COMPOUND))
		return;

I guess the assumption was that all of a hub's ports would be
unpluggable if the hub wasn't part of a compound device.  But that's
not correct for root hubs.  This test should be deleted.

Alan Stern

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