laptop-mode-tools: USB autosuspend ignores user input devices

laptop-mode-tools: USB autosuspend ignores user input devices

Human input devices (HIDs) like mice and keyboards cannot handle the
auto suspend properly.  This patch modifies the autosuspend script to
check for and ignore all HIDs when enabling auto suspend.

BUG=chromium-os:4534
TEST=verified that HID autosuspend files are not being written to while
non-HID autosuspend files are
Signed-off-by: Simon Que <sque@chromium.org>;

Change-Id: If4a881a0efb3177697e1f4b4f15b8171c881f165

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=8d11ab2

[Benson Leung, 2010-09-30 18:06:42]

You should add the laptop mode maintainer's feedback here as well, simon.

http://codereview.chromium.org/3590005/diff/6001/7001
File laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend
(right):

http://codereview.chromium.org/3590005/diff/6001/7001#newcode28
laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend:28:
for subdevice in $device/*$device_base*; do
Any reason you have a wildcard before $device_base as well as after?

It seems that subdevices of devices such as 1-1 have a format of 1-1.1 or
1-1:1.0. Have you found some cases where there are leading characters in the
name as well?

http://codereview.chromium.org/3590005/diff/6001/7001#newcode63
laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend:63:
if ! is_hid /sys/bus/usb/devices/$usb_device; then
A piece of feedback from the maintainer was to create a config option to enable
or disable this behavior. Something like DISABLE_USBHID_POWERSAVE. It is a good
idea.

[Simon Que, 2010-09-30 18:17:07]

http://codereview.chromium.org/3590005/diff/6001/7001
File laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend
(right):

http://codereview.chromium.org/3590005/diff/6001/7001#newcode28
laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend:28:
for subdevice in $device/*$device_base*; do
No, I'll fix it in the next version of this patch.

http://codereview.chromium.org/3590005/diff/6001/7001#newcode63
laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend:63:
if ! is_hid /sys/bus/usb/devices/$usb_device; then
From the maintainer:  As for the USB HID devices, I agree that there might be
many faulty devices.  But I don't like to idea of blacklisting the entire
class.

But yes, as a distributor, one might want to blacklist HID devices based on
their experiences and user bug reports. Could you re-do the patch with a
configoption ? Something like: DISABLE_USBHID_POWERSAVE=0
That will be better in my opinion.

[Benson Leung, 2010-09-30 20:09:30]

On 2010/09/30 19:14:22, Simon Que wrote:
> 

LGTM. Go ahead and push it.

[Sameer Nanda, 2010-09-30 20:41:25]

http://codereview.chromium.org/3590005/diff/12001/13002
File laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend
(right):

http://codereview.chromium.org/3590005/diff/12001/13002#newcode24
laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend:24:
if [ "DRIVER=usbhid" = "`grep DRIVER=usbhid $device/uevent`" ]; then
better to use "grep -q" (so that the text doesn't spew on the screen) and use
the return status to know whether the grep was successful or not.

http://codereview.chromium.org/3590005/diff/12001/13002#newcode28
laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend:28:
for subdevice in $device/$device_base*; do
hmm, is this needed? If any of the child devices don't  auto-suspend, the parent
shouldn't get auto-suspended (by the kernel).

http://codereview.chromium.org/3590005/diff/12001/13002#newcode31
laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend:31:
then
nit: local coding style seems to be put 'then' on the same line as if

http://codereview.chromium.org/3590005/diff/12001/13002#newcode64
laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend:64:
|| [ "$DISABLE_USBHID_AUTOSUSPEND" != "1" ]; then
double negatives make the code harder to follow. Please change to
ENABLE_USBHID_AUTOSUSPEND instead.
Also, local convention seems to be add 'x' for numeric checks.

[Benson Leung, 2010-09-30 20:57:41]

Responded to sameer.

http://codereview.chromium.org/3590005/diff/12001/13002
File laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend
(right):

http://codereview.chromium.org/3590005/diff/12001/13002#newcode28
laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend:28:
for subdevice in $device/$device_base*; do
On 2010/09/30 20:41:25, Sameer Nanda wrote:
> hmm, is this needed? If any of the child devices don't  auto-suspend, the
parent
> shouldn't get auto-suspended (by the kernel).

This is necessary because the autosuspend control for a usb device is one level
up from the directory that identifies the device as a usbhid.

For example, when I plug in a USB keyboard, /sys/bus/usb/devices/4-2 appears.

The autosuspend and level controls are here: 
/sys/bus/usb/devices/4-2/power

However, the uevent for the device 4-2 doesn't identify the device as usbhid.

If you look at the subdevice here, you'll find a uevent:
/sys/bus/usb/devices/4-2/4-2:1.0/

That will tell DRIVER=usbhid.

These :1.0 directories are leaf nodes, so they won't have subdevices themselves.

[Sameer Nanda, 2010-09-30 21:07:56]

http://codereview.chromium.org/3590005/diff/12001/13002
File laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend
(right):

http://codereview.chromium.org/3590005/diff/12001/13002#newcode28
laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend:28:
for subdevice in $device/$device_base*; do
On 2010/09/30 20:57:41, Benson Leung wrote:
> On 2010/09/30 20:41:25, Sameer Nanda wrote:
> > hmm, is this needed? If any of the child devices don't  auto-suspend, the
> parent
> > shouldn't get auto-suspended (by the kernel).
> 
> This is necessary because the autosuspend control for a usb device is one
level
> up from the directory that identifies the device as a usbhid.
> 
> For example, when I plug in a USB keyboard, /sys/bus/usb/devices/4-2 appears.
> 
> The autosuspend and level controls are here: 
> /sys/bus/usb/devices/4-2/power
> 
> However, the uevent for the device 4-2 doesn't identify the device as usbhid.
> 
> If you look at the subdevice here, you'll find a uevent:
> /sys/bus/usb/devices/4-2/4-2:1.0/
> 
> That will tell DRIVER=usbhid.
> 
> These :1.0 directories are leaf nodes, so they won't have subdevices
themselves.

Ah. So a comment would be useful since this is non-obvious when reading the
code.

[Benson Leung, 2010-10-01 20:45:38]

Very small nit, but otherwise LGTM.

http://codereview.chromium.org/3590005/diff/21001/20003
File laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend
(right):

http://codereview.chromium.org/3590005/diff/21001/20003#newcode39
laptop-mode-tools_1.52/usr/share/laptop-mode-tools/modules/usb-autosuspend:39: 
Delete empty line.

[Sameer Nanda, 2010-10-01 20:51:52]

LGTM. Thanks!