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

From: Alan Stern
Date: Thu Aug 22 2019 - 10:50:01 EST


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
> >>>> suspended.
> >>>
> >>> Hi,
> >>>
> >>> 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.

Alan Stern