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

[Kai-Heng Feng <kai.heng.feng@canonical.com>]

at 22:49, Alan Stern <stern@rowland.harvard.edu> wrote:

> On Thu, 22 Aug 2019, Kai-Heng Feng wrote:
>
>> at 18:38, Oliver Neukum <oneukum@suse.com> wrote:
>>
>>> Am Donnerstag, den 22.08.2019, 18:04 +0800 schrieb Kai-Heng Feng:
>>>> Hi Oliver,
>>>>
>>>> at 17:45, Oliver Neukum <oneukum@suse.com> 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