2007-08-02 23:56:30

by Matthew Garrett

[permalink] [raw]
Subject: [PATCH] USB: Only enable autosuspend by default on certain device classes

We're seeing a large number of problems with devices not appreciating
USB autosuspend, especially printers and scanners. According to
http://www.microsoft.com/whdc/system/bus/USB/USBFAQ_intro.mspx only a
subset of drivers support it in Windows XP, meaning that most devices
are probably untested in this situation. This patch alters the behaviour
to match that of Windows. Userspace can still whitelist devices as
appropriate, and the set of classes supporting autosuspend probably
covers pretty much every driver likely to be found on any portable
device.

Signed-off-by: Matthew Garrett <[email protected]>

---

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index caaa46f..12ba789 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1278,6 +1278,22 @@ int usb_new_device(struct usb_device *udev)
{
int err;

+#ifdef CONFIG_USB_SUSPEND
+ /* Disable autosuspend for most devices - Windows only enables it
+ for a small subset of classes, so most hardware hasn't been tested
+ with it. Userspace can always reenable at a later point */
+
+ switch (udev->descriptor.bDeviceClass) {
+ case USB_CLASS_HID:
+ case USB_CLASS_COMM:
+ case USB_CLASS_WIRELESS_CONTROLLER:
+ case USB_CLASS_HUB:
+ break;
+ default:
+ udev->autosuspend_disabled = 1;
+ }
+#endif
+
/* Determine quirks */
usb_detect_quirks(udev);


--
Matthew Garrett | [email protected]


2007-08-03 01:12:42

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 12:56:13AM +0100, Matthew Garrett wrote:
> We're seeing a large number of problems with devices not appreciating
> USB autosuspend, especially printers and scanners. According to
> http://www.microsoft.com/whdc/system/bus/USB/USBFAQ_intro.mspx only a
> subset of drivers support it in Windows XP, meaning that most devices
> are probably untested in this situation. This patch alters the behaviour
> to match that of Windows. Userspace can still whitelist devices as
> appropriate, and the set of classes supporting autosuspend probably
> covers pretty much every driver likely to be found on any portable
> device.
>
> Signed-off-by: Matthew Garrett <[email protected]>

Well, if you do this, then you can pretty much delete the whole quirk
table we have, right?

And personally, I want to do better than Windows XP when it comes to
power management. This patch is only going to suspend a very tiny
subset of devices, including a whole bunch of ones that do not even have
drivers in Linux, causing our power footprint to be bigger than needed.

Also, we have udev rules for SANE that disables their autosuspend
settings, which handles the majority of the devices we have seen with
problems.

So I really don't want to accept this patch. But, what problems are you
seeing with our current suspend logic that you feel we need to be this
harsh?

thanks,

greg k-h

2007-08-03 01:48:18

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Thu, Aug 02, 2007 at 06:15:05PM -0700, Greg KH wrote:

> Well, if you do this, then you can pretty much delete the whole quirk
> table we have, right?

At the moment, yes.

> And personally, I want to do better than Windows XP when it comes to
> power management. This patch is only going to suspend a very tiny
> subset of devices, including a whole bunch of ones that do not even have
> drivers in Linux, causing our power footprint to be bigger than needed.

I agree. I'd much rather see us suspending devices whenever possible -
it's just that I have concerns over the scalability of the blacklist,
given the number of devices that seem to have issues.

> Also, we have udev rules for SANE that disables their autosuspend
> settings, which handles the majority of the devices we have seen with
> problems.

Several printers seem to have the issue as well, and the blacklist seems
to contain some odd miscellaneous devices like the Blackberry. The main
concern I have is that kernel developers just don't tend to be the sort
of people that use webcams, printers or scanners, so we're relying on
normal users to go to the effort of reporting that their device has
stopped working.

> So I really don't want to accept this patch. But, what problems are you
> seeing with our current suspend logic that you feel we need to be this
> harsh?

It's definitely a brute force approach, but it's one that means that we
get the low hanging fruit (ie, pretty much anything that's likely to be
plugged into a laptop) while massively reducing the probability of
breaking anyone's system. Saving some power is a nice win, but breaking
someone's printer is a pretty big loss.

--
Matthew Garrett | [email protected]

2007-08-03 02:47:04

by Alan Stern

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 3 Aug 2007, Matthew Garrett wrote:

> On Thu, Aug 02, 2007 at 06:15:05PM -0700, Greg KH wrote:
>
> > Well, if you do this, then you can pretty much delete the whole quirk
> > table we have, right?
>
> At the moment, yes.
>
> > And personally, I want to do better than Windows XP when it comes to
> > power management. This patch is only going to suspend a very tiny
> > subset of devices, including a whole bunch of ones that do not even have
> > drivers in Linux, causing our power footprint to be bigger than needed.
>
> I agree. I'd much rather see us suspending devices whenever possible -
> it's just that I have concerns over the scalability of the blacklist,
> given the number of devices that seem to have issues.

While I agree in general, perhaps a different approach would work
better. For instance, we could blacklist a few known-bad device
classes (maybe even using the existing blacklist) rather than
whitelisting a few known-good ones -- or trying to blacklist each
member of the bad classes!

Also, building something this sweeping into a kernel driver feels like
a mistake. It ought to be more easily configurable from userspace, say
via a sysfs file. Although this wouldn't be so important if we take
the blacklist-classes route.

Alan Stern

2007-08-03 06:01:19

by David Brownell

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Thursday 02 August 2007, Alan Stern wrote:
> Also, building something this sweeping into a kernel driver feels like
> a mistake. ?It ought to be more easily configurable from userspace, say
> via a sysfs file.

Yeah, I could have sworn there was extensive discussion over the
creation of a sysfs .../power/autususpend attribute which can enable
or disable autosuspend on a per-device basis.

Seems to me it ought to be practical to organize a database that can
be consulted by an outcall from udev, disabling autosuspend on devices
which are known to be broken. The "modules.usbmap" syntax is an obvious
place to start (painful though it is), and I'm sure there are folk who
would prefer web-accessible/updatable databases.

It'd need people to maintain that, of course, along with whatever
tools consult it. But that's a solvable problem, and it would keep
the problem properly outside of the kernel.

Long term, of course, this is just a pile of bugs for device vendors
to fix in their next revisions ... so they don't end up on the list
of "devices to avoid buying" for use with Linux systems.

- Dave

2007-08-03 06:06:26

by David Brownell

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Thursday 02 August 2007, Matthew Garrett wrote:
> The main
> concern I have is that kernel developers just don't tend to be the sort
> of people that use webcams, printers or scanners, so we're relying on
> normal users to go to the effort of reporting that their device has
> stopped working.

True, that rather suggests that some more desktoppy (that's now a word!)
people should get involved. Folk who know how to minimize that pain.

Sometimes when I plug in a USB device I get a dialog asking if I want to
configure it ... surely it would be possible to have that mechanism also
consult a database (part of a distro, or on some web server) fpr info
about the device, and offer the option for to poke at the device a bit to
see if it behaves. That stuff works for music CDs; why not USB devices?

- Dave

2007-08-03 07:55:41

by Oliver Neukum

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

Am Freitag 03 August 2007 schrieb Matthew Garrett:
> > Also, we have udev rules for SANE that disables their autosuspend
> > settings, which handles the majority of the devices we have seen with
> > problems.
>
> Several printers seem to have the issue as well, and the blacklist seems
> to contain some odd miscellaneous devices like the Blackberry. The main

Then make autosuspend support for the printer driver a config option.
This is not a reason to change the core usb code. The core code needs
to be involved only for device that are driven through usbfs. The major types
are:

- scanners
- PTP devices
- OBEX

Scanners are covered by SANE's latest CVS
PTP are a class and could be covered by a single udev rule
Obex is comm, so the patch wouldn't help.

> concern I have is that kernel developers just don't tend to be the sort
> of people that use webcams, printers or scanners, so we're relying on
> normal users to go to the effort of reporting that their device has
> stopped working.

Kernel developers are a diverser lot than you think ;-)
We don't enable autosuspend in drivers we can't test, except where
the lack of a kernel driver forces us to use a broad swipe. Printers
were tested, too, and most printers seem to work.

Regards
Oliver

2007-08-03 11:29:44

by Matthew Garrett

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Thu, Aug 02, 2007 at 11:01:08PM -0700, David Brownell wrote:

> Seems to me it ought to be practical to organize a database that can
> be consulted by an outcall from udev, disabling autosuspend on devices
> which are known to be broken. The "modules.usbmap" syntax is an obvious
> place to start (painful though it is), and I'm sure there are folk who
> would prefer web-accessible/updatable databases.

It's certainly possible to do that, but it's also possible to have a
userspace solution that whitelists devices. The question is whether the
default kernel behaviour should be "Save power, but potentially break
some of my devices" or "Don't break my devices, but use some more
powre".

--
Matthew Garrett | [email protected]

2007-08-03 11:41:55

by Oliver Neukum

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

Am Freitag 03 August 2007 schrieb Matthew Garrett:
> On Thu, Aug 02, 2007 at 11:01:08PM -0700, David Brownell wrote:
>
> > Seems to me it ought to be practical to organize a database that can
> > be consulted by an outcall from udev, disabling autosuspend on devices
> > which are known to be broken. ?The "modules.usbmap" syntax is an obvious
> > place to start (painful though it is), and I'm sure there are folk who
> > would prefer web-accessible/updatable databases.
>
> It's certainly possible to do that, but it's also possible to have a
> userspace solution that whitelists devices. The question is whether the
> default kernel behaviour should be "Save power, but potentially break
> some of my devices" or "Don't break my devices, but use some more
> powre".

If both options have drawbacks, IMHO we follow the standard, which
says that devices must support suspension.

Regards
Oliver

2007-08-03 12:04:43

by Matthew Garrett

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 01:44:02PM +0200, Oliver Neukum wrote:
> Am Freitag 03 August 2007 schrieb Matthew Garrett:
> > It's certainly possible to do that, but it's also possible to have a
> > userspace solution that whitelists devices. The question is whether the
> > default kernel behaviour should be "Save power, but potentially break
> > some of my devices" or "Don't break my devices, but use some more
> > powre".
>
> If both options have drawbacks, IMHO we follow the standard, which
> says that devices must support suspension.

Except that lots of hardware doesn't follow the standard in this
respect, otherwise we wouldn't be having this discussion. Personally, I
think "Will break an unknown number of devices" is a significantly
larger drawback than "Will consume a small quantity of additional
power".

--
Matthew Garrett | [email protected]

2007-08-03 12:32:31

by Rogan Dawes

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

Matthew Garrett wrote:
> On Fri, Aug 03, 2007 at 01:44:02PM +0200, Oliver Neukum wrote:
>> Am Freitag 03 August 2007 schrieb Matthew Garrett:
>>> It's certainly possible to do that, but it's also possible to have a
>>> userspace solution that whitelists devices. The question is whether the
>>> default kernel behaviour should be "Save power, but potentially break
>>> some of my devices" or "Don't break my devices, but use some more
>>> powre".
>> If both options have drawbacks, IMHO we follow the standard, which
>> says that devices must support suspension.
>
> Except that lots of hardware doesn't follow the standard in this
> respect, otherwise we wouldn't be having this discussion. Personally, I
> think "Will break an unknown number of devices" is a significantly
> larger drawback than "Will consume a small quantity of additional
> power".
>

I guess the question could be phrased:

Which one is more likely to conclude at some point?

That is, if we blacklist by default, we consume that additional power
indefinitely, because it is unlikely that people will report "my machine
uses 200mW more than I think it should", and thus we are unlikely to
build up knowledge of exactly which devices/classes should be blacklisted.

Compare that to:

"My USB printer broke, guess I'd better report it to LKML".

The first option is unlikely to ever reach a satisfactory conclusion,
whereas the second one is quite likely to flush out the guilty parties
within a relatively short time.

FWIW.

Rogan

2007-08-03 12:33:22

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 02:26:43PM +0200, Rogan Dawes wrote:

> Compare that to:
>
> "My USB printer broke, guess I'd better report it to LKML".

But while this is still a likely probability, the chances are no
distribution is going to ship with CONFIG_USB_SUSPEND enabled. Breaking
people's hardware (even if, at a fundamental level, it's the hardware
that's broken) generally irritates users - and I suspect that the users
it'll irritate the most are the ones who won't report it to LKML.

--
Matthew Garrett | [email protected]

2007-08-03 12:34:18

by Felipe Balbi

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On 8/3/07, Rogan Dawes <[email protected]> wrote:
> Matthew Garrett wrote:
> > On Fri, Aug 03, 2007 at 01:44:02PM +0200, Oliver Neukum wrote:
> >> Am Freitag 03 August 2007 schrieb Matthew Garrett:
> >>> It's certainly possible to do that, but it's also possible to have a
> >>> userspace solution that whitelists devices. The question is whether the
> >>> default kernel behaviour should be "Save power, but potentially break
> >>> some of my devices" or "Don't break my devices, but use some more
> >>> powre".
> >> If both options have drawbacks, IMHO we follow the standard, which
> >> says that devices must support suspension.
> >
> > Except that lots of hardware doesn't follow the standard in this
> > respect, otherwise we wouldn't be having this discussion. Personally, I
> > think "Will break an unknown number of devices" is a significantly
> > larger drawback than "Will consume a small quantity of additional
> > power".
> >
>
> I guess the question could be phrased:
>
> Which one is more likely to conclude at some point?
>
> That is, if we blacklist by default, we consume that additional power
> indefinitely, because it is unlikely that people will report "my machine
> uses 200mW more than I think it should", and thus we are unlikely to
> build up knowledge of exactly which devices/classes should be blacklisted.
>
> Compare that to:
>
> "My USB printer broke, guess I'd better report it to LKML".
>
> The first option is unlikely to ever reach a satisfactory conclusion,
> whereas the second one is quite likely to flush out the guilty parties
> within a relatively short time.

Even though we should be following what the specs says and find other
ways of threating the "messy" devices. A sysfs entry for
enabling/disabling autosuspend and even to add devices to blacklist
seems quite nice to me.

>
> FWIW.
>
> Rogan
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems? Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >> http://get.splunk.com/
> _______________________________________________
> [email protected]
> To unsubscribe, use the last form field at:
> https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
>


--
Best Regards,

Felipe Balbi
[email protected]

2007-08-03 14:08:05

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Friday 03 August 2007, Matthew Garrett wrote:
> On Fri, Aug 03, 2007 at 02:26:43PM +0200, Rogan Dawes wrote:
>
> > Which one is more likely to conclude at some point?

Good question ... though "how will it conclude" is also relevant.


> > Compare that to:
> >
> > "My USB printer broke, guess I'd better report it to LKML".

Which is, as I pointed out, the wrong response. Desktoppy
people should be making their tools do more intelligent things
with new USB devices they see ... like updating databases of
broken devices, and configuring *this* system to know that of
the devices it regularly deals with, this handful is broken.

Remember, these are the same users who wouldn't know what an
LKML is (do you plug them into the USB port??) if it bit them.


> But while this is still a likely probability, the chances are no
> distribution is going to ship with CONFIG_USB_SUSPEND enabled.

So you're saying all the distros want to make PM problems worse?

And that with all the other desktoppy stuff they're doing, not one
of them is willing to help make things better?

Pardon me if I want to hear distro vendors agree with you before I
believe that.


> Breaking
> people's hardware (even if, at a fundamental level, it's the hardware
> that's broken) generally irritates users - and I suspect that the users
> it'll irritate the most are the ones who won't report it to LKML.

Having a laptop drain its battery an hour before it needs to is
also irritating. (As are the extortionate prices for each model's
unique batteries, but that's a different issue.)

2007-08-03 14:09:58

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 07:01:11AM -0700, David Brownell wrote:

> Which is, as I pointed out, the wrong response. Desktoppy
> people should be making their tools do more intelligent things
> with new USB devices they see ... like updating databases of
> broken devices, and configuring *this* system to know that of
> the devices it regularly deals with, this handful is broken.

Popping up a box saying "Is your device broken?" isn't good UI.

> > But while this is still a likely probability, the chances are no
> > distribution is going to ship with CONFIG_USB_SUSPEND enabled.
>
> So you're saying all the distros want to make PM problems worse?

No. But given a choice between working hardware or marginally better
runtime PM, they're going to choose working hardware.

> And that with all the other desktoppy stuff they're doing, not one
> of them is willing to help make things better?

This patch is exactly that - a way of getting most of the benefits of
autosuspend without any real probability of breaking hardware. If you
mean "Are the distributions willing to pop up dialogs asking users to
start caring about obscure aspects of the USB spec", then I don't think
that's actually making things better.

> Pardon me if I want to hear distro vendors agree with you before I
> believe that.

I'm speaking as part of the Ubuntu kernel team, and I've been discussing
this with Fedora people.

>
> > Breaking
> > people's hardware (even if, at a fundamental level, it's the hardware
> > that's broken) generally irritates users - and I suspect that the users
> > it'll irritate the most are the ones who won't report it to LKML.
>
> Having a laptop drain its battery an hour before it needs to is
> also irritating. (As are the extortionate prices for each model's
> unique batteries, but that's a different issue.)

You commonly run a laptop off battery while having a printer plugged in?
--
Matthew Garrett | [email protected]

2007-08-03 14:23:20

by Dave Jones

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Thu, Aug 02, 2007 at 11:06:08PM -0700, David Brownell wrote:

> Sometimes when I plug in a USB device I get a dialog asking if I want to
> configure it ... surely it would be possible to have that mechanism also
> consult a database (part of a distro, or on some web server) fpr info
> about the device, and offer the option for to poke at the device a bit to
> see if it behaves. That stuff works for music CDs; why not USB devices?

Not everyone who uses USB devices uses X.
It's a bit difficult to pop up a "unscrew my printer" dialog when you're
on the console.

Dave

--
http://www.codemonkey.org.uk

2007-08-03 14:26:11

by Dave Jones

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 09:57:45AM +0200, Oliver Neukum wrote:

> Kernel developers are a diverser lot than you think ;-)
> We don't enable autosuspend in drivers we can't test, except where
> the lack of a kernel driver forces us to use a broad swipe. Printers
> were tested, too, and most printers seem to work.

My experience suggests the opposite. Of the several I've tried so far,
none have worked with usb suspend.

Dave

--
http://www.codemonkey.org.uk

2007-08-03 14:28:29

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 3 Aug 2007, Matthew Garrett wrote:

> This patch is exactly that - a way of getting most of the benefits of
> autosuspend without any real probability of breaking hardware. If you
> mean "Are the distributions willing to pop up dialogs asking users to
> start caring about obscure aspects of the USB spec", then I don't think
> that's actually making things better.

Quite aside from issues involving desktop ease-of-use and distribution
intentions, there is a technical matter to consider. With the default
autosuspend timeout set to 2 seconds, as it is now, it can often happen
that userspace isn't able to respond in time to prevent a device from
being suspended. At bootup especially, the system is so busy running
lots of tasks that response to a newly-detected device can be delayed
for tens of seconds. Even loading the device driver's module can take
so long that the device gets autosuspended first.

There are two possible solutions, both involving the kernel (since
userspace can't respond in time). One is to change the default timeout
to something larger, or even disable it completely. Then people would
need to rely on userspace tools to enable autosuspend on known-good
devices. The other possibility is to have a fairly reliable blacklist
or whitelist and again rely on userspace to manage edge cases. This is
of course more flexible than a blanket default setting, but it's still
pretty rigid. On the other hand, a blacklist can't be changed without
rebuilding the kernel whereas the default timeout can be adjusted on
the boot command line.

I don't know what the "best" approach is, but I can't see any
alternative to these two. Furthermore, whatever approach we settle on
_has_ to be able to handle devices which simply die upon being
suspended.

Alan Stern

2007-08-03 14:29:58

by Oliver Neukum

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

Am Freitag 03 August 2007 schrieb Dave Jones:
> On Fri, Aug 03, 2007 at 09:57:45AM +0200, Oliver Neukum wrote:
>
> > Kernel developers are a diverser lot than you think ;-)
> > We don't enable autosuspend in drivers we can't test, except where
> > the lack of a kernel driver forces us to use a broad swipe. Printers
> > were tested, too, and most printers seem to work.
>
> My experience suggests the opposite. Of the several I've tried so far,
> none have worked with usb suspend.

vendor:product please.

In addition, I'll make a config option for usblp using autosuspend.

Regards
Oliver

2007-08-03 14:34:19

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 10:28:19AM -0400, Alan Stern wrote:
> On Fri, 3 Aug 2007, Matthew Garrett wrote:

> There are two possible solutions, both involving the kernel (since
> userspace can't respond in time). One is to change the default timeout
> to something larger, or even disable it completely. Then people would
> need to rely on userspace tools to enable autosuspend on known-good
> devices. The other possibility is to have a fairly reliable blacklist
> or whitelist and again rely on userspace to manage edge cases. This is
> of course more flexible than a blanket default setting, but it's still
> pretty rigid. On the other hand, a blacklist can't be changed without
> rebuilding the kernel whereas the default timeout can be adjusted on
> the boot command line.

Windows will autosuspend hubs, bluetooth devices, HID devices and CDC
devices, so I think we're safe suspending those by default. I'm not so
enthusiastic about the "Increase the timeout case" - it doesn't avoid
any races, just makes them less likely. USB is likely to get loaded in
the initramfs, but we may not have a full set of udev rules until the
root fs is up and that can take an effectively arbitrarily large amount
of time.

--
Matthew Garrett | [email protected]

2007-08-03 14:38:16

by David Brownell

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Friday 03 August 2007, Matthew Garrett wrote:
> On Fri, Aug 03, 2007 at 07:01:11AM -0700, David Brownell wrote:
>
> > Which is, as I pointed out, the wrong response. Desktoppy
> > people should be making their tools do more intelligent things
> > with new USB devices they see ... like updating databases of
> > broken devices, and configuring *this* system to know that of
> > the devices it regularly deals with, this handful is broken.
>
> Popping up a box saying "Is your device broken?" isn't good UI.

Which is why I didn't suggest doing that, of course. The only
one making that kind of straw man argument seems to be you.


> > > But while this is still a likely probability, the chances are no
> > > distribution is going to ship with CONFIG_USB_SUSPEND enabled.
> >
> > So you're saying all the distros want to make PM problems worse?
>
> No. But given a choice between working hardware or marginally better
> runtime PM, they're going to choose working hardware.

However, you've strongly implied that you want to see a short term
patch, of the type which will (as Rogan Dawes implied) prevent solving
the problem in the long term ...


> > And that with all the other desktoppy stuff they're doing, not one
> > of them is willing to help make things better?
>
> This patch is exactly that - a way of getting most of the benefits of
> autosuspend without any real probability of breaking hardware. If you
> mean "Are the distributions willing to pop up dialogs asking users to
> start caring about obscure aspects of the USB spec", then I don't think
> that's actually making things better.

Evidently you were reading someone else's message and blaming its
content on me ... since the focus of what I wrote was having a
database *OUTSIDE THE KERNEL* which would obviate the need for
any user interaction in most cases.

If I were to describe any dialog users would see, it would be more
like "I don't recognize this device, help me set it up right...".
As with music CDs, that help might update the database for the next
person. (Assuming this were done well, of course.)


> > Pardon me if I want to hear distro vendors agree with you before I
> > believe that.
>
> I'm speaking as part of the Ubuntu kernel team, and I've been discussing
> this with Fedora people.

And has the discussion included the userspace people? (I don't
know how Ubuntu or Fedora folk structure such platform-scope
tasks. By inference, there's not enough cross-pollination...)

Speaking of which, what's this /dev/bus/usb/* crap on Ubuntu?
I had to undo all that on my Feisty system before any normal
/proc/bus/usb stuff would work again.


> > > Breaking
> > > people's hardware (even if, at a fundamental level, it's the hardware
> > > that's broken) generally irritates users - and I suspect that the users
> > > it'll irritate the most are the ones who won't report it to LKML.
> >
> > Having a laptop drain its battery an hour before it needs to is
> > also irritating. (As are the extortionate prices for each model's
> > unique batteries, but that's a different issue.)
>
> You commonly run a laptop off battery while having a printer plugged in?

Unfortunately I need to run laptop off AC since its battery life is
painfully short, and since Linux behaves so incredibly rudely when
the battery power goes down to almost-zero: it lets it go to zero
rather than hibernating. (And doesn't automatically enter suspend
when it idles, either...)

Note by the way that if you were -- for the sake of argument -- accept
my premise that this should all be handled in userspace ... it's very
easy to make userspace code do what you want. It doesn't need to be
done inside the kernel.

- Dave

2007-08-03 14:41:22

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 3 Aug 2007, Matthew Garrett wrote:

> Windows will autosuspend hubs, bluetooth devices, HID devices and CDC
> devices, so I think we're safe suspending those by default.

And we know that we're not safe suspending scanners and many printers
by default. But that leaves plenty of other device classes unaccounted
for.

> I'm not so
> enthusiastic about the "Increase the timeout case" - it doesn't avoid
> any races, just makes them less likely. USB is likely to get loaded in
> the initramfs, but we may not have a full set of udev rules until the
> root fs is up and that can take an effectively arbitrarily large amount
> of time.

If it takes longer than 15 minutes, something is wrong. At that point
the user will have worse things to worry about than whether some USB
devices got suspended.

And 15 minutes is a perfectly reasonable autosuspend timeout for
devices that might be plugged in all day long.

Alan Stern

2007-08-03 14:43:45

by Jiri Kosina

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 3 Aug 2007, Matthew Garrett wrote:

> Windows will autosuspend hubs, bluetooth devices, HID devices

Hi Matthew,

are you sure about windows suspending the HID devices in runtime? I have
never seen LEDs of USB keyboard connected to windows host to go off after
some time of not using it.

We have been playing with runtime autosuspend of HID devices, are
currently postponed the full support, as it turns out that many devices
don't support this feature properly (probably due to not being tested in
Windows).

--
Jiri Kosina

2007-08-03 14:50:57

by Dave Jones

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by?default on certain device classes

On Fri, Aug 03, 2007 at 04:32:07PM +0200, Oliver Neukum wrote:
> Am Freitag 03 August 2007 schrieb Dave Jones:
> > On Fri, Aug 03, 2007 at 09:57:45AM +0200, Oliver Neukum wrote:
> >
> > > Kernel developers are a diverser lot than you think ;-)
> > > We don't enable autosuspend in drivers we can't test, except where
> > > the lack of a kernel driver forces us to use a broad swipe. Printers
> > > were tested, too, and most printers seem to work.
> >
> > My experience suggests the opposite. Of the several I've tried so far,
> > none have worked with usb suspend.
>
> vendor:product please.

Here's one that I have handy..
Bus 002 Device 004: ID 04a9:1097 Canon, Inc.

(The others aren't powered on right now, and I'm remote from them ;-)

Dave

--
http://www.codemonkey.org.uk

2007-08-03 14:52:57

by David Brownell

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Friday 03 August 2007, Dave Jones wrote:
> On Thu, Aug 02, 2007 at 11:06:08PM -0700, David Brownell wrote:
>
> > Sometimes when I plug in a USB device I get a dialog asking if I want to
> > configure it ... surely it would be possible to have that mechanism also
> > consult a database (part of a distro, or on some web server) for info
> > about the device, and offer the option for to poke at the device a bit to
> > see if it behaves. That stuff works for music CDs; why not USB devices?
>
> Not everyone who uses USB devices uses X.

True, but for desktop users there doesn't seem to be any
real counter-argument to my point.


> It's a bit difficult to pop up a "unscrew my printer" dialog when you're
> on the console.

Right. Remotely operated systems (like servers) might
need some kind of "default assume-broken" setting.

- Dave

2007-08-03 14:58:45

by Jiri Kosina

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 3 Aug 2007, Dave Jones wrote:

> Interesting. Which devices did you notice failing? Was it a case that
> they would sleep and not come back out of that state?

Random keyboards, even connection to EHCI/OHCI seemed to make difference.
We have been doing some experiments with Alan during OLS and it seemed
quite hopeless.

A few keyboards we have been testing with seemed to be losing keypressess
when coming out of suspend (if and only if the root hub wasn't suspended
too, etc. Magic).

--
Jiri Kosina

2007-08-03 15:03:24

by Matthew Garrett

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 07:37:55AM -0700, David Brownell wrote:
> On Friday 03 August 2007, Matthew Garrett wrote:
> > Popping up a box saying "Is your device broken?" isn't good UI.
>
> Which is why I didn't suggest doing that, of course. The only
> one making that kind of straw man argument seems to be you.

But however you phrase it, that's effectively what it is. "Does your
device work?" just makes users wonder why the damn computer doesn't know
already. "This option may prevent your device from working. Click here
to switch it off" results in them wondering why it was switched on in
the first place. Many of our users aren't technical - they don't care
about saving 200mW, they just care about their printer working when they
plug it in.

And, frankly, if I got a requestor like that every time I plugged in a
new USB device I'd be fairly unhappy.

> > No. But given a choice between working hardware or marginally better
> > runtime PM, they're going to choose working hardware.
>
> However, you've strongly implied that you want to see a short term
> patch, of the type which will (as Rogan Dawes implied) prevent solving
> the problem in the long term ...

Because I don't believe we'll ever identify every device that gets
broken, and so as a result I think it's better to disable the
functionality by default for anything that might be broken.

> If I were to describe any dialog users would see, it would be more
> like "I don't recognize this device, help me set it up right...".
> As with music CDs, that help might update the database for the next
> person. (Assuming this were done well, of course.)

Users understand CDs. They don't understand runtime power management.

> > I'm speaking as part of the Ubuntu kernel team, and I've been discussing
> > this with Fedora people.
>
> And has the discussion included the userspace people? (I don't
> know how Ubuntu or Fedora folk structure such platform-scope
> tasks. By inference, there's not enough cross-pollination...)

I also handle a lot of the desktop/kernel integration.

> Speaking of which, what's this /dev/bus/usb/* crap on Ubuntu?
> I had to undo all that on my Feisty system before any normal
> /proc/bus/usb stuff would work again.

"Usbfs files can't handle Access Control Lists (ACL), which are the
default way to grant access to USB devices for untrusted users of a
desktop system. The usbfs functionality is replaced by real device-nodes
managed by udev. These nodes live in /dev/bus/usb and are used by
libusb."

(From Kconfig)

> > You commonly run a laptop off battery while having a printer plugged in?
>
> Unfortunately I need to run laptop off AC since its battery life is
> painfully short, and since Linux behaves so incredibly rudely when
> the battery power goes down to almost-zero: it lets it go to zero
> rather than hibernating. (And doesn't automatically enter suspend
> when it idles, either...)

System/Preferences/Power Management

> Note by the way that if you were -- for the sake of argument -- accept
> my premise that this should all be handled in userspace ... it's very
> easy to make userspace code do what you want. It doesn't need to be
> done inside the kernel.

It can be, but I'd prefer to have userspace enable functionality than
have the kernel break things.
--
Matthew Garrett | [email protected]

2007-08-03 15:04:40

by Dave Jones

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 04:43:16PM +0200, Jiri Kosina wrote:
> On Fri, 3 Aug 2007, Matthew Garrett wrote:
>
> > Windows will autosuspend hubs, bluetooth devices, HID devices
>
> Hi Matthew,
>
> are you sure about windows suspending the HID devices in runtime? I have
> never seen LEDs of USB keyboard connected to windows host to go off after
> some time of not using it.

Not so sure about keyboards, but I've seen the LEDs on USB mice dim or go off
after a few seconds of inactivity under Windows, but under Linux they stay on.

> We have been playing with runtime autosuspend of HID devices, are
> currently postponed the full support, as it turns out that many devices
> don't support this feature properly (probably due to not being tested in
> Windows).

Interesting. Which devices did you notice failing?
Was it a case that they would sleep and not come back out of that state?

Dave

--
http://www.codemonkey.org.uk

2007-08-03 15:06:24

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 04:43:16PM +0200, Jiri Kosina wrote:

> are you sure about windows suspending the HID devices in runtime? I have
> never seen LEDs of USB keyboard connected to windows host to go off after
> some time of not using it.

Ah, sorry - on reading more closely, HID devices will only be
autosuspended if they're whitelisted.

> We have been playing with runtime autosuspend of HID devices, are
> currently postponed the full support, as it turns out that many devices
> don't support this feature properly (probably due to not being tested in
> Windows).

This doesn't surprise me...
--
Matthew Garrett | [email protected]

2007-08-03 15:11:14

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 10:41:13AM -0400, Alan Stern wrote:
> On Fri, 3 Aug 2007, Matthew Garrett wrote:
> > I'm not so
> > enthusiastic about the "Increase the timeout case" - it doesn't avoid
> > any races, just makes them less likely. USB is likely to get loaded in
> > the initramfs, but we may not have a full set of udev rules until the
> > root fs is up and that can take an effectively arbitrarily large amount
> > of time.
>
> If it takes longer than 15 minutes, something is wrong. At that point
> the user will have worse things to worry about than whether some USB
> devices got suspended.

Imagine cases where / is fscked from initramfs?

--
Matthew Garrett | [email protected]

2007-08-03 15:24:54

by David Brownell

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Friday 03 August 2007, Dave Jones wrote:
> ?> We have been playing with runtime autosuspend of HID devices, are
> ?> currently postponed the full support, as it turns out that many devices
> ?> don't support this feature properly (probably due to not being tested in
> ?> Windows).
>
> Interesting. ?Which devices did you notice failing?
> Was it a case that they would sleep and not come back out of that state?

And could you elaborate on "many"? What proportion of HID devices
(by volume, model, etc) seem to have problems?

I'd certainly expect there to be some HID devices which misbehave
(I have one, a cheap MacAlly trackball). Early testing seemed to
indicate such misbehavior was the exception to the rule ... folk
kept disbelieving me when I pointed out some HID devices misbehaved,
since nobody else had seen such a thing!

The specific failure I saw was that the device advertised itself as
supporting remote wakeup, but it couldn't issue that signaling. It
came back from suspend just fine ... but moving or clicking would
not do what it should.

- Dave

2007-08-03 15:31:18

by Jiri Kosina

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 3 Aug 2007, David Brownell wrote:

> And could you elaborate on "many"? What proportion of HID devices (by
> volume, model, etc) seem to have problems?

Last time I tried with two random USB keyboards - one from Logitech and
one from Chicony, I don't remember the exact PIDs, but could look them up
if it is interesting for someone.

> The specific failure I saw was that the device advertised itself as
> supporting remote wakeup, but it couldn't issue that signaling. It came
> back from suspend just fine ... but moving or clicking would not do what
> it should.

What I have been seeing with both these keyboards was: if connected to
UHCI controller, root hub not auto-suspended, as soon as they got
autosuspended, and keys were pressed on them rapidly, very often some
keypressess got lost. I didn't experience this on OHCI, but I remember
Alan saying that he triggered it on OHCI too, right?

Seemed like a timing issue - by lowering the polling timeout we were able
to make things much better, but that of course costs us more power etc.
and it's even not sure if it is an ultimate solution.

--
Jiri Kosina

2007-08-03 16:06:19

by Oliver Neukum

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

Am Freitag 03 August 2007 schrieb Matthew Garrett:
> > Which is why I didn't suggest doing that, of course. ?The only
> > one making that kind of straw man argument seems to be you.
>
> But however you phrase it, that's effectively what it is. "Does your
> device work?" just makes users wonder why the damn computer doesn't know
> already. "This option may prevent your device from working. Click here
> to switch it off" results in them wondering why it was switched on in
> the first place. Many of our users aren't technical - they don't care
> about saving 200mW, they just care about their printer working when they
> plug it in.

Devices rarely simply crash. Although Windows doesn't do runtime power
management, it certainly will suspend all devices when the system goes
into suspension. Buggy devices typically disconnect and reconnect when
resumed. This is testable for in software without user intervention.

Regards
Oliver

2007-08-03 16:30:09

by David Brownell

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Friday 03 August 2007, Matthew Garrett wrote:
> On Fri, Aug 03, 2007 at 07:37:55AM -0700, David Brownell wrote:
> > On Friday 03 August 2007, Matthew Garrett wrote:
> > > Popping up a box saying "Is your device broken?" isn't good UI.
> >
> > Which is why I didn't suggest doing that, of course. The only
> > one making that kind of straw man argument seems to be you.
>
> But however you phrase it, that's effectively what it is.

As http://en.wikipedia.org/wiki/Straw_Man phrases it,
"A straw man argument is an informal fallacy based on
misrepresentation of an opponent's position ... the
opponent's actual argument has not been refuted."

That's clearly what you've been doing by proposing some
specific -- and obviously bad -- user interfaces, none
of which have the fundamental characteristic I described.


> And, frankly, if I got a requestor like that every time I plugged in a
> new USB device I'd be fairly unhappy.

Which is why my comment was about something else entirely!

That is, having an out-of-kernel database which could preclude
the need for such requestors for devices already known.


> > If I were to describe any dialog users would see, it would be more
> > like "I don't recognize this device, help me set it up right...".
> > As with music CDs, that help might update the database for the next
> > person. (Assuming this were done well, of course.)
>
> Users understand CDs. They don't understand runtime power management.

A new straw man! Because power management isn't the relevant point.

All they'd ever have to understand is: is it working correctly right
now? That's after an experimental autosuspend, and after poking the
hardware to verify that, from the kernel perspective, it acts OK.

Oliver pointed out that the typical failure mode is easily detected
in software. So when the user says "OK, I'll help set it up", the
worst case would be that the device is NOT recorded already in that
database, the user might need to be ready to unplug then replug the
device (when that experiment fails).


> > Speaking of which, what's this /dev/bus/usb/* crap on Ubuntu?
> > I had to undo all that on my Feisty system before any normal
> > /proc/bus/usb stuff would work again.
>
> "Usbfs files can't handle Access Control Lists (ACL), which are the
> default way to grant access to USB devices for untrusted users of a
> desktop system. The usbfs functionality is replaced by real device-nodes
> managed by udev. These nodes live in /dev/bus/usb and are used by
> libusb."
>
> (From Kconfig)

That's shortly after the explanation that the relevant Kconfig
option is for ** /proc/bus/usb ** files ... note that despite the
strangeness in that text (usbfs still hasn't been "replaced", so
that should say "will eventually be replaced" not "is replaced"),
it's clear that /proc/bus/usb/ and /dev/bus/usb/ are two different
things. And thus: that Ubuntu's /dev/bus/usb/ setup is flakey.


> > > You commonly run a laptop off battery while having a printer plugged in?
> >
> > Unfortunately I need to run laptop off AC since its battery life is
> > painfully short, and since Linux behaves so incredibly rudely when
> > the battery power goes down to almost-zero: it lets it go to zero
> > rather than hibernating. (And doesn't automatically enter suspend
> > when it idles, either...)
>
> System/Preferences/Power Management

There's no option there to affect what happens when it's running
on battery power. Though I'm curious what it means when it has
a "suspend" option (not "hibernate") ... I wouldn't mind STR.


> > Note by the way that if you were -- for the sake of argument -- accept
> > my premise that this should all be handled in userspace ... it's very
> > easy to make userspace code do what you want. It doesn't need to be
> > done inside the kernel.
>
> It can be, but I'd prefer to have userspace enable functionality than
> have the kernel break things.

That side of things has been absent from the discussion so far.

When something is wrongly blacklisted (by whatever), how are you
proposing that it get un-blacklisted? Seems to me that whatever
mechanism resolves that issue should also work the other way around...

- Dave

2007-08-03 16:50:28

by Matthew Garrett

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 09:29:16AM -0700, David Brownell wrote:
> On Friday 03 August 2007, Matthew Garrett wrote:
> > And, frankly, if I got a requestor like that every time I plugged in a
> > new USB device I'd be fairly unhappy.
>
> Which is why my comment was about something else entirely!
>
> That is, having an out-of-kernel database which could preclude
> the need for such requestors for devices already known.

Plus a mechanism for pushing data into it, plus a mechanism for ensuring
that inaccurate data doesn't get in there, plus some what of pushing
updates out to users, plus privilege escalation for setting the value,
plus policy management to ensure that normal users can't mess with the
autosuspend values for other users? No, this isn't trivial - especially
when there's a straightforward in-kernel mechanism (only enable it when
it's known to be safe)

> > "Usbfs files can't handle Access Control Lists (ACL), which are the
> > default way to grant access to USB devices for untrusted users of a
> > desktop system. The usbfs functionality is replaced by real device-nodes
> > managed by udev. These nodes live in /dev/bus/usb and are used by
> > libusb."
> >
> > (From Kconfig)
>
> That's shortly after the explanation that the relevant Kconfig
> option is for ** /proc/bus/usb ** files ... note that despite the
> strangeness in that text (usbfs still hasn't been "replaced", so
> that should say "will eventually be replaced" not "is replaced"),
> it's clear that /proc/bus/usb/ and /dev/bus/usb/ are two different
> things. And thus: that Ubuntu's /dev/bus/usb/ setup is flakey.

Both /proc/bus/usb and /dev/bus/usb are provided. Anything that fails to
work with /dev/bus/usb is buggy - libusb copes fine. We're in the
process of transitioning away from the legacy interface. It could be
worse - we could have just removed it on the grounds that it doesn't
work properly.

> > System/Preferences/Power Management
>
> There's no option there to affect what happens when it's running
> on battery power. Though I'm curious what it means when it has
> a "suspend" option (not "hibernate") ... I wouldn't mind STR.

That's odd. In the "On battery power" tab I see an option to choose the
action when the battery power is critically low.

> > It can be, but I'd prefer to have userspace enable functionality than
> > have the kernel break things.
>
> That side of things has been absent from the discussion so far.
>
> When something is wrongly blacklisted (by whatever), how are you
> proposing that it get un-blacklisted? Seems to me that whatever
> mechanism resolves that issue should also work the other way around...

The worst case scenario in the "Disable by default, allow userspace to
whitelist" case is that some machines draw slightly more power. The
worst case scenario in the "Enable by default, allow userspace to
blacklist" case is that some hardware doesn't work because of race
conditions between autosuspend and userspace having the opportunity to
disable it, or devices not being in the blacklist, or somebody not
having adequately new usrspace, or...
--
Matthew Garrett | [email protected]

2007-08-03 17:42:17

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 01:32:53PM +0100, Matthew Garrett wrote:
> On Fri, Aug 03, 2007 at 02:26:43PM +0200, Rogan Dawes wrote:
>
> > Compare that to:
> >
> > "My USB printer broke, guess I'd better report it to LKML".
>
> But while this is still a likely probability, the chances are no
> distribution is going to ship with CONFIG_USB_SUSPEND enabled.

I wouldn't be so sure, I was thinking of doing just that based on an
internal conversation I had yesterday.

Let's see what breaks and what happens :)

thanks,

greg k-h

2007-08-03 17:46:51

by Greg KH

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 09:29:16AM -0700, David Brownell wrote:
> On Friday 03 August 2007, Matthew Garrett wrote:
> > > Speaking of which, what's this /dev/bus/usb/* crap on Ubuntu?
> > > I had to undo all that on my Feisty system before any normal
> > > /proc/bus/usb stuff would work again.
> >
> > "Usbfs files can't handle Access Control Lists (ACL), which are the
> > default way to grant access to USB devices for untrusted users of a
> > desktop system. The usbfs functionality is replaced by real device-nodes
> > managed by udev. These nodes live in /dev/bus/usb and are used by
> > libusb."
> >
> > (From Kconfig)
>
> That's shortly after the explanation that the relevant Kconfig
> option is for ** /proc/bus/usb ** files ... note that despite the
> strangeness in that text (usbfs still hasn't been "replaced", so
> that should say "will eventually be replaced" not "is replaced"),
> it's clear that /proc/bus/usb/ and /dev/bus/usb/ are two different
> things. And thus: that Ubuntu's /dev/bus/usb/ setup is flakey.

Hm, if you look at SuSE and Fedora, they too are putting usbfs in
/dev/bus/usb/ now, not mounting the filesystem, but using the device
nodes for access due to ACLs for local users.

libusb works just fine with this, and I think that all other programs
that directly access the old /proc/bus/usb mount are fixed up, with the
exception of usbview (but I do have patches floating around for that to
solve it.)

thanks,

greg k-h

2007-08-03 17:47:28

by Alan Stern

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 3 Aug 2007, Jiri Kosina wrote:

> What I have been seeing with both these keyboards was: if connected to
> UHCI controller, root hub not auto-suspended, as soon as they got
> autosuspended, and keys were pressed on them rapidly, very often some
> keypressess got lost. I didn't experience this on OHCI, but I remember
> Alan saying that he triggered it on OHCI too, right?
>
> Seemed like a timing issue - by lowering the polling timeout we were able
> to make things much better, but that of course costs us more power etc.
> and it's even not sure if it is an ultimate solution.

Jiri and I ran a few tests at OLS, and we each did additional testing
on our own. We looked at a small selection of keyboards; the ones I
tested were by Apple and HP. Some keyboards had embedded hubs and
others didn't. Some of our testing was with the keyboard behind an
external hub. Sometimes only the keyboard controller was suspended,
sometimes the controller and the embedded hub were, sometimes the
external hub and everything downstream of it were, and sometimes the
root hub was. We tested with both UHCI and OHCI -- I may even have
done some tests with EHCI and a high-speed hub, I don't remember now.

The end result was that some scenarios worked more reliably than
others. There were lots of variables and it was hard to tie overall
behavior with system settings. It did seem that in situations where
the topmost suspended device was plugged into a UHCI root hub,
increasing the the root hub's polling rate helped. But it didn't
always help, and in any case we certainly don't want to change a kernel
timer from 250 ms to 32 ms whenever a device is suspended!

The bad behavior we observed, as Jiri described, was that rapid typing
on a suspended keyboard would often cause one or more of the keystrokes
to be lost. The probability of this happening varied with the
circumstances, but I don't think I ever found a combination that was
100% reliable. It could well be a timing issue, or buffering --
there's no real way to know.

An additional drawback to autosuspend for keyboards is the fact that
the NumLock, CapsLock, etc. LEDs go out.

We didn't test any mice (at least, I didn't). However it has been
reported that while some suspended mice will send wakeup requests when
they are moved, others won't. Certainly an optical mouse won't.

All in all, it appears that the simplest and most user-friendly
approach is just not to autosuspend keyboards and mice.

Alan Stern

2007-08-03 17:48:47

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 10:44:35AM -0700, Greg KH wrote:
> On Fri, Aug 03, 2007 at 01:32:53PM +0100, Matthew Garrett wrote:
> > But while this is still a likely probability, the chances are no
> > distribution is going to ship with CONFIG_USB_SUSPEND enabled.
>
> I wouldn't be so sure, I was thinking of doing just that based on an
> internal conversation I had yesterday.

Well, we did - with hindsight it may not have been such a great plan :)
I believe that Fedora did as well, but have disabled it in an update
kernel.

--
Matthew Garrett | [email protected]

2007-08-03 17:49:29

by Alan Stern

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 3 Aug 2007, Oliver Neukum wrote:

> Devices rarely simply crash.

It's rare, but it does happen. I've seen a device get so messed up by
suspend that it needed a reset; it wouldn't be surprising to find other
devices requiring a power cycle.

Alan Stern

2007-08-03 19:30:07

by Chuck Ebbert

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On 08/03/2007 01:48 PM, Matthew Garrett wrote:
> On Fri, Aug 03, 2007 at 10:44:35AM -0700, Greg KH wrote:
>> On Fri, Aug 03, 2007 at 01:32:53PM +0100, Matthew Garrett wrote:
>>> But while this is still a likely probability, the chances are no
>>> distribution is going to ship with CONFIG_USB_SUSPEND enabled.
>> I wouldn't be so sure, I was thinking of doing just that based on an
>> internal conversation I had yesterday.
>
> Well, we did - with hindsight it may not have been such a great plan :)
> I believe that Fedora did as well, but have disabled it in an update
> kernel.

Yeah, autosuspend broke too many devices. Way too many.

2007-08-03 19:35:49

by Pete Zaitcev

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 3 Aug 2007 10:24:16 -0400, Dave Jones <[email protected]> wrote:
> On Fri, Aug 03, 2007 at 09:57:45AM +0200, Oliver Neukum wrote:
>
> > Kernel developers are a diverser lot than you think ;-)
> > We don't enable autosuspend in drivers we can't test, except where
> > the lack of a kernel driver forces us to use a broad swipe. Printers
> > were tested, too, and most printers seem to work.
>
> My experience suggests the opposite. Of the several I've tried so far,
> none have worked with usb suspend.

All of mine work. I'm wondering if this has something to do with
a hub or motherboard... How should we test it? Tried plugging elsewhere?

-- Pete

2007-08-03 19:45:54

by Dave Jones

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 12:34:47PM -0700, Pete Zaitcev wrote:
> On Fri, 3 Aug 2007 10:24:16 -0400, Dave Jones <[email protected]> wrote:
> > On Fri, Aug 03, 2007 at 09:57:45AM +0200, Oliver Neukum wrote:
> >
> > > Kernel developers are a diverser lot than you think ;-)
> > > We don't enable autosuspend in drivers we can't test, except where
> > > the lack of a kernel driver forces us to use a broad swipe. Printers
> > > were tested, too, and most printers seem to work.
> >
> > My experience suggests the opposite. Of the several I've tried so far,
> > none have worked with usb suspend.
>
> All of mine work. I'm wondering if this has something to do with
> a hub or motherboard... How should we test it? Tried plugging elsewhere?

no hubs involved, directly plugged them into the mainboard.
Fairly recent Intel chipsets on all the systems I tested.

Dave

--
http://www.codemonkey.org.uk

2007-08-03 20:01:23

by Pete Zaitcev

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 03 Aug 2007 15:29:21 -0400, Chuck Ebbert <[email protected]> wrote:

> > Well, we did - with hindsight it may not have been such a great plan :)
> > I believe that Fedora did as well, but have disabled it in an update
> > kernel.
>
> Yeah, autosuspend broke too many devices. Way too many.

Glad you chimed in, Chuck. I was wondering about it... I saw something
like 3 reports about broken printers, two of them for the same printer,
Samsung ML-2010. It's on the installed base of 300,000 to 500,000.
People simply do not report squat. Today DaveJ said that none of his
printers work, which sounds bad, and was news to me. But the point is,
we are trying to extrapolate from too few cases.

I am wondering if Ubuntu has better user reporting, so if Matthew
complains, it really means something.

-- Pete

2007-08-03 20:03:49

by Pete Zaitcev

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 3 Aug 2007 15:45:32 -0400, Dave Jones <[email protected]> wrote:

> > > My experience suggests the opposite. Of the several I've tried so far,
> > > none have worked with usb suspend.
> >
> > All of mine work. I'm wondering if this has something to do with
> > a hub or motherboard... How should we test it? Tried plugging elsewhere?
>
> no hubs involved, directly plugged them into the mainboard.
> Fairly recent Intel chipsets on all the systems I tested.

Sounds bad.

BTW, when I took over from Vojtech, I looked at the suspend methods
for the printer and it seemed possible that they can only work
if the device is not open when suspend occurs. I was going to look
at that at the first opportunity.

So, I'd rather keep at least some kind of capability to suspend
printers, even if disabled by default. I thought Matthew's patch
was too harsh.

-- Pete

2007-08-03 20:10:54

by Dave Jones

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 06:08:11PM +0200, Oliver Neukum wrote:
> Am Freitag 03 August 2007 schrieb Matthew Garrett:
> > > Which is why I didn't suggest doing that, of course. ?The only
> > > one making that kind of straw man argument seems to be you.
> >
> > But however you phrase it, that's effectively what it is. "Does your
> > device work?" just makes users wonder why the damn computer doesn't know
> > already. "This option may prevent your device from working. Click here
> > to switch it off" results in them wondering why it was switched on in
> > the first place. Many of our users aren't technical - they don't care
> > about saving 200mW, they just care about their printer working when they
> > plug it in.
>
> Devices rarely simply crash. Although Windows doesn't do runtime power
> management, it certainly will suspend all devices when the system goes
> into suspension. Buggy devices typically disconnect and reconnect when
> resumed. This is testable for in software without user intervention.

The printer I mentioned earlier this thread wouldn't work again
until I physically unplugged and replugged the usb cable.
It spewed descriptor read errors every time I tried to talk to it.
Even unloading and reloading the usb modules didn't bring it back
to life.

Dave

--
http://www.codemonkey.org.uk

2007-08-03 20:19:34

by Dave Jones

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 10:44:35AM -0700, Greg KH wrote:
> On Fri, Aug 03, 2007 at 01:32:53PM +0100, Matthew Garrett wrote:
> > On Fri, Aug 03, 2007 at 02:26:43PM +0200, Rogan Dawes wrote:
> >
> > > Compare that to:
> > >
> > > "My USB printer broke, guess I'd better report it to LKML".
> >
> > But while this is still a likely probability, the chances are no
> > distribution is going to ship with CONFIG_USB_SUSPEND enabled.
>
> I wouldn't be so sure, I was thinking of doing just that based on an
> internal conversation I had yesterday.
>
> Let's see what breaks and what happens :)

here's a head start for you.

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=243038
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=246713
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=243953
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=242359

That's just the ones that were handy..

Dave

--
http://www.codemonkey.org.uk

2007-08-03 21:17:37

by Alan Stern

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, 3 Aug 2007, Dave Jones wrote:

> here's a head start for you.
>
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=243038
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=246713
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=243953
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=242359
>
> That's just the ones that were handy..

The last report appears to be related more to the EHCI-cpufreq problem,
for which a patch was recently posted.

Alan Stern

2007-08-03 21:34:01

by Matthew Garrett

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 05:17:24PM -0400, Alan Stern wrote:

> The last report appears to be related more to the EHCI-cpufreq problem,
> for which a patch was recently posted.

There seem to be multiple issues there, with at least one of them being
autosuspend related.

--
Matthew Garrett | [email protected]

2007-08-03 21:34:31

by Dave Jones

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes

On Fri, Aug 03, 2007 at 05:17:24PM -0400, Alan Stern wrote:
> On Fri, 3 Aug 2007, Dave Jones wrote:
>
> > here's a head start for you.
> >
> > https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=243038
> > https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=246713
> > https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=243953
> > https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=242359
> >
> > That's just the ones that were handy..
>
> The last report appears to be related more to the EHCI-cpufreq problem,
> for which a patch was recently posted.

I was a bit iffy about including that one, but decided to because
some of the reporters noted that the problem 'went away' after
we pushed out a kernel disabling usb suspend by default.
See comments 16 & 17.

Clearly not the problem everyone was seeing, but it looks like
a few people piled on one bug with the same symptom from multiple
problems.

Dave

--
http://www.codemonkey.org.uk

2007-08-07 09:15:05

by Amit Kucheria

[permalink] [raw]
Subject: Re: [PATCH] USB: Only enable autosuspend by default on certain device classes

On 8/3/07, Pete Zaitcev <[email protected]> wrote:

> I am wondering if Ubuntu has better user reporting, so if Matthew
> complains, it really means something.

https://bugs.launchpad.net/bugs/85488

Most of the current quirks list was assembled by Oliver from this thread IIRC.

/Amit

--
Amit Kucheria, Linux developer