linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device
@ 2021-12-08  9:39 Yinbo Zhu
  2021-12-08  9:39 ` [PATCH v1 2/2] usb: core: enable remote wakeup function for usb controller Yinbo Zhu
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Yinbo Zhu @ 2021-12-08  9:39 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Greg Kroah-Hartman,
	Thinh Nguyen, Mathias Nyman, Alan Stern, Rajat Jain, Chris Chiu,
	linux-usb, linux-input, linux-kernel
  Cc: Yinbo Zhu

The remote wake-up function is a regular function on usb hid device
and I think keeping it enabled by default will make usb application
more convenient. This patch is to enable remote wakeup function for
usb hid device.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
 drivers/hid/usbhid/hid-core.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 2dcaf31..3619b95 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1063,7 +1063,9 @@ static int usbhid_start(struct hid_device *hid)
 	struct usb_host_interface *interface = intf->cur_altsetting;
 	struct usb_device *dev = interface_to_usbdev(intf);
 	struct usbhid_device *usbhid = hid->driver_data;
+	struct usb_host_config *config;
 	unsigned int n, insize = 0;
+	int ncfg;
 	int ret;
 
 	mutex_lock(&usbhid->mutex);
@@ -1179,14 +1181,19 @@ static int usbhid_start(struct hid_device *hid)
 	/* Some keyboards don't work until their LEDs have been set.
 	 * Since BIOSes do set the LEDs, it must be safe for any device
 	 * that supports the keyboard boot protocol.
-	 * In addition, enable remote wakeup by default for all keyboard
-	 * devices supporting the boot protocol.
 	 */
 	if (interface->desc.bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT &&
 			interface->desc.bInterfaceProtocol ==
 				USB_INTERFACE_PROTOCOL_KEYBOARD) {
 		usbhid_set_leds(hid);
-		device_set_wakeup_enable(&dev->dev, 1);
+	}
+
+	for (ncfg = 0; ncfg < dev->descriptor.bNumConfigurations; ncfg++) {
+		config = &dev->config[ncfg];
+		if ((config->desc.bmAttributes & (1 << 5)) == 0)
+			break;
+		if (ncfg + 1 == dev->descriptor.bNumConfigurations)
+			device_set_wakeup_enable(&dev->dev, 1);
 	}
 
 	mutex_unlock(&usbhid->mutex);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v1 2/2] usb: core: enable remote wakeup function for usb controller
  2021-12-08  9:39 [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device Yinbo Zhu
@ 2021-12-08  9:39 ` Yinbo Zhu
  2021-12-08 22:04   ` Alan Stern
  2021-12-08 10:03 ` [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device Oliver Neukum
  2021-12-08 11:55 ` Greg Kroah-Hartman
  2 siblings, 1 reply; 15+ messages in thread
From: Yinbo Zhu @ 2021-12-08  9:39 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Greg Kroah-Hartman,
	Thinh Nguyen, Mathias Nyman, Alan Stern, Rajat Jain, Chris Chiu,
	linux-usb, linux-input, linux-kernel
  Cc: Yinbo Zhu

The remote wake up function is a regular function on usb device and
I think keeping it enabled by default will make the usb application
more convenient and usb device remote wake up function keep enabled
that ask usb controller remote wake up was enabled at first.

This patch only enable wake up on usb root hub device, among which,
usb3.0 root hub doesn't be set wakeup node property but use command
USB_INTRF_FUNC_SUSPEND to enable remote wake up function.

Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
---
 drivers/usb/core/hub.c | 20 ++++++++++++++++++--
 include/linux/usb.h    |  4 +++-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 86658a8..cb4b956 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2509,6 +2509,8 @@ static void set_usb_port_removable(struct usb_device *udev)
  */
 int usb_new_device(struct usb_device *udev)
 {
+	struct usb_host_config *config;
+	int ncfg;
 	int err;
 
 	if (udev->parent) {
@@ -2540,6 +2542,18 @@ int usb_new_device(struct usb_device *udev)
 	udev->dev.devt = MKDEV(USB_DEVICE_MAJOR,
 			(((udev->bus->busnum-1) * 128) + (udev->devnum-1)));
 
+	for (ncfg = 0; ncfg < udev->descriptor.bNumConfigurations; ncfg++) {
+		config = &udev->config[ncfg];
+		if ((config->desc.bmAttributes & (1 << 5)) == 0)
+			break;
+		if (ncfg + 1 == udev->descriptor.bNumConfigurations) {
+			err = usb_enable_remote_wakeup(udev);
+			if (err)
+				dev_dbg(&udev->dev,
+				      "won't remote wakeup, err %d\n", err);
+		}
+	}
+
 	/* Tell the world! */
 	announce_device(udev);
 
@@ -3234,7 +3248,7 @@ void usb_enable_ltm(struct usb_device *udev)
  * enable remote wake for the first interface.  FIXME if the interface
  * association descriptor shows there's more than one function.
  */
-static int usb_enable_remote_wakeup(struct usb_device *udev)
+int usb_enable_remote_wakeup(struct usb_device *udev)
 {
 	if (udev->speed < USB_SPEED_SUPER)
 		return usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
@@ -3249,6 +3263,7 @@ static int usb_enable_remote_wakeup(struct usb_device *udev)
 					USB_INTRF_FUNC_SUSPEND_LP,
 				NULL, 0, USB_CTRL_SET_TIMEOUT);
 }
+EXPORT_SYMBOL_GPL(usb_enable_remote_wakeup);
 
 /*
  * usb_disable_remote_wakeup - disable remote wakeup for a device
@@ -3260,7 +3275,7 @@ static int usb_enable_remote_wakeup(struct usb_device *udev)
  * disable remote wake for the first interface.  FIXME if the interface
  * association descriptor shows there's more than one function.
  */
-static int usb_disable_remote_wakeup(struct usb_device *udev)
+int usb_disable_remote_wakeup(struct usb_device *udev)
 {
 	if (udev->speed < USB_SPEED_SUPER)
 		return usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
@@ -3273,6 +3288,7 @@ static int usb_disable_remote_wakeup(struct usb_device *udev)
 				USB_INTRF_FUNC_SUSPEND,	0, NULL, 0,
 				USB_CTRL_SET_TIMEOUT);
 }
+EXPORT_SYMBOL_GPL(usb_disable_remote_wakeup);
 
 /* Count of wakeup-enabled devices at or below udev */
 unsigned usb_wakeup_enabled_descendants(struct usb_device *udev)
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 7ccaa76..8097eee 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -754,8 +754,10 @@ static inline bool usb_acpi_power_manageable(struct usb_device *hdev, int index)
 	{ return true; }
 #endif
 
-/* USB autosuspend and autoresume */
 #ifdef CONFIG_PM
+extern int usb_enable_remote_wakeup(struct usb_device *udev);
+extern int usb_disable_remote_wakeup(struct usb_device *udev);
+
 extern void usb_enable_autosuspend(struct usb_device *udev);
 extern void usb_disable_autosuspend(struct usb_device *udev);
 
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device
  2021-12-08  9:39 [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device Yinbo Zhu
  2021-12-08  9:39 ` [PATCH v1 2/2] usb: core: enable remote wakeup function for usb controller Yinbo Zhu
@ 2021-12-08 10:03 ` Oliver Neukum
  2021-12-10  9:50   ` zhuyinbo
  2021-12-08 11:55 ` Greg Kroah-Hartman
  2 siblings, 1 reply; 15+ messages in thread
From: Oliver Neukum @ 2021-12-08 10:03 UTC (permalink / raw)
  To: Yinbo Zhu, Jiri Kosina, Benjamin Tissoires, Greg Kroah-Hartman,
	Thinh Nguyen, Mathias Nyman, Alan Stern, Rajat Jain, Chris Chiu,
	linux-usb, linux-input, linux-kernel


On 08.12.21 10:39, Yinbo Zhu wrote:
> The remote wake-up function is a regular function on usb hid device
> and I think keeping it enabled by default will make usb application
> more convenient. This patch is to enable remote wakeup function for
> usb hid device.
>
Hi,

I am afraid we cannot do this. It will cause regression.
Consider for example the case of laptops with touchscreens
that will trigger a wake up when the laptop is closed.

    Regards

        Oliver


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device
  2021-12-08  9:39 [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device Yinbo Zhu
  2021-12-08  9:39 ` [PATCH v1 2/2] usb: core: enable remote wakeup function for usb controller Yinbo Zhu
  2021-12-08 10:03 ` [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device Oliver Neukum
@ 2021-12-08 11:55 ` Greg Kroah-Hartman
  2021-12-10  9:54   ` zhuyinbo
  2 siblings, 1 reply; 15+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-08 11:55 UTC (permalink / raw)
  To: Yinbo Zhu
  Cc: Jiri Kosina, Benjamin Tissoires, Thinh Nguyen, Mathias Nyman,
	Alan Stern, Rajat Jain, Chris Chiu, linux-usb, linux-input,
	linux-kernel

On Wed, Dec 08, 2021 at 05:39:50PM +0800, Yinbo Zhu wrote:
> The remote wake-up function is a regular function on usb hid device
> and I think keeping it enabled by default will make usb application
> more convenient. This patch is to enable remote wakeup function for
> usb hid device.

How many devices did you test this on?

As Oliver said, this will cause problems, there's a reason no operating
system does this :(

sorry,

greg k-h

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/2] usb: core: enable remote wakeup function for usb controller
  2021-12-08  9:39 ` [PATCH v1 2/2] usb: core: enable remote wakeup function for usb controller Yinbo Zhu
@ 2021-12-08 22:04   ` Alan Stern
  2021-12-10  9:27     ` zhuyinbo
  0 siblings, 1 reply; 15+ messages in thread
From: Alan Stern @ 2021-12-08 22:04 UTC (permalink / raw)
  To: Yinbo Zhu
  Cc: Jiri Kosina, Benjamin Tissoires, Greg Kroah-Hartman,
	Thinh Nguyen, Mathias Nyman, Rajat Jain, Chris Chiu, linux-usb,
	linux-input, linux-kernel

On Wed, Dec 08, 2021 at 05:39:51PM +0800, Yinbo Zhu wrote:
> The remote wake up function is a regular function on usb device and
> I think keeping it enabled by default will make the usb application
> more convenient and usb device remote wake up function keep enabled
> that ask usb controller remote wake up was enabled at first.
> 
> This patch only enable wake up on usb root hub device, among which,

You say the patch only affects root hub devices, but this doesn't appear 
to be true.

> usb3.0 root hub doesn't be set wakeup node property but use command
> USB_INTRF_FUNC_SUSPEND to enable remote wake up function.
> 
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> ---
>  drivers/usb/core/hub.c | 20 ++++++++++++++++++--
>  include/linux/usb.h    |  4 +++-
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 86658a8..cb4b956 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2509,6 +2509,8 @@ static void set_usb_port_removable(struct usb_device *udev)
>   */
>  int usb_new_device(struct usb_device *udev)
>  {
> +	struct usb_host_config *config;
> +	int ncfg;
>  	int err;
>  
>  	if (udev->parent) {
> @@ -2540,6 +2542,18 @@ int usb_new_device(struct usb_device *udev)
>  	udev->dev.devt = MKDEV(USB_DEVICE_MAJOR,
>  			(((udev->bus->busnum-1) * 128) + (udev->devnum-1)));
>  
> +	for (ncfg = 0; ncfg < udev->descriptor.bNumConfigurations; ncfg++) {
> +		config = &udev->config[ncfg];
> +		if ((config->desc.bmAttributes & (1 << 5)) == 0)
> +			break;
> +		if (ncfg + 1 == udev->descriptor.bNumConfigurations) {
> +			err = usb_enable_remote_wakeup(udev);
> +			if (err)
> +				dev_dbg(&udev->dev,
> +				      "won't remote wakeup, err %d\n", err);
> +		}
> +	}

I don't see anything in there which treats root hubs differently from 
other devices.

Besides, enabling wakeup for root hubs is generally a bad idea.  Suppose 
you closed a laptop's lid and then unplugged a USB device -- with wakeup 
enabled, the unplug would cause the laptop to wake up again without your 
knowledge.

Alan Stern

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/2] usb: core: enable remote wakeup function for usb controller
  2021-12-08 22:04   ` Alan Stern
@ 2021-12-10  9:27     ` zhuyinbo
  2021-12-10 16:35       ` Alan Stern
  0 siblings, 1 reply; 15+ messages in thread
From: zhuyinbo @ 2021-12-10  9:27 UTC (permalink / raw)
  To: Alan Stern
  Cc: Jiri Kosina, benjamin.tissoires, gregkh, Thinh.Nguyen,
	mathias.nyman, rajatja, chris.chiu, linux-usb, linux-input,
	linux-kernel, zhuyinbo, Greg Kroah-Hartman, Thinh Nguyen,
	Mathias Nyman, Rajat Jain, Chris Chiu, linux-usb, linux-input,
	linux-kernel



在 2021/12/9 上午6:04, Alan Stern 写道:
> On Wed, Dec 08, 2021 at 05:39:51PM +0800, Yinbo Zhu wrote:
>> The remote wake up function is a regular function on usb device and
>> I think keeping it enabled by default will make the usb application
>> more convenient and usb device remote wake up function keep enabled
>> that ask usb controller remote wake up was enabled at first.
>>
>> This patch only enable wake up on usb root hub device, among which,
> 
> You say the patch only affects root hub devices, but this doesn't appear
> to be true.
> 
>> usb3.0 root hub doesn't be set wakeup node property but use command
>> USB_INTRF_FUNC_SUSPEND to enable remote wake up function.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
>> ---
>>   drivers/usb/core/hub.c | 20 ++++++++++++++++++--
>>   include/linux/usb.h    |  4 +++-
>>   2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 86658a8..cb4b956 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -2509,6 +2509,8 @@ static void set_usb_port_removable(struct usb_device *udev)
>>    */
>>   int usb_new_device(struct usb_device *udev)
>>   {
>> +	struct usb_host_config *config;
>> +	int ncfg;
>>   	int err;
>>   
>>   	if (udev->parent) {
>> @@ -2540,6 +2542,18 @@ int usb_new_device(struct usb_device *udev)
>>   	udev->dev.devt = MKDEV(USB_DEVICE_MAJOR,
>>   			(((udev->bus->busnum-1) * 128) + (udev->devnum-1)));
>>   
>> +	for (ncfg = 0; ncfg < udev->descriptor.bNumConfigurations; ncfg++) {
>> +		config = &udev->config[ncfg];
>> +		if ((config->desc.bmAttributes & (1 << 5)) == 0)
>> +			break;
>> +		if (ncfg + 1 == udev->descriptor.bNumConfigurations) {
>> +			err = usb_enable_remote_wakeup(udev);
>> +			if (err)
>> +				dev_dbg(&udev->dev,
>> +				      "won't remote wakeup, err %d\n", err);
>> +		}
>> +	}
> 
> I don't see anything in there which treats root hubs differently from
> other devices.
> 
Hi Alan Stern,

You can find following code, non-root-hub had removed Wakeup sysfs 
attributes and disabled wakeup and root-hub had added wakeup sysfs 
attibutes before call usb_new_device, so this patch was only enabled
remote wakeup for root-hub device.
int usb_new_device(struct usb_device *udev)
{
         if (udev->parent) {
                 /* Initialize non-root-hub device wakeup to disabled;
                  * device (un)configuration controls wakeup capable
                  * sysfs power/wakeup controls wakeup enabled/disabled
                  */
                 device_init_wakeup(&udev->dev, 0);
         }

> Besides, enabling wakeup for root hubs is generally a bad idea.  Suppose
> you closed a laptop's lid and then unplugged a USB device -- with wakeup
> enabled, the unplug would cause the laptop to wake up again without your
> knowledge.
> 
> Alan Stern
when closed laptop's lid and then unplugged a non-hid usb device it 
doesn't cause laptop to wakeup. and if that usb device is hid type and 
cause laptop into wakeup state then system will continue into suspend 
state becuase system ask that need accepted a acpi lid open event.
and for laptop usb wakeup that as general ask bios to enable usb wakeup 
then if need do more things to enable usb wakeup I think this usb wakeup 
function isn't friendly and inconveient, so enable it by default.
after add this patch, if want to use usb wakeup function it only need 
enable bios configure it think it is appropriate.

BRs,
Yinbo.
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device
  2021-12-08 10:03 ` [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device Oliver Neukum
@ 2021-12-10  9:50   ` zhuyinbo
  2021-12-14 14:21     ` Oliver Neukum
  0 siblings, 1 reply; 15+ messages in thread
From: zhuyinbo @ 2021-12-10  9:50 UTC (permalink / raw)
  To: Oliver Neukum, Jiri Kosina, benjamin.tissoires, gregkh,
	Thinh.Nguyen, mathias.nyman, stern, rajatja, chris.chiu,
	linux-usb, linux-input, linux-kernel, zhuyinbo,
	Greg Kroah-Hartman, Thinh Nguyen, Mathias Nyman, Alan Stern,
	Rajat Jain, Chris Chiu, linux-usb, linux-input, linux-kernel



在 2021/12/8 下午6:03, Oliver Neukum 写道:
> 
> On 08.12.21 10:39, Yinbo Zhu wrote:
>> The remote wake-up function is a regular function on usb hid device
>> and I think keeping it enabled by default will make usb application
>> more convenient. This patch is to enable remote wakeup function for
>> usb hid device.
>>
> Hi,
> 
> I am afraid we cannot do this. It will cause regression.
> Consider for example the case of laptops with touchscreens
> that will trigger a wake up when the laptop is closed.
> 
>      Regards
> 
>          Oliver
Hi oliver,


system ask that must it must be accped a acpi lid open event then system 
will always into resume state for laptop, otherwise, eventhough that 
system be wakeuped by other event then system will continue into suspend.

and for laptop usb wakeup that as general ask bios to enable usb wakeup 
then if need do more things to enable usb wakeup I think this usb wakeup 
function isn't friendly and inconveient, so enable it by default.
after add this patch, if want to use usb wakeup function it only need 
enable bios configure it think it is appropriate.

BRs,
Yinbo.
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device
  2021-12-08 11:55 ` Greg Kroah-Hartman
@ 2021-12-10  9:54   ` zhuyinbo
  2021-12-10 10:45     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 15+ messages in thread
From: zhuyinbo @ 2021-12-10  9:54 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Kosina, benjamin.tissoires, gregkh, Thinh.Nguyen,
	mathias.nyman, stern, rajatja, chris.chiu, linux-usb,
	linux-input, linux-kernel, zhuyinbo, Thinh Nguyen, Mathias Nyman,
	Alan Stern, Rajat Jain, Chris Chiu, linux-usb, linux-input,
	linux-kernel



在 2021/12/8 下午7:55, Greg Kroah-Hartman 写道:
> On Wed, Dec 08, 2021 at 05:39:50PM +0800, Yinbo Zhu wrote:
>> The remote wake-up function is a regular function on usb hid device
>> and I think keeping it enabled by default will make usb application
>> more convenient. This patch is to enable remote wakeup function for
>> usb hid device.
> 
> How many devices did you test this on?
> 
> As Oliver said, this will cause problems, there's a reason no operating
> system does this :(
> 
> sorry,
> 
> greg k-h
Hi greg,

About that oliver said that I had expained, and I add this change was 
according that usb device whether support remote wakeup and if it 
support wakeup then to enabled it so I think it should be okay for all 
hid device.

BRs,
Yinbo.
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device
  2021-12-10  9:54   ` zhuyinbo
@ 2021-12-10 10:45     ` Greg Kroah-Hartman
  2021-12-16 11:40       ` zhuyinbo
  0 siblings, 1 reply; 15+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-10 10:45 UTC (permalink / raw)
  To: zhuyinbo
  Cc: Jiri Kosina, benjamin.tissoires, Thinh.Nguyen, mathias.nyman,
	stern, rajatja, chris.chiu, linux-usb, linux-input, linux-kernel

On Fri, Dec 10, 2021 at 05:54:33PM +0800, zhuyinbo wrote:
> 
> 
> 在 2021/12/8 下午7:55, Greg Kroah-Hartman 写道:
> > On Wed, Dec 08, 2021 at 05:39:50PM +0800, Yinbo Zhu wrote:
> > > The remote wake-up function is a regular function on usb hid device
> > > and I think keeping it enabled by default will make usb application
> > > more convenient. This patch is to enable remote wakeup function for
> > > usb hid device.
> > 
> > How many devices did you test this on?
> > 
> > As Oliver said, this will cause problems, there's a reason no operating
> > system does this :(
> > 
> > sorry,
> > 
> > greg k-h
> Hi greg,
> 
> About that oliver said that I had expained, and I add this change was
> according that usb device whether support remote wakeup and if it support
> wakeup then to enabled it so I think it should be okay for all hid device.

Again, what devices did you test this on?

And look at other operating systems, as I said, there is a reason that
no one does this.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/2] usb: core: enable remote wakeup function for usb controller
  2021-12-10  9:27     ` zhuyinbo
@ 2021-12-10 16:35       ` Alan Stern
  0 siblings, 0 replies; 15+ messages in thread
From: Alan Stern @ 2021-12-10 16:35 UTC (permalink / raw)
  To: zhuyinbo
  Cc: Jiri Kosina, benjamin.tissoires, gregkh, Thinh.Nguyen,
	mathias.nyman, rajatja, chris.chiu, linux-usb, linux-input,
	linux-kernel

On Fri, Dec 10, 2021 at 05:27:30PM +0800, zhuyinbo wrote:
> 
> 
> 在 2021/12/9 上午6:04, Alan Stern 写道:
> > On Wed, Dec 08, 2021 at 05:39:51PM +0800, Yinbo Zhu wrote:
> > > The remote wake up function is a regular function on usb device and
> > > I think keeping it enabled by default will make the usb application
> > > more convenient and usb device remote wake up function keep enabled
> > > that ask usb controller remote wake up was enabled at first.
> > > 
> > > This patch only enable wake up on usb root hub device, among which,
> > 
> > You say the patch only affects root hub devices, but this doesn't appear
> > to be true.
> > 
> > > usb3.0 root hub doesn't be set wakeup node property but use command
> > > USB_INTRF_FUNC_SUSPEND to enable remote wake up function.
> > > 
> > > Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> > > ---
> > >   drivers/usb/core/hub.c | 20 ++++++++++++++++++--
> > >   include/linux/usb.h    |  4 +++-
> > >   2 files changed, 21 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > index 86658a8..cb4b956 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -2509,6 +2509,8 @@ static void set_usb_port_removable(struct usb_device *udev)
> > >    */
> > >   int usb_new_device(struct usb_device *udev)
> > >   {
> > > +	struct usb_host_config *config;
> > > +	int ncfg;
> > >   	int err;
> > >   	if (udev->parent) {
> > > @@ -2540,6 +2542,18 @@ int usb_new_device(struct usb_device *udev)
> > >   	udev->dev.devt = MKDEV(USB_DEVICE_MAJOR,
> > >   			(((udev->bus->busnum-1) * 128) + (udev->devnum-1)));
> > > +	for (ncfg = 0; ncfg < udev->descriptor.bNumConfigurations; ncfg++) {
> > > +		config = &udev->config[ncfg];
> > > +		if ((config->desc.bmAttributes & (1 << 5)) == 0)
> > > +			break;
> > > +		if (ncfg + 1 == udev->descriptor.bNumConfigurations) {
> > > +			err = usb_enable_remote_wakeup(udev);
> > > +			if (err)
> > > +				dev_dbg(&udev->dev,
> > > +				      "won't remote wakeup, err %d\n", err);
> > > +		}
> > > +	}
> > 
> > I don't see anything in there which treats root hubs differently from
> > other devices.
> > 
> Hi Alan Stern,
> 
> You can find following code, non-root-hub had removed Wakeup sysfs
> attributes and disabled wakeup and root-hub had added wakeup sysfs attibutes
> before call usb_new_device, so this patch was only enabled
> remote wakeup for root-hub device.
> int usb_new_device(struct usb_device *udev)
> {
>         if (udev->parent) {
>                 /* Initialize non-root-hub device wakeup to disabled;
>                  * device (un)configuration controls wakeup capable
>                  * sysfs power/wakeup controls wakeup enabled/disabled
>                  */
>                 device_init_wakeup(&udev->dev, 0);
>         }

Okay.  But in any case, you're doing this in the wrong place.  Remote 
wakeup capability depends on the configuration, so you must not enable 
in usb_new_device() before the configuration has been chosen.

Furthermore, remote wakeup gets turned on only at the time when the 
device is suspended.  We don't leave it on all the time.

> > Besides, enabling wakeup for root hubs is generally a bad idea.  Suppose
> > you closed a laptop's lid and then unplugged a USB device -- with wakeup
> > enabled, the unplug would cause the laptop to wake up again without your
> > knowledge.
> > 
> > Alan Stern
> when closed laptop's lid and then unplugged a non-hid usb device it doesn't
> cause laptop to wakeup. and if that usb device is hid type and cause laptop
> into wakeup state then system will continue into suspend state becuase
> system ask that need accepted a acpi lid open event.

Not all laptops have ACPI.

> and for laptop usb wakeup that as general ask bios to enable usb wakeup then
> if need do more things to enable usb wakeup I think this usb wakeup function
> isn't friendly and inconveient, so enable it by default.
> after add this patch, if want to use usb wakeup function it only need enable
> bios configure it think it is appropriate.

The decision about whether or not to enable remote wakeup for a USB 
device is a matter of policy.  It has to be decided by the user, not by 
the kernel.

This is why there are userspace tools, like Powertop, that 
automatically enable remote wakeup when devices are detected and that 
allow the user to control which devices get enabled.  Using these tools 
is easy and convenient -- that's why they exist -- so the kernel's 
interface does not need to be friendly.

Alan Stern

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device
  2021-12-10  9:50   ` zhuyinbo
@ 2021-12-14 14:21     ` Oliver Neukum
  2021-12-16 10:59       ` zhuyinbo
  0 siblings, 1 reply; 15+ messages in thread
From: Oliver Neukum @ 2021-12-14 14:21 UTC (permalink / raw)
  To: zhuyinbo, Oliver Neukum, Jiri Kosina, benjamin.tissoires, gregkh,
	Thinh.Nguyen, mathias.nyman, stern, rajatja, chris.chiu,
	linux-usb, linux-input, linux-kernel


On 10.12.21 10:50, zhuyinbo wrote:
Hi,
> system ask that must it must be accped a acpi lid open event then
> system will always into resume state for laptop, otherwise, eventhough
> that system be wakeuped by other event then system will continue into
> suspend.
Lid events are necesarily for the whole system.
>
> and for laptop usb wakeup that as general ask bios to enable usb
> wakeup then if need do more things to enable usb wakeup I think this
> usb wakeup function isn't friendly and inconveient, so enable it by
> default.
> after add this patch, if want to use usb wakeup function it only need
> enable bios configure it think it is appropriate.
>
No. If you wish your laptop to be resumed by USB events, that is one thing.
You can alter the system settings. That must work. But it is a different
issue

from the default.

In general any HID device must have wakeup capability to be usable for
selective suspend. You cannot draw conclusions from that.

    Regards
        Oliver


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device
  2021-12-14 14:21     ` Oliver Neukum
@ 2021-12-16 10:59       ` zhuyinbo
  2021-12-16 12:42         ` Oliver Neukum
  0 siblings, 1 reply; 15+ messages in thread
From: zhuyinbo @ 2021-12-16 10:59 UTC (permalink / raw)
  To: Oliver Neukum, gregkh, Jiri Kosina, benjamin.tissoires, gregkh,
	Thinh.Nguyen, mathias.nyman, stern, gregkh, Thinh.Nguyen,
	mathias.nyman, stern, rajatja, chris.chiu, linux-usb,
	linux-input, linux-kernel
  Cc: rajatja, chris.chiu, linux-usb, linux-input, linux-kernel, zhuyinbo



在 2021/12/14 下午10:21, Oliver Neukum 写道:
> 
> On 10.12.21 10:50, zhuyinbo wrote:
> Hi,
>> system ask that must it must be accped a acpi lid open event then
>> system will always into resume state for laptop, otherwise, eventhough
>> that system be wakeuped by other event then system will continue into
>> suspend.
> Lid events are necesarily for the whole system.
>>
>> and for laptop usb wakeup that as general ask bios to enable usb
>> wakeup then if need do more things to enable usb wakeup I think this
>> usb wakeup function isn't friendly and inconveient, so enable it by
>> default.
>> after add this patch, if want to use usb wakeup function it only need
>> enable bios configure it think it is appropriate.
>>
> No. If you wish your laptop to be resumed by USB events, that is one thing.
> You can alter the system settings. That must work. But it is a different
> issue
Hi Oliver,

if you only talk about wakeup source you can think that usb-wakeup 
source and acpi-lid wakeup source was different things. but if you talk 
about laptop and distinguish lid and other event and you shoud know the 
cannotation why system still continue sleep when lid closed then system 
by other event wakeup. if you need test usb-wakeup for laptop and that 
lid shouldn't be closed.
> 
> from the default.
> 
> In general any HID device must have wakeup capability to be usable for
> selective suspend. You cannot draw conclusions from that.
you still can has wakeup capability, but it should be keep enabled by 
default. because the hid device should be convenient for human, if you 
don't think so and I think HID definition is ridiculous.

In addition, I had said that laptop usb wakeup was disabled in system 
bios by default and if user want enable usb wakeup that was only by 
configure bios and doesn't need enable wakeup node if my patch was applied
> 
>      Regards
>          Oliver
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device
  2021-12-10 10:45     ` Greg Kroah-Hartman
@ 2021-12-16 11:40       ` zhuyinbo
  0 siblings, 0 replies; 15+ messages in thread
From: zhuyinbo @ 2021-12-16 11:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Jiri Kosina, benjamin.tissoires, Thinh.Nguyen, mathias.nyman,
	stern, rajatja, chris.chiu, linux-usb, linux-input, linux-kernel,
	zhuyinbo, Thinh.Nguyen, mathias.nyman, stern, rajatja,
	chris.chiu, linux-usb, linux-input, linux-kernel



在 2021/12/10 下午6:45, Greg Kroah-Hartman 写道:
> On Fri, Dec 10, 2021 at 05:54:33PM +0800, zhuyinbo wrote:
>>
>>
>> 在 2021/12/8 下午7:55, Greg Kroah-Hartman 写道:
>>> On Wed, Dec 08, 2021 at 05:39:50PM +0800, Yinbo Zhu wrote:
>>>> The remote wake-up function is a regular function on usb hid device
>>>> and I think keeping it enabled by default will make usb application
>>>> more convenient. This patch is to enable remote wakeup function for
>>>> usb hid device.
>>>
>>> How many devices did you test this on?
>>>
>>> As Oliver said, this will cause problems, there's a reason no operating
>>> system does this :(
>>>
>>> sorry,
>>>
>>> greg k-h
>> Hi greg,
>>
>> About that oliver said that I had expained, and I add this change was
>> according that usb device whether support remote wakeup and if it support
>> wakeup then to enabled it so I think it should be okay for all hid device.
> 
> Again, what devices did you test this on?
Hi greg k-h

mouse device and keyboard device. in hid code that keyboad was enabled 
on wakeup node by default.
> 
> And look at other operating systems, as I said, there is a reason that
> no one does this.
System should be do that. otherwise that lid open event was no make 
sense.  and you need know the order that after lid open then system will 
be wakeup then system will accept lid open event by input subsystem .

in addition, whatever that usb wakeup was disabled in bios by default 
even though after apply my patch.
> 
> thanks,
> 
> greg k-h
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device
  2021-12-16 10:59       ` zhuyinbo
@ 2021-12-16 12:42         ` Oliver Neukum
  2022-01-04 11:44           ` zhuyinbo
  0 siblings, 1 reply; 15+ messages in thread
From: Oliver Neukum @ 2021-12-16 12:42 UTC (permalink / raw)
  To: zhuyinbo, Oliver Neukum, gregkh, Jiri Kosina, benjamin.tissoires,
	Thinh.Nguyen, mathias.nyman, stern, rajatja, chris.chiu,
	linux-usb, linux-input, linux-kernel


On 16.12.21 11:59, zhuyinbo wrote:
>
>
Hi,


> if you only talk about wakeup source you can think that usb-wakeup
> source and acpi-lid wakeup source was different things. but if you
> talk about laptop and distinguish lid and other event and you shoud
> know the cannotation why system still continue sleep when lid closed
> then system by other event wakeup. if you need test usb-wakeup for
> laptop and that lid shouldn't be closed.
I am sorry, I am not sure what you wish to say here. Could you rephrase it?
>> from the default.
>>
>> In general any HID device must have wakeup capability to be usable for
>> selective suspend. You cannot draw conclusions from that.
> you still can has wakeup capability, but it should be keep enabled by
> default. because the hid device should be convenient for human,

Well, no. We are talking about a kernel default. That needs to be so that it
always works on all systems. Convinience is secondary.


> if you don't think so and I think HID definition is ridiculous.
It does have its weaknesses, in particular with respect to differentiating
between events for wakeups. But we cannot change it.
>
>
> In addition, I had said that laptop usb wakeup was disabled in system
> bios by default and if user want enable usb wakeup that was only by
> configure bios and doesn't need enable wakeup node if my patch was
> applied
If you deviate from the default, you deviate. That is reducing the number of
changes is worth little. The default must be above everything else safe.

    Regards
        Oliver


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device
  2021-12-16 12:42         ` Oliver Neukum
@ 2022-01-04 11:44           ` zhuyinbo
  0 siblings, 0 replies; 15+ messages in thread
From: zhuyinbo @ 2022-01-04 11:44 UTC (permalink / raw)
  To: Oliver Neukum, gregkh, Jiri Kosina, benjamin.tissoires,
	Thinh.Nguyen, mathias.nyman, stern, rajatja, chris.chiu,
	linux-usb, linux-input, linux-kernel, zhuyinbo,
	benjamin.tissoires, Thinh.Nguyen, mathias.nyman, stern, rajatja,
	chris.chiu, linux-usb, linux-input, linux-kernel



在 2021/12/16 下午8:42, Oliver Neukum 写道:
> 
> On 16.12.21 11:59, zhuyinbo wrote:
>>
>>
> Hi,
> 
> 
>> if you only talk about wakeup source you can think that usb-wakeup
>> source and acpi-lid wakeup source was different things. but if you
>> talk about laptop and distinguish lid and other event and you shoud
>> know the cannotation why system still continue sleep when lid closed
>> then system by other event wakeup. if you need test usb-wakeup for
>> laptop and that lid shouldn't be closed.
> I am sorry, I am not sure what you wish to say here. Could you rephrase it?
>>> from the default.
That connotation lid was closed represent human will not use laptop and 
system must keep it was sleep and even though the laptop was 
accidentally awakened.
>>>
>>> In general any HID device must have wakeup capability to be usable for
>>> selective suspend. You cannot draw conclusions from that.
>> you still can has wakeup capability, but it should be keep enabled by
>> default. because the hid device should be convenient for human,
> 
> Well, no. We are talking about a kernel default. That needs to be so that it
> always works on all systems. Convinience is secondary.
if system can doesn't consider lid open event and ignore the connotation 
about lid open event I think that system behavior is inappropriate. you 
don't think my patch was inapproriate that on some system doesn't 
consider lid open event.

In additon, if it doesn't include my patch and non-keyboard hid device 
doesn't make system wakeup by ohci. because ohci driver doesn't export 
wakeup property for usb slave device.
> 
> 
>> if you don't think so and I think HID definition is ridiculous.
> It does have its weaknesses, in particular with respect to differentiating
> between events for wakeups. But we cannot change it.
>>
>>
>> In addition, I had said that laptop usb wakeup was disabled in system
>> bios by default and if user want enable usb wakeup that was only by
>> configure bios and doesn't need enable wakeup node if my patch was
>> applied
> If you deviate from the default, you deviate. That is reducing the number of
> changes is worth little. The default must be above everything else safe.
> 
>      Regards
>          Oliver
bios and kernel was two sets of things and they should has their own 
indepdent configuration.  if bios enable usb wakeup but wakeup is still 
not work well. Do you think it is appropriate?

in additon, The keyboard device is enabled by default, and other hid 
devices should also be enabled. Otherwise, it will be treated differently.

> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-01-04 11:44 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-08  9:39 [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device Yinbo Zhu
2021-12-08  9:39 ` [PATCH v1 2/2] usb: core: enable remote wakeup function for usb controller Yinbo Zhu
2021-12-08 22:04   ` Alan Stern
2021-12-10  9:27     ` zhuyinbo
2021-12-10 16:35       ` Alan Stern
2021-12-08 10:03 ` [PATCH v1 1/2] HID: usbhid: enable remote wakeup function for usbhid device Oliver Neukum
2021-12-10  9:50   ` zhuyinbo
2021-12-14 14:21     ` Oliver Neukum
2021-12-16 10:59       ` zhuyinbo
2021-12-16 12:42         ` Oliver Neukum
2022-01-04 11:44           ` zhuyinbo
2021-12-08 11:55 ` Greg Kroah-Hartman
2021-12-10  9:54   ` zhuyinbo
2021-12-10 10:45     ` Greg Kroah-Hartman
2021-12-16 11:40       ` zhuyinbo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).