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

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

[Tom Gundersen]

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);
+
 	ret = hid_add_device(hid);
 	if (ret) {
 		if (ret != -ENODEV)
-- 
2.4.3

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

[Greg Kroah-Hartman]

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>

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

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

[Alan Stern]

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

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

[Greg Kroah-Hartman]

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

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

[Alan Stern]

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

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

[Greg Kroah-Hartman]

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

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

[Jiri Kosina]

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.

-- 
Jiri Kosina
SUSE Labs

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

[Greg Kroah-Hartman]

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

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

[Alan Stern]

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

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

[Oliver Neukum]

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

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

[Jiri Kosina]

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

-- 
Jiri Kosina
SUSE Labs

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

[Oliver Neukum]

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

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

[Jiri Kosina]

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,

-- 
Jiri Kosina
SUSE Labs

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

[Andreas Mohr]

Hi,

Greg Kroah-Hartman wrote:
> On Fri, Jun 26, 2015 at 09:20:19PM -0400, Alan Stern wrote:
> > I can test it next week, if you would like to see the actual values.

> That would be great.

That one? (plus mouse plugged into keyboard hub)

http://lisas.de/~andi/lkml/Apple_A1048_lsusb/

HTH,

Andreas Mohr

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

[Alan Stern]

On Thu, 2 Jul 2015, Andreas Mohr wrote:

> Hi,
> 
> Greg Kroah-Hartman wrote:
> > On Fri, Jun 26, 2015 at 09:20:19PM -0400, Alan Stern wrote:
> > > I can test it next week, if you would like to see the actual values.
> 
> > That would be great.
> 
> That one? (plus mouse plugged into keyboard hub)
> 
> http://lisas.de/~andi/lkml/Apple_A1048_lsusb/

Yes, except that you included output for lots of other devices in 
addition to the hub of interest.

I was speaking of this:

	http://marc.info/?l=linux-usb&m=143567774004341&w=2

It seems quite likely that these keyboards won't work well with the 
proposed patch.

Alan Stern