Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0

From: Kai-Heng Feng
Date: Sat Aug 24 2019 - 12:30:31 EST

at 22:49, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:

On Thu, 22 Aug 2019, Kai-Heng Feng wrote:

at 18:38, Oliver Neukum <oneukum@xxxxxxxx> wrote:

Am Donnerstag, den 22.08.2019, 18:04 +0800 schrieb Kai-Heng Feng:
Hi Oliver,

at 17:45, Oliver Neukum <oneukum@xxxxxxxx> wrote:

Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng:
The optical sensor of the mouse gets turned off when it's runtime
suspended, so moving the mouse can't wake the mouse up, despite that
USB remote wakeup is successfully set.

Introduce a new quirk to prevent the mouse from getting runtime


I am afraid this is a bad approach in principle. The device
behaves according to spec.

Can you please point out which spec it is? Is it USB 2.0 spec?

Well, sort of. The USB spec merely states how to enter and exit
a suspended state and that device state must not be lost.
It does not tell you what a suspended device must be able to do.

But shouldnât remote wakeup signaling wakes the device up and let it exit
suspend state?
Or itâs okay to let the device be suspended when remote wakeup is needed
but broken?

And it behaves like most hardware.

So seems like most hardware are broken.
Maybe a more appropriate solution is to disable RPM for all USB mice.

That is a decision a distro certainly can make. However, the kernel
does not, by default, call usb_enable_autosuspend() for HID devices
for this very reason. It is enabled by default only for hubs,
BT dongles and UVC cameras (and some minor devices)

In other words, if on your system it is on, you need to look
at udev, not the kernel.

So if a device is broken when âpower/controlâ is flipped by user, we should
deal it at userspace? That doesnât sound right to me.

If you do not want runtime PM for such devices, do not switch
it on.

A device should work regardless of runtime PM status.

Well, no. Runtime PM is a trade off. You lose something if you use
it. If it worked just as well as full power, you would never use
full power, would you?

I am not asking the suspended state to work as full power, but to prevent a
device enters suspend state because of broken remote wakeup.

Whether the loss of functionality or performance is worth the energy
savings is a policy decision. Hence it belongs into udev.
Ideally the kernel would tell user space what will work in a
suspended state. Unfortunately HID does not provide support for that.

I really donât think âloss of functionallyâ belongs to policy decision. But
thatâs just my opinion.

This is a deficiency of user space. The kernel has an ioctl()
to let user space tell it, whether a device is fully needed.
X does not use them.

Ok, Iâll take a look at other device drivers that use it.

The refcounting needs to be done correctly.

Will do.

Well, I am afraid your patch breaks it and if you do not break
it, the patch is reduced to nothing.

Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance
the refcount?

This patch does something that udev should do and in a
questionable manner.

IMO if the device doesnât support runtime suspend, then it needs to be
disabled in kernel but not workaround in userspace.

You switch it on from user space. Of course the kernel default
must be safe, as you said. It already is.

Iâd also like to hear maintainers' opinion on this issue.

I agree with Oliver. There is no formal requirement on what actions
should cause a mouse to generate a remote wakeup request. Some mice
will do it when they are moved and some mice won't.

If you don't like the way a particular mouse behaves then you should
not allow it to go into runtime suspend. By default, the kernel
prevents _all_ USB mice from being runtime suspended; the only way a
mouse can be suspended is if some userspace program tells the kernel to
allow it.

It might be a udev script which does this, or a powertop setting, or
something else. Regardless, what the kernel does is correct.
Furthermore, the kernel has to accomodate users who don't mind pressing
a mouse button to wake up their mice. For their sake, the kernel
should not forbid a mouse from ever going into runtime suspend merely
because it won't generate a wakeup request when it is moved.

True, if some users donât mind clicking mouse button before using it then we need to keep the current behavior.


Alan Stern