| Message ID | 1435346647-29473-1-git-send-email-teg@jklm.no (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Fri, Jun 26, 2015 at 09:24:07PM +0200, 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@jklm.no> > Cc: Jiri Kosina <jkosina@suse.cz> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.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); > + As this duplicates what udev has been doing for a while now, we should be safe here. thanks for the patch. Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@jklm.no> > Cc: Jiri Kosina <jkosina@suse.cz> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.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 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-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@jklm.no> > > Cc: Jiri Kosina <jkosina@suse.cz> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.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-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 26, 2015 at 04:08:20PM -0400, 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@jklm.no> > > Cc: Jiri Kosina <jkosina@suse.cz> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.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 > 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. > Is that really how those devices are marked? I can't find any of my keyboard with hubs that mark things that way. > 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? udev has been doing this for a while now by default, with no reports of problems, so I think we should be safe. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 26 Jun 2015, Greg Kroah-Hartman wrote: > > 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. > > > > Is that really how those devices are marked? I can't find any of my > keyboard with hubs that mark things that way. My Apple keyboard isn't here at the moment, and I don't remember exactly what its hub descriptor contains. In theory, it _should_ mark the permanently attached port as non-removable. I can test it next week, if you would like to see the actual values. > > 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? > > udev has been doing this for a while now by default, with no reports of > problems, so I think we should be safe. I thought udev used a whitelist of devices known to work okay with autosuspend. Does it really turn on autosuspend for _every_ USB HID device that is marked as removable? (Come to think of it, given the bug in the hub driver, no device attached directly to the root hub will _ever_ be marked as removable AFAICS. So maybe that bug is masking possible regressions.) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 26, 2015 at 09:20:19PM -0400, Alan Stern wrote: > On Fri, 26 Jun 2015, Greg Kroah-Hartman wrote: > > > > 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. > > > > > > > Is that really how those devices are marked? I can't find any of my > > keyboard with hubs that mark things that way. > > My Apple keyboard isn't here at the moment, and I don't remember > exactly what its hub descriptor contains. In theory, it _should_ mark > the permanently attached port as non-removable. > > I can test it next week, if you would like to see the actual values. That would be great. > > > 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? > > > > udev has been doing this for a while now by default, with no reports of > > problems, so I think we should be safe. > > I thought udev used a whitelist of devices known to work okay with > autosuspend. Does it really turn on autosuspend for _every_ USB HID > device that is marked as removable? Yes, it had a tiny whitelist of 3-4 devices, and then would turn on autosuspend for anything not marked as removable or unknown. Look at /usr/lib/udev/rules.d/42-usb-hid-pm.rules on your system for them, it's these lines: ACTION=="add", SUBSYSTEM=="usb", SUBSYSTEMS=="usb", ATTR{../removable}=="removable", GOTO="usb_hid_pm_end" ACTION=="add", SUBSYSTEM=="usb", SUBSYSTEMS=="usb", ATTR{../removable}=="unknown", GOTO="usb_hid_pm_end" > (Come to think of it, given the bug in the hub driver, no device > attached directly to the root hub will _ever_ be marked as removable > AFAICS. So maybe that bug is masking possible regressions.) Maybe that's the issue, don't know, it would be good to figure out as upstream udev just deleted that whole rules file :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 26 Jun 2015, Greg Kroah-Hartman wrote: > > I thought udev used a whitelist of devices known to work okay with > > autosuspend. Does it really turn on autosuspend for _every_ USB HID > > device that is marked as removable? > > Yes, it had a tiny whitelist of 3-4 devices, and then would turn on > autosuspend for anything not marked as removable or unknown. Look at > /usr/lib/udev/rules.d/42-usb-hid-pm.rules on your system for them, it's > these lines: > ACTION=="add", SUBSYSTEM=="usb", SUBSYSTEMS=="usb", ATTR{../removable}=="removable", GOTO="usb_hid_pm_end" > ACTION=="add", SUBSYSTEM=="usb", SUBSYSTEMS=="usb", ATTR{../removable}=="unknown", GOTO="usb_hid_pm_end" > > > (Come to think of it, given the bug in the hub driver, no device > > attached directly to the root hub will _ever_ be marked as removable > > AFAICS. So maybe that bug is masking possible regressions.) > > Maybe that's the issue, don't know, it would be good to figure out as > upstream udev just deleted that whole rules file :) Last time we were testing this, autosuspend for USB HID devices was quite a disaster. Do you have any idea whether udev developers tested the "autosuspend on by default for USB HID devices" on reasonable set of devices? The culrpits that I remember from top of my head (it's been long time ago): - the LEDs for suspended device go off. This is very confusing at least on keyboards, and brings really bad user experience - many keyboards were losing first keystroke when waking up from suspend. We've been debugging this with Alan, and never root-caused it to a problem in our code, it seems to be the property of the HW I really do want to keep the autosuspend disabled by default on USB HID devices (at least keyboards), and enable it based on whitelist. If udev is not going to do this any more, I am afraid we'll have to move the default into the kernel.
On Sat, Jun 27, 2015 at 08:29:16AM +0200, Jiri Kosina wrote: > On Fri, 26 Jun 2015, Greg Kroah-Hartman wrote: > > > > I thought udev used a whitelist of devices known to work okay with > > > autosuspend. Does it really turn on autosuspend for _every_ USB HID > > > device that is marked as removable? > > > > Yes, it had a tiny whitelist of 3-4 devices, and then would turn on > > autosuspend for anything not marked as removable or unknown. Look at > > /usr/lib/udev/rules.d/42-usb-hid-pm.rules on your system for them, it's > > these lines: > > ACTION=="add", SUBSYSTEM=="usb", SUBSYSTEMS=="usb", ATTR{../removable}=="removable", GOTO="usb_hid_pm_end" > > ACTION=="add", SUBSYSTEM=="usb", SUBSYSTEMS=="usb", ATTR{../removable}=="unknown", GOTO="usb_hid_pm_end" > > > > > (Come to think of it, given the bug in the hub driver, no device > > > attached directly to the root hub will _ever_ be marked as removable > > > AFAICS. So maybe that bug is masking possible regressions.) > > > > Maybe that's the issue, don't know, it would be good to figure out as > > upstream udev just deleted that whole rules file :) > > Last time we were testing this, autosuspend for USB HID devices was quite > a disaster. > > Do you have any idea whether udev developers tested the "autosuspend on by > default for USB HID devices" on reasonable set of devices? That's accidentally turned on for all HID devices due to a bug in a udev rule in the latest version of udev/systemd and yes, it is a trainwreck. We can't do that at all, we need a real whitelist. Because of that, the udev developers wanted to just delete the whole rule file as it should be done in a real whitelist. But I noticed that it did have the 'autosuspend for connected devices' was in the udev rule file that was removed, and it had been there for a while, so we should add that to the kernel as we don't want to degrade in power usage if possible. > The culrpits that I remember from top of my head (it's been long time > ago): > > - the LEDs for suspended device go off. This is very confusing at least on > keyboards, and brings really bad user experience > > - many keyboards were losing first keystroke when waking up from suspend. > We've been debugging this with Alan, and never root-caused it to a > problem in our code, it seems to be the property of the HW > > I really do want to keep the autosuspend disabled by default on USB HID > devices (at least keyboards), and enable it based on whitelist. If udev is > not going to do this any more, I am afraid we'll have to move the default > into the kernel. We can add the whitelist to udev, that's the way to do it, but that's not what this patch does. Or at least that's not what it should be doing :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2015-06-27 at 08:29 +0200, Jiri Kosina wrote: > On Fri, 26 Jun 2015, Greg Kroah-Hartman wrote: > Last time we were testing this, autosuspend for USB HID devices was quite > a disaster. > > Do you have any idea whether udev developers tested the "autosuspend on by > default for USB HID devices" on reasonable set of devices? > > The culrpits that I remember from top of my head (it's been long time > ago): > > - the LEDs for suspended device go off. This is very confusing at least on > keyboards, and brings really bad user experience That is a bug. hidinput_count_leds() is supposed to prevent that. What did you test? > - many keyboards were losing first keystroke when waking up from suspend. > We've been debugging this with Alan, and never root-caused it to a > problem in our code, it seems to be the property of the HW - mice don't wake up from movement alone. And again I would state that we don't get enough information from user space. Hardware seems to be designed for sleeping while a screen saver is on. In kernel space we just get a binary input desired or not desired from open/close. That is insufficient. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 29 Jun 2015, Oliver Neukum wrote: > > Last time we were testing this, autosuspend for USB HID devices was quite > > a disaster. > > > > Do you have any idea whether udev developers tested the "autosuspend on by > > default for USB HID devices" on reasonable set of devices? > > > > The culrpits that I remember from top of my head (it's been long time > > ago): > > > > - the LEDs for suspended device go off. This is very confusing at least on > > keyboards, and brings really bad user experience > > That is a bug. hidinput_count_leds() is supposed to prevent that. This is a HW property and nothing kernel can do about. I am not saying it doesn't bring the LEDs up to a proper state again once auto-resumed. But I hate the LEDs going off a few seconds after I stop typing (i.e. once the keyboard gets auto-suspended).
On Mon, 2015-06-29 at 13:16 +0200, Jiri Kosina wrote: > On Mon, 29 Jun 2015, Oliver Neukum wrote: > > > > Last time we were testing this, autosuspend for USB HID devices was quite > > > a disaster. > > > > > > Do you have any idea whether udev developers tested the "autosuspend on by > > > default for USB HID devices" on reasonable set of devices? > > > > > > The culrpits that I remember from top of my head (it's been long time > > > ago): > > > > > > - the LEDs for suspended device go off. This is very confusing at least on > > > keyboards, and brings really bad user experience > > > > That is a bug. hidinput_count_leds() is supposed to prevent that. > > This is a HW property and nothing kernel can do about. I am not saying it > doesn't bring the LEDs up to a proper state again once auto-resumed. But I > hate the LEDs going off a few seconds after I stop typing (i.e. once the > keyboard gets auto-suspended). That is the point. Unless you give the option to override, they shouldn't autosuspend. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 29 Jun 2015, Oliver Neukum wrote: > > This is a HW property and nothing kernel can do about. I am not saying it > > doesn't bring the LEDs up to a proper state again once auto-resumed. But I > > hate the LEDs going off a few seconds after I stop typing (i.e. once the > > keyboard gets auto-suspended). > > That is the point. Unless you give the option to override, they > shouldn't autosuspend. Ah, you're right, I completely forgot about the logic we've put into hidinput_count_leds() quite some time ago. Sorry for the noise,
On Sat, 27 Jun 2015, Greg Kroah-Hartman wrote: > On Fri, Jun 26, 2015 at 09:20:19PM -0400, Alan Stern wrote: > > My Apple keyboard isn't here at the moment, and I don't remember > > exactly what its hub descriptor contains. In theory, it _should_ mark > > the permanently attached port as non-removable. > > > > I can test it next week, if you would like to see the actual values. > > That would be great. Here we go: # lsusb -v -s 3:4 Bus 003 Device 004: ID 05ac:1002 Apple, Inc. Extended Keyboard Hub [Mitsumi] Device Descriptor: bLength 18 bDescriptorType 1 bcdUSB 1.10 bDeviceClass 9 Hub bDeviceSubClass 0 Unused bDeviceProtocol 0 Full speed (or root) hub bMaxPacketSize0 8 idVendor 0x05ac Apple, Inc. idProduct 0x1002 Extended Keyboard Hub [Mitsumi] bcdDevice 1.22 iManufacturer 1 Mitsumi Electric iProduct 2 Hub in Apple Extended USB Keyboard iSerial 0 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 25 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 0 bmAttributes 0xa0 (Bus Powered) Remote Wakeup MaxPower 50mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 0 bNumEndpoints 1 bInterfaceClass 9 Hub bInterfaceSubClass 0 Unused bInterfaceProtocol 0 Full speed (or root) hub iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0001 1x 1 bytes bInterval 255 Hub Descriptor: bLength 9 bDescriptorType 41 nNbrPorts 3 wHubCharacteristic 0x0004 Ganged power switching Compound device Ganged overcurrent protection bPwrOn2PwrGood 22 * 2 milli seconds bHubContrCurrent 50 milli Ampere DeviceRemovable 0x02 PortPwrCtrlMask 0xff Hub Port Status: Port 1: 0000.0103 power enable connect Port 2: 0000.0100 power Port 3: 0000.0100 power Device Status: 0x0000 (Bus Powered) As you can see, the hub descriptor says that the hub is part of a compound device. Port 1 is attached to the internal keyboard controller, so it is connected and enabled, whereas the other two ports don't have anything plugged in right now. Most importantly, the DeviceRemovable bitmask is set to 0x02. Since bit 0 is reserved, the bit that is set corresponds to port 1. It is set to indicate that the port is non-removable (i.e., the meaning is the opposite of what the name suggests). And sure enough: $ cat /sys/bus/usb/devices/3-1.4.1/removable fixed (The internal hub is 3-1.4, and the keyboard controller is therefore 3-1.4.1.) I don't have any computers with a device permanently attached to an xHCI root-hub port. If someone else does, maybe they can check what happens when these two lines: if (!(wHubCharacteristics & HUB_CHAR_COMPOUND)) return; are deleted from drivers/usb/core/hub.c:set_usb_port_removable(). Deleting those lines may cause the device to show up as "fixed" rather than "removable". Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + ret = hid_add_device(hid); if (ret) { if (ret != -ENODEV)
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@jklm.no> Cc: Jiri Kosina <jkosina@suse.cz> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.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(+)