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

[Kai-Heng Feng]

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
> > > > > > 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.

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

Kai-Heng

> Alan Stern