Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752434AbbFZU21 (ORCPT ); Fri, 26 Jun 2015 16:28:27 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:37921 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751897AbbFZU2V (ORCPT ); Fri, 26 Jun 2015 16:28:21 -0400 Date: Fri, 26 Jun 2015 16:28:20 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Tom Gundersen cc: linux-usb@vger.kernel.org, , , Jiri Kosina , Greg Kroah-Hartman Subject: Re: [PATCH][RFC] usbhid: enable autosuspend for internal devices In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2927 Lines: 78 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: . > > > > Signed-off-by: Tom Gundersen > > Cc: Jiri Kosina > > Cc: Greg Kroah-Hartman > > --- > > > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/