| Message ID | 20230222013944.31095-1-michael@allwinnertech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | HID: usbhid: enable remote wakeup for mice | expand |
On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote: > This patch fixes a problem that USB mouse can't wake up the device that > enters standby. This not a problem, it is that way by design. > At present, the kernel only checks whether certain USB manufacturers > support wake-up, which will easily cause inconvenience to the > development work of other manufacturers and add unnecessary work to the > maintenance of kernel. > > The USB protocol supports judging whether a usb supports the wake-up > function, so it should be more reasonable to add a wake-up source by > directly checking the settings from the USB protocol. But you do not do that in this patch, why not? > There was a similar issue on the keyboard before, which was fixed by > this patch (3d61510f4eca), but now the problem happened on the mouse. > This patch uses a similar idea to fix this problem. > > Signed-off-by: Michael Wu <michael@allwinnertech.com> > --- > drivers/hid/usbhid/hid-core.c | 8 ++++++++ > drivers/hid/usbhid/usbmouse.c | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c > index be4c731aaa65..d3a6755cca09 100644 > --- a/drivers/hid/usbhid/hid-core.c > +++ b/drivers/hid/usbhid/hid-core.c > @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid) > device_set_wakeup_enable(&dev->dev, 1); > } > > + /** > + * NOTE: enable remote wakeup by default for all mouse devices > + * supporting the boot protocol. > + */ > + if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT && > + interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE) > + device_set_wakeup_enable(&dev->dev, 1); Sorry, but we can not take this unless it is proven that this will work properly for all of these devices. Other operating systems do not do this last I checked, so there will be problems. thanks, greg k-h
Hello! On 2/22/23 4:39 AM, Michael Wu wrote: > This patch fixes a problem that USB mouse can't wake up the device that > enters standby. > > At present, the kernel only checks whether certain USB manufacturers > support wake-up, which will easily cause inconvenience to the > development work of other manufacturers and add unnecessary work to the > maintenance of kernel. > > The USB protocol supports judging whether a usb supports the wake-up > function, so it should be more reasonable to add a wake-up source by > directly checking the settings from the USB protocol. > > There was a similar issue on the keyboard before, which was fixed by > this patch (3d61510f4eca), but now the problem happened on the mouse. > This patch uses a similar idea to fix this problem. > > Signed-off-by: Michael Wu <michael@allwinnertech.com> > --- > drivers/hid/usbhid/hid-core.c | 8 ++++++++ > drivers/hid/usbhid/usbmouse.c | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c > index be4c731aaa65..d3a6755cca09 100644 > --- a/drivers/hid/usbhid/hid-core.c > +++ b/drivers/hid/usbhid/hid-core.c > @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid) > device_set_wakeup_enable(&dev->dev, 1); > } > > + /** The kernel-doc comments should not be used here in the function body. > + * NOTE: enable remote wakeup by default for all mouse devices > + * supporting the boot protocol. > + */ > + if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT && > + interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE) > + device_set_wakeup_enable(&dev->dev, 1); > + > mutex_unlock(&usbhid->mutex); > return 0; > [...] MBR, Sergey
On 22.02.23 02:39, Michael Wu wrote: > This patch fixes a problem that USB mouse can't wake up the device that > enters standby. Hi, I am afraid I need to ask you to be a bit more precise here. Are you referring to USB mice being unable to wake up a system, even if you so request via sysfs, or that they won't by default? Regards Oliver
+Richard On 2/22/2023 00:04, Greg KH wrote: > On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote: >> This patch fixes a problem that USB mouse can't wake up the device that >> enters standby. > > This not a problem, it is that way by design. > >> At present, the kernel only checks whether certain USB manufacturers >> support wake-up, which will easily cause inconvenience to the >> development work of other manufacturers and add unnecessary work to the >> maintenance of kernel. >> >> The USB protocol supports judging whether a usb supports the wake-up >> function, so it should be more reasonable to add a wake-up source by >> directly checking the settings from the USB protocol. > > But you do not do that in this patch, why not? > >> There was a similar issue on the keyboard before, which was fixed by >> this patch (3d61510f4eca), but now the problem happened on the mouse. >> This patch uses a similar idea to fix this problem. >> >> Signed-off-by: Michael Wu <michael@allwinnertech.com> >> --- >> drivers/hid/usbhid/hid-core.c | 8 ++++++++ >> drivers/hid/usbhid/usbmouse.c | 1 + >> 2 files changed, 9 insertions(+) >> >> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c >> index be4c731aaa65..d3a6755cca09 100644 >> --- a/drivers/hid/usbhid/hid-core.c >> +++ b/drivers/hid/usbhid/hid-core.c >> @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid) >> device_set_wakeup_enable(&dev->dev, 1); >> } >> >> + /** >> + * NOTE: enable remote wakeup by default for all mouse devices >> + * supporting the boot protocol. >> + */ >> + if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT && >> + interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE) >> + device_set_wakeup_enable(&dev->dev, 1); > > Sorry, but we can not take this unless it is proven that this will work > properly for all of these devices. Other operating systems do not do > this last I checked, so there will be problems. > > thanks, > > greg k-h > Richard and I both sent out relatively similar patches in the past, but they never went anywhere. We did confirm that Windows does set a similar policy as well though, which is what prompted us to attempt this. As there is more interest again maybe we can revive that discussion and merge together some ideas from the sets of patches. Previous submissions: v4: https://lore.kernel.org/linux-usb/20220825045517.16791-1-mario.limonciello@amd.com/ v3: https://lore.kernel.org/linux-usb/20220701023328.2783-10-mario.limonciello@amd.com/ v2: https://lore.kernel.org/linux-usb/20220616183142.14472-1-mario.limonciello@amd.com/ v1: https://lore.kernel.org/linux-usb/20220404214557.3329796-1-richard.gong@amd.com/
Dear Sergei: On 2/22/2023 4:59 PM, Sergei Shtylyov wrote: > Hello! > > On 2/22/23 4:39 AM, Michael Wu wrote: > >> This patch fixes a problem that USB mouse can't wake up the device that >> enters standby. >> >> At present, the kernel only checks whether certain USB manufacturers >> support wake-up, which will easily cause inconvenience to the >> development work of other manufacturers and add unnecessary work to the >> maintenance of kernel. >> >> The USB protocol supports judging whether a usb supports the wake-up >> function, so it should be more reasonable to add a wake-up source by >> directly checking the settings from the USB protocol. >> >> There was a similar issue on the keyboard before, which was fixed by >> this patch (3d61510f4eca), but now the problem happened on the mouse. >> This patch uses a similar idea to fix this problem. >> >> Signed-off-by: Michael Wu <michael@allwinnertech.com> >> --- >> drivers/hid/usbhid/hid-core.c | 8 ++++++++ >> drivers/hid/usbhid/usbmouse.c | 1 + >> 2 files changed, 9 insertions(+) >> >> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c >> index be4c731aaa65..d3a6755cca09 100644 >> --- a/drivers/hid/usbhid/hid-core.c >> +++ b/drivers/hid/usbhid/hid-core.c >> @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid) >> device_set_wakeup_enable(&dev->dev, 1); >> } >> >> + /** > > The kernel-doc comments should not be used here in the function body. We will remove the NOTE comments. > >> + * NOTE: enable remote wakeup by default for all mouse devices >> + * supporting the boot protocol. >> + */ >> + if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT && >> + interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE) >> + device_set_wakeup_enable(&dev->dev, 1); >> + >> mutex_unlock(&usbhid->mutex); >> return 0; >> > [...] > > MBR, Sergey
Dear Greg, On 2/22/2023 2:04 PM, Greg KH wrote: > On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote: >> This patch fixes a problem that USB mouse can't wake up the device that >> enters standby. > > This not a problem, it is that way by design. I got it, maybe it's a little problem to say that. > >> At present, the kernel only checks whether certain USB manufacturers >> support wake-up, which will easily cause inconvenience to the >> development work of other manufacturers and add unnecessary work to the >> maintenance of kernel. >> >> The USB protocol supports judging whether a usb supports the wake-up >> function, so it should be more reasonable to add a wake-up source by >> directly checking the settings from the USB protocol. > > But you do not do that in this patch, why not? I just want to explain the background of my patch, to prove we could use a similar way to avoid such a "disturbing" situation. To reduce the influence, my patch enables remote wakeup for USB mouse devices refer to what keyboard do. > >> There was a similar issue on the keyboard before, which was fixed by >> this patch (3d61510f4eca), but now the problem happened on the mouse. >> This patch uses a similar idea to fix this problem. >> >> Signed-off-by: Michael Wu <michael@allwinnertech.com> >> --- >> drivers/hid/usbhid/hid-core.c | 8 ++++++++ >> drivers/hid/usbhid/usbmouse.c | 1 + >> 2 files changed, 9 insertions(+) >> >> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c >> index be4c731aaa65..d3a6755cca09 100644 >> --- a/drivers/hid/usbhid/hid-core.c >> +++ b/drivers/hid/usbhid/hid-core.c >> @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid) >> device_set_wakeup_enable(&dev->dev, 1); >> } >> >> + /** >> + * NOTE: enable remote wakeup by default for all mouse devices >> + * supporting the boot protocol. >> + */ >> + if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT && >> + interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE) >> + device_set_wakeup_enable(&dev->dev, 1); > > Sorry, but we can not take this unless it is proven that this will work > properly for all of these devices. Other operating systems do not do > this last I checked, so there will be problems. As Mario Limonciello says, they has confirmed that the Microsoft Windows does set a similar policy as well. Can we talk about more in this topic: why does Linux not support it? Of course, if you have other great idea, I will appreciate that if we can have some further discussion. > > thanks, > > greg k-h
Dear Oliver, On 2/22/2023 5:34 PM, Oliver Neukum wrote: > On 22.02.23 02:39, Michael Wu wrote: >> This patch fixes a problem that USB mouse can't wake up the device that >> enters standby. > > Hi, > > I am afraid I need to ask you to be a bit more precise here. > Are you referring to USB mice being unable to wake up a system, > even if you so request via sysfs, or that they won't by default? > > Regards > Oliver Yes. I can't use any USB mouse which supports Remote Wakeup to wake up the system by default. If I enable the wakeup node in /sys/bus/usb/devices which is disabled by default, the mouse can wakeup the system successfully. Clearly, the only thing I can do is to register the wakeup source for USB Mouse devices.
On Thu, Feb 23, 2023 at 07:18:12PM +0800, Michael Wu wrote: > Dear Greg, > > On 2/22/2023 2:04 PM, Greg KH wrote: > > On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote: > > > This patch fixes a problem that USB mouse can't wake up the device that > > > enters standby. > > > > This not a problem, it is that way by design. > > I got it, maybe it's a little problem to say that. It is. > > > At present, the kernel only checks whether certain USB manufacturers > > > support wake-up, which will easily cause inconvenience to the > > > development work of other manufacturers and add unnecessary work to the > > > maintenance of kernel. > > > > > > The USB protocol supports judging whether a usb supports the wake-up > > > function, so it should be more reasonable to add a wake-up source by > > > directly checking the settings from the USB protocol. > > > > But you do not do that in this patch, why not? > > I just want to explain the background of my patch, to prove we could use a > similar way to avoid such a "disturbing" situation. > To reduce the influence, my patch enables remote wakeup for USB mouse > devices refer to what keyboard do. Keyboards are not mice :) > > > There was a similar issue on the keyboard before, which was fixed by > > > this patch (3d61510f4eca), but now the problem happened on the mouse. > > > This patch uses a similar idea to fix this problem. > > > > > > Signed-off-by: Michael Wu <michael@allwinnertech.com> > > > --- > > > drivers/hid/usbhid/hid-core.c | 8 ++++++++ > > > drivers/hid/usbhid/usbmouse.c | 1 + > > > 2 files changed, 9 insertions(+) > > > > > > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c > > > index be4c731aaa65..d3a6755cca09 100644 > > > --- a/drivers/hid/usbhid/hid-core.c > > > +++ b/drivers/hid/usbhid/hid-core.c > > > @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid) > > > device_set_wakeup_enable(&dev->dev, 1); > > > } > > > + /** > > > + * NOTE: enable remote wakeup by default for all mouse devices > > > + * supporting the boot protocol. > > > + */ > > > + if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT && > > > + interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE) > > > + device_set_wakeup_enable(&dev->dev, 1); > > > > Sorry, but we can not take this unless it is proven that this will work > > properly for all of these devices. Other operating systems do not do > > this last I checked, so there will be problems. > > As Mario Limonciello says, they has confirmed that the Microsoft Windows > does set a similar policy as well. Can we talk about more in this topic: why > does Linux not support it? > Of course, if you have other great idea, I will appreciate that if we can > have some further discussion. You need to provide some sort of "proof" that this has been heavily tested on a huge range of devices before we can change this. When this was first implemented, Windows did not work this way and many devices on the market were broken if this were to be enabled. I'm sure the mailing list archives from 20+ years ago have many more details, please dig around there for specifics. If you feel strongly that this is the way forward, why not do it in userspace today for your systems as part of testing this out? It should not require a kernel change, right? thanks, greg k-h
On 22.02.23 20:50, Limonciello, Mario wrote: > Richard and I both sent out relatively similar patches in the past, but they never went anywhere. The problem here is that the default will never satisfy a great majority. just a majority. So if we admit that there is no optimal solution, why not pick the tested, that is, current solution, as long as it does least? > We did confirm that Windows does set a similar policy as well though, which is what prompted us to attempt this. > > As there is more interest again maybe we can revive that discussion and merge together some ideas from the sets of patches. Well, the most obvious question, as this can also be done with a udev rule, is, what are the risks of a change? > Previous submissions: > > v4: > https://lore.kernel.org/linux-usb/20220825045517.16791-1-mario.limonciello@amd.com/ This one at least restricts it to some cases. And as much as it pains me, we need to ask whether there are risks in not emulating Windows. And on the other hand, what happens if we do this. Are you sure, for example, that you do not break use cases for S4 with this change? If a laptop goes into S4 because power is too low, we really do not want it to wake up just because somebody accidentally hits a mouse button. Regards Oliver
On 23.02.23 12:22, Michael Wu wrote: > Yes. I can't use any USB mouse which supports Remote Wakeup to wake up the system by default. > If I enable the wakeup node in /sys/bus/usb/devices which is disabled by default, the mouse can wakeup the system successfully. > Clearly, the only thing I can do is to register the wakeup source for USB Mouse devices. > Hi, thank you for answering this. I feel a long discussion coming up, which I would avoid if I could, so it is important that we are clear and do not discuss about a misunderstanding. Regards Oliver
On 23.02.23 12:23, Greg KH wrote: >> I just want to explain the background of my patch, to prove we could use a >> similar way to avoid such a "disturbing" situation. >> To reduce the influence, my patch enables remote wakeup for USB mouse >> devices refer to what keyboard do. > > Keyboards are not mice :) > OK, let me explain, why I never proposed switching on autosuspend for mice or using them as a system wakeup source. The reasons are very similar. Basically the standard gives us no way to ask a device what constitutes a wakeup event for it. We just get the blanket statement of support or no support. For runtime PM I would want my mouse to generate a remote wakeup whenever a button is pressed or the mouse is moved. Under this premise runtime PM with a mouse works wonderfully. Testing that, however, is a challenge. It turns out that mice that claim that they support remote wakeup by and large deactivate their LED/laser when you send them into suspension. Hence they react only to buttons being pressed or mouse wheels moved. As a system wakeup source a mouse that generates events when it is moved, however, would make the system unsuspendable, whenever even a bit of vibration is acting on the system. And as S4 is used in many setups to prevent an uncontrolled shutdown at low power, this must work. I suspect that most, but _not_ _all_ mice, are designed for use with a system that ties power management to the screen saver. That is a mode of operation that due to the lack of coupling between GUI and kernel is hard to copy. Frankly given these constraints the only default safe in every case seems to me to leave the kernel's default at off and put the work into udev's hwdb. Not that that is a good solution, merely the best I can come up with. Regards Oliver
[AMD Official Use Only - General] > -----Original Message----- > From: Oliver Neukum <oneukum@suse.com> > Sent: Thursday, February 23, 2023 06:02 > To: Greg KH <gregkh@linuxfoundation.org>; Michael Wu > <michael@allwinnertech.com> > Cc: jikos@kernel.org; benjamin.tissoires@redhat.com; linux- > usb@vger.kernel.org; linux-input@vger.kernel.org; linux- > kernel@vger.kernel.org; Limonciello, Mario <Mario.Limonciello@amd.com>; > Gong, Richard <Richard.Gong@amd.com> > Subject: Re: [PATCH] HID: usbhid: enable remote wakeup for mice > > On 23.02.23 12:23, Greg KH wrote: > > >> I just want to explain the background of my patch, to prove we could use > a > >> similar way to avoid such a "disturbing" situation. > >> To reduce the influence, my patch enables remote wakeup for USB > mouse > >> devices refer to what keyboard do. > > > > Keyboards are not mice :) > > > > OK, let me explain, why I never proposed switching on autosuspend > for mice or using them as a system wakeup source. The reasons are very > similar. > > Basically the standard gives us no way to ask a device what constitutes > a wakeup event for it. We just get the blanket statement of support > or no support. > > For runtime PM I would want my mouse to generate a remote wakeup > whenever a button is pressed or the mouse is moved. Under this > premise runtime PM with a mouse works wonderfully. Testing that, > however, is a challenge. > It turns out that mice that claim that they support remote wakeup > by and large deactivate their LED/laser when you send them into > suspension. Hence they react only to buttons being pressed or mouse > wheels moved. > > As a system wakeup source a mouse that generates events when > it is moved, however, would make the system unsuspendable, whenever > even > a bit of vibration is acting on the system. > And as S4 is used in many setups to prevent an uncontrolled shutdown > at low power, this must work. At least in my version of the series, this is part of the reason that it was only intended to be used with s2idle. The kernel driver is well aware of what power state you're in the suspend callback (pm_suspend_target_state). What about if we agreed to treat this one special by examining that? If the sysfs is set to "enabled" * During suspend if your target is s2idle -> program it * During suspend if your target is mem -> disable it * During suspend if your target is hibernate -> disable it With that type of policy on how to handle the suspend call in place perhaps we could set it to enabled by default? > > I suspect that most, but _not_ _all_ mice, are designed for use > with a system that ties power management to the screen saver. > That is a mode of operation that due to the lack of coupling > between GUI and kernel is hard to copy. > > Frankly given these constraints the only default safe in every > case seems to me to leave the kernel's default at off and > put the work into udev's hwdb. Not that that is a good solution, > merely the best I can come up with. > > Regards > Oliver Turning on "autosuspend" for USB mice makes them behave pretty similarly to how they work when they're marked for remote wakeup. On some mice the lasers turn off, and they only wakeup when you press a button or roll a wheel.
Dear Greg： On 2/23/2023 7:23 PM, Greg KH wrote: > On Thu, Feb 23, 2023 at 07:18:12PM +0800, Michael Wu wrote: >> Dear Greg, >> >> On 2/22/2023 2:04 PM, Greg KH wrote: >>> On Wed, Feb 22, 2023 at 09:39:44AM +0800, Michael Wu wrote: >>>> This patch fixes a problem that USB mouse can't wake up the device that >>>> enters standby. >>> >>> This not a problem, it is that way by design. >> >> I got it, maybe it's a little problem to say that. > > It is. > >>>> At present, the kernel only checks whether certain USB manufacturers >>>> support wake-up, which will easily cause inconvenience to the >>>> development work of other manufacturers and add unnecessary work to the >>>> maintenance of kernel. >>>> >>>> The USB protocol supports judging whether a usb supports the wake-up >>>> function, so it should be more reasonable to add a wake-up source by >>>> directly checking the settings from the USB protocol. >>> >>> But you do not do that in this patch, why not? >> >> I just want to explain the background of my patch, to prove we could use a >> similar way to avoid such a "disturbing" situation. >> To reduce the influence, my patch enables remote wakeup for USB mouse >> devices refer to what keyboard do. > > Keyboards are not mice :) Sorry, What I wanted to say is that we registered the mouse wake-up source by referring to the practice of the keyboard. > >>>> There was a similar issue on the keyboard before, which was fixed by >>>> this patch (3d61510f4eca), but now the problem happened on the mouse. >>>> This patch uses a similar idea to fix this problem. >>>> >>>> Signed-off-by: Michael Wu <michael@allwinnertech.com> >>>> --- >>>> drivers/hid/usbhid/hid-core.c | 8 ++++++++ >>>> drivers/hid/usbhid/usbmouse.c | 1 + >>>> 2 files changed, 9 insertions(+) >>>> >>>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c >>>> index be4c731aaa65..d3a6755cca09 100644 >>>> --- a/drivers/hid/usbhid/hid-core.c >>>> +++ b/drivers/hid/usbhid/hid-core.c >>>> @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid) >>>> device_set_wakeup_enable(&dev->dev, 1); >>>> } >>>> + /** >>>> + * NOTE: enable remote wakeup by default for all mouse devices >>>> + * supporting the boot protocol. >>>> + */ >>>> + if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT && >>>> + interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE) >>>> + device_set_wakeup_enable(&dev->dev, 1); >>> >>> Sorry, but we can not take this unless it is proven that this will work >>> properly for all of these devices. Other operating systems do not do >>> this last I checked, so there will be problems. >> >> As Mario Limonciello says, they has confirmed that the Microsoft Windows >> does set a similar policy as well. Can we talk about more in this topic: why >> does Linux not support it? >> Of course, if you have other great idea, I will appreciate that if we can >> have some further discussion. > > You need to provide some sort of "proof" that this has been heavily > tested on a huge range of devices before we can change this. > > When this was first implemented, Windows did not work this way and many > devices on the market were broken if this were to be enabled. I'm sure > the mailing list archives from 20+ years ago have many more details, > please dig around there for specifics. > > If you feel strongly that this is the way forward, why not do it in > userspace today for your systems as part of testing this out? It should > not require a kernel change, right? Thanks for your advises. I'm clear now. I will try it in userspace. > > thanks, > > greg k-h
On 23.02.23 20:41, Limonciello, Mario wrote: Hi, >> As a system wakeup source a mouse that generates events when >> it is moved, however, would make the system unsuspendable, whenever >> even >> a bit of vibration is acting on the system. >> And as S4 is used in many setups to prevent an uncontrolled shutdown >> at low power, this must work. > > At least in my version of the series, this is part of the reason that it was > only intended to be used with s2idle. Yes, that is sensible. If these patches are to be taken at all, that will be a necessary condition to meet. But it is not sufficient. > The kernel driver is well aware of what power state you're in the suspend > callback (pm_suspend_target_state). > > What about if we agreed to treat this one special by examining that? > > If the sysfs is set to "enabled" If user space needs to manipulate sysfs at all, we can have user space tell kernel space exactly what to do. Hence I see no point in conditional interpretations values in sysfs at that point. We are discussing the kernel's default here. > * During suspend if your target is s2idle -> program it > * During suspend if your target is mem -> disable it > * During suspend if your target is hibernate -> disable it To my mind these defaults make sense. However, do they make much more sense than what we are doing now? > With that type of policy on how to handle the suspend call in place > perhaps we could set it to enabled by default? It pains me to say, but I am afraid in that regard the only decision that will not cause ugly surprises is to follow Windows. Yet, what is wrong about the current defaults? > Turning on "autosuspend" for USB mice makes them behave pretty > similarly to how they work when they're marked for remote wakeup. Because it is exactly the same mechanism. > On some mice the lasers turn off, and they only wakeup when you > press a button or roll a wheel. Yes. And _some_ is the exact problem. If we could tell, _how_ mice react, this discussion were unnecessary. Regards Oliver
[Public] > -----Original Message----- > From: Oliver Neukum <oneukum@suse.com> > Sent: Tuesday, February 28, 2023 03:03 > To: Limonciello, Mario <Mario.Limonciello@amd.com>; Oliver Neukum > <oneukum@suse.com>; Greg KH <gregkh@linuxfoundation.org>; Michael > Wu <michael@allwinnertech.com> > Cc: jikos@kernel.org; benjamin.tissoires@redhat.com; linux- > usb@vger.kernel.org; linux-input@vger.kernel.org; linux- > kernel@vger.kernel.org; Gong, Richard <Richard.Gong@amd.com> > Subject: Re: [PATCH] HID: usbhid: enable remote wakeup for mice > > On 23.02.23 20:41, Limonciello, Mario wrote: > > Hi, > > >> As a system wakeup source a mouse that generates events when > >> it is moved, however, would make the system unsuspendable, whenever > >> even > >> a bit of vibration is acting on the system. > >> And as S4 is used in many setups to prevent an uncontrolled shutdown > >> at low power, this must work. > > > > At least in my version of the series, this is part of the reason that it was > > only intended to be used with s2idle. > > Yes, that is sensible. If these patches are to be taken at all, that will > be a necessary condition to meet. But it is not sufficient. Ack. > > > The kernel driver is well aware of what power state you're in the suspend > > callback (pm_suspend_target_state). > > > > What about if we agreed to treat this one special by examining that? > > > > If the sysfs is set to "enabled" > > If user space needs to manipulate sysfs at all, we can have user space > tell kernel space exactly what to do. Hence I see no point in > conditional interpretations values in sysfs at that point. > > We are discussing the kernel's default here. Right, I was meaning if the kernel defaulted to enabled or if userspace changed it to enabled to follow this behavior. > > > * During suspend if your target is s2idle -> program it > > * During suspend if your target is mem -> disable it > > * During suspend if your target is hibernate -> disable it > > To my mind these defaults make sense. > However, do they make much more sense than what we are doing now? If you're talking about purely "policy default", I think it makes more sense. Userspace can still change it, and it better aligns with what Windows does out of the box. > > > With that type of policy on how to handle the suspend call in place > > perhaps we could set it to enabled by default? > > It pains me to say, but I am afraid in that regard the only > decision that will not cause ugly surprises is to follow Windows. > Yet, what is wrong about the current defaults? I still keep getting inquiries about this where teams that work on the same hardware for Windows and Linux complain about this difference during their testing. I keep educating them to change it in sysfs (or to use a udev rule), but you have to question if you keep getting something asked about policy over and over if it's actually the right policy.
On Tue, Feb 28, 2023 at 06:50:18PM +0000, Limonciello, Mario wrote: > I still keep getting inquiries about this where teams that work on the same > hardware for Windows and Linux complain about this difference during > their testing. > > I keep educating them to change it in sysfs (or to use a udev rule), but > you have to question if you keep getting something asked about policy > over and over if it's actually the right policy. Why not complain to the Windows team to get them to change their policy back as they are the ones that changed it over time and are not backwards-compatible with older systems? :) thanks, greg k-h
[AMD Official Use Only - General] > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Tuesday, February 28, 2023 13:05 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Oliver Neukum <oneukum@suse.com>; Michael Wu > <michael@allwinnertech.com>; jikos@kernel.org; > benjamin.tissoires@redhat.com; linux-usb@vger.kernel.org; linux- > input@vger.kernel.org; linux-kernel@vger.kernel.org; Gong, Richard > <Richard.Gong@amd.com> > Subject: Re: [PATCH] HID: usbhid: enable remote wakeup for mice > > On Tue, Feb 28, 2023 at 06:50:18PM +0000, Limonciello, Mario wrote: > > I still keep getting inquiries about this where teams that work on the same > > hardware for Windows and Linux complain about this difference during > > their testing. > > > > I keep educating them to change it in sysfs (or to use a udev rule), but > > you have to question if you keep getting something asked about policy > > over and over if it's actually the right policy. > > Why not complain to the Windows team to get them to change their policy > back as they are the ones that changed it over time and are not > backwards-compatible with older systems? > Heh. I don't think that's actually true though - Modern Standby is relatively new. Picking new policies tied to that shouldn't break backwards compatibility.
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index be4c731aaa65..d3a6755cca09 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -1189,6 +1189,14 @@ static int usbhid_start(struct hid_device *hid) device_set_wakeup_enable(&dev->dev, 1); } + /** + * NOTE: enable remote wakeup by default for all mouse devices + * supporting the boot protocol. + */ + if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT && + interface->desc.bInterfaceProtocol == USB_INTERFACE_PROTOCOL_MOUSE) + device_set_wakeup_enable(&dev->dev, 1); + mutex_unlock(&usbhid->mutex); return 0; diff --git a/drivers/hid/usbhid/usbmouse.c b/drivers/hid/usbhid/usbmouse.c index 3fd93c2e4f4a..2fbc3f49e420 100644 --- a/drivers/hid/usbhid/usbmouse.c +++ b/drivers/hid/usbhid/usbmouse.c @@ -188,6 +188,7 @@ static int usb_mouse_probe(struct usb_interface *intf, const struct usb_device_i goto fail3; usb_set_intfdata(intf, mouse); + device_set_wakeup_enable(&dev->dev, 1); return 0; fail3:
This patch fixes a problem that USB mouse can't wake up the device that enters standby. At present, the kernel only checks whether certain USB manufacturers support wake-up, which will easily cause inconvenience to the development work of other manufacturers and add unnecessary work to the maintenance of kernel. The USB protocol supports judging whether a usb supports the wake-up function, so it should be more reasonable to add a wake-up source by directly checking the settings from the USB protocol. There was a similar issue on the keyboard before, which was fixed by this patch (3d61510f4eca), but now the problem happened on the mouse. This patch uses a similar idea to fix this problem. Signed-off-by: Michael Wu <michael@allwinnertech.com> --- drivers/hid/usbhid/hid-core.c | 8 ++++++++ drivers/hid/usbhid/usbmouse.c | 1 + 2 files changed, 9 insertions(+)