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 <[email protected]>
Cc: Jiri Kosina <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
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
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 <[email protected]>
> Cc: Jiri Kosina <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> ---
>
> 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 <[email protected]>
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 <[email protected]>
> Cc: Jiri Kosina <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> ---
>
> 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
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 <[email protected]>
> > Cc: Jiri Kosina <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > ---
> >
> > 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
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 <[email protected]>
> > Cc: Jiri Kosina <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > ---
> >
> > 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
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
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
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
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
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
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
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
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
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
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
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