| Message ID | 20190822091744.3451-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0 | expand |
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. And it behaves like most hardware. If you do not want runtime PM for such devices, do not switch it on. The refcounting needs to be done correctly. This patch does something that udev should do and in a questionable manner. Regards Oliver
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? > 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. > If you do not want runtime PM for such devices, do not switch > it on. A device should work regardless of runtime PM status. > The refcounting needs to be done correctly. Will do. > > 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. Kai-Heng > > Regards > Oliver
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. > > 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. > > 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? 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. 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. > > 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. > > > > 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. Regards Oliver
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. Kai-Heng > > Regards > Oliver
Am Donnerstag, den 22.08.2019, 21:23 +0800 schrieb Kai-Heng Feng: > at 18:38, Oliver Neukum <oneukum@suse.com> wrote: > > 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? Yes. Have you tested using a button? If they indeed do not work, then the device lies about supporting remote wakeup. That would warrant a quirk, but for remote wakeup. > Or it’s okay to let the device be suspended when remote wakeup is needed > but broken? Again, the HID spec does not specify what should trigger a remote wakeup. Limiting this to mouse buttons but not movements is inconvinient, but not buggy. This is indeed what Windows does. The device is suspended when the screen saver switches on. That we do not do that is a deficiency of X. To use runtime PM regularly you need an .ini file > > 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 it is broken, as in crashing we could talk about it. If it merely does not do what you want, then, yes, that is for user space to deal with. > > 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. What then would be the difference between suspended and active? A small delay in data transfer? > > 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. That is just what we do if, for example, you choose between the configs of a USB device or when you use authorization. > Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance > the refcount? No, the refcount is good. If remote wakeup is totally broken, you need to introduce a quirk that will prevent the kernel from believing the device when it claims to support it. Regards Oliver
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. Alan Stern
at 21:37, Oliver Neukum <oneukum@suse.com> wrote: > Am Donnerstag, den 22.08.2019, 21:23 +0800 schrieb Kai-Heng Feng: >> at 18:38, Oliver Neukum <oneukum@suse.com> wrote: >>> 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? > > Yes. Have you tested using a button? If they indeed do not work, then > the device lies about supporting remote wakeup. That would warrant a > quirk, but for remote wakeup. Button click can wake the mouse up but not movement. > >> Or it’s okay to let the device be suspended when remote wakeup is needed >> but broken? > > Again, the HID spec does not specify what should trigger a remote > wakeup. Limiting this to mouse buttons but not movements is > inconvinient, but not buggy. Ok, I still find the behavior really surprising. > > This is indeed what Windows does. The device is suspended when the > screen saver switches on. That we do not do that is a deficiency > of X. > To use runtime PM regularly you need an .ini file Thanks for the explanation. I guess we can mimic the behavior in systemd or upower. > > >>> 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 it is broken, as in crashing we could talk about it. If it merely > does not do what you want, then, yes, that is for user space to deal > with. Ok, I’ll take a look at userspace then. > >>> 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. > > What then would be the difference between suspended and active? A small > delay in data transfer? Non-operational but with wakeup capability and vise versa. > >>> 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. > > That is just what we do if, for example, you choose between the configs > of a USB device or when you use authorization. > >> Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance >> the refcount? > > No, the refcount is good. If remote wakeup is totally broken, you need > to introduce a quirk that will prevent the kernel from believing the > device when it claims to support it. Ok. I’ll see if it’s possible to mimic other OS under current Linux Desktop. Kai-Heng > > Regards > Oliver
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
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c index 166f41f3173b..40574f856a93 100644 --- a/drivers/hid/hid-quirks.c +++ b/drivers/hid/hid-quirks.c @@ -108,7 +108,7 @@ static const struct hid_device_id hid_quirks[] = { { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_C05A), HID_QUIRK_ALWAYS_POLL }, { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOUSE_C06A), HID_QUIRK_ALWAYS_POLL }, { HID_USB_DEVICE(USB_VENDOR_ID_MCS, USB_DEVICE_ID_MCS_GAMEPADBLOCK), HID_QUIRK_MULTI_INPUT }, - { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PIXART_MOUSE), HID_QUIRK_ALWAYS_POLL }, + { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PIXART_MOUSE), HID_QUIRK_NO_RUNTIME_PM }, { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_POWER_COVER), HID_QUIRK_NO_INIT_REPORTS }, { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_SURFACE_PRO_2), HID_QUIRK_NO_INIT_REPORTS }, { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_TOUCH_COVER_2), HID_QUIRK_NO_INIT_REPORTS }, diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index c7bc9db5b192..08a6b4f5cfb2 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -713,7 +713,8 @@ static int usbhid_open(struct hid_device *hid) } } - usb_autopm_put_interface(usbhid->intf); + if (!(hid->quirks & HID_QUIRK_NO_RUNTIME_PM)) + usb_autopm_put_interface(usbhid->intf); /* * In case events are generated while nobody was listening, diff --git a/include/linux/hid.h b/include/linux/hid.h index d770ab1a0479..bec413226146 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -342,6 +342,7 @@ struct hid_item { /* BIT(9) reserved for backward compatibility, was NO_INIT_INPUT_REPORTS */ #define HID_QUIRK_ALWAYS_POLL BIT(10) #define HID_QUIRK_INPUT_PER_APP BIT(11) +#define HID_QUIRK_NO_RUNTIME_PM BIT(12) #define HID_QUIRK_SKIP_OUTPUT_REPORTS BIT(16) #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID BIT(17) #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP BIT(18)
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. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/hid/hid-quirks.c | 2 +- drivers/hid/usbhid/hid-core.c | 3 ++- include/linux/hid.h | 1 + 3 files changed, 4 insertions(+), 2 deletions(-)