| Message ID | Pine.LNX.4.44L0.1504221028370.1630-100000@iolanthe.rowland.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Apr 22, 2015 at 11:28:29AM -0400, Alan Stern wrote: > On Wed, 22 Apr 2015, Tom Yan wrote: > > > On 21 April 2015 at 23:51, Alan Stern <stern@rowland.harvard.edu> wrote: > > > Anyway, you're suggesting that drivers should never override sysfs > > > attribute values. But there doesn't seem to be any other way to > > > implement the kernel's policy that wakeup should be enabled by default > > > for all keyboard devices. > > > > I just doubt if there should be any of these kinds of kernel's policy, > > The kernel _has_ to have some policy about default values. It's > unavoidable -- even if you say that the default value for everything > will be 0, that's still a policy! > > > especially for non-critical attributes like wakeup. Obviously now udev > > is put to a very embarassed position (supposedly it should be the one > > managing these policy, but now the fact is the kernel took its job > > from it). Also, from the case of my two receivers, we can see that it > > also causes unnecessary inconsistency to user experience. > > The inconsistency is a bug. Not all keyboard drivers have implemented > the wakeup policy. That can be fixed. Try applying the patch below > and let me know what happens. > > Overriding udev is unfortunate and we should try to avoid it. But at > the moment, I can't see any way to avoid it without making a lot of > users unhappy (because their keyboards will no longer automatically be > enabled for wakeup). > > > To me it's more of "driver's policy" than the kernel's. > > What's the difference? Drivers are part of the kernel, after all. > > > If it's not > > trying to make people with same hardware capibilities share the same > > experience on the same attributes, what's the meaning of these > > policies then? Yes of course there might be a (great) chance that it > > might make (many) people with certain hardware feel happier, but > > objectively does it mean anything? Not to mention that not everyone > > likes the policy. (Can anyone even confirm that the majority likes > > wakeup to be enabled for keyboards by default?) > > If the kernel developers never did anything until they had first > checked to see that a majority of users were in favor, they would never > do anything at all. > > Do you think Microsoft checked with all their users before introducing > Windows Vista or Windows 8? Obviously Linux is not like Windows, for > many reasons, but in some respects their developments are similar. > > > IMHO it would be best that any general policies considered important > > to be off-loaded to udev (as a udev rule?). Only when there's no > > better way (like "communicate directly" with udev?) for the driver to > > set necessary specific policies for itself, it goes back to this > > not-so-good method. > > If we were to change the policy now, it would be a regression because > it would make lots of keyboards stop being wakeup-enabled by default. > Kernel developers are not allowed to cause regressions except in a few > rare cases (such as those involving security problems). > > > > After all, only the driver knows whether or not the device it > > > manages is a keyboard. > > > > I am not sure that I understand what does this mean practically to this issue. > > It means that the wakeup setting cannot be initialized correctly when > the sysfs attribute is created, because the attribute is created before > the kernel knows that the device is a keyboard (since only the driver > knows this, and the driver doesn't get bound to the device until after > the attribute is created). > > Alan Stern > > > > Index: usb-4.0/drivers/hid/hid-logitech-dj.c > =================================================================== > --- usb-4.0.orig/drivers/hid/hid-logitech-dj.c > +++ usb-4.0/drivers/hid/hid-logitech-dj.c > @@ -990,6 +990,7 @@ static int logi_dj_probe(struct hid_devi > const struct hid_device_id *id) > { > struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > + struct usb_device *udev = interface_to_usbdev(intf); > struct dj_receiver_dev *djrcv_dev; > int retval; > > @@ -1078,6 +1079,9 @@ static int logi_dj_probe(struct hid_devi > goto logi_dj_recv_query_paired_devices_failed; > } > > + /* Keyboards are enabled for wakeup by default */ > + device_set_wakeup_enable(&udev->dev, 1); But this device isn't always a keyboard. For example, mine works with a mouse. It's a "universal receiver", you can't know what type of HID device is plugged into it until it connects to it. I don't mind making it auto wakeup, if that works, but the comment isn't correct. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 22 Apr 2015, Greg KH wrote: > > Index: usb-4.0/drivers/hid/hid-logitech-dj.c > > =================================================================== > > --- usb-4.0.orig/drivers/hid/hid-logitech-dj.c > > +++ usb-4.0/drivers/hid/hid-logitech-dj.c > > @@ -990,6 +990,7 @@ static int logi_dj_probe(struct hid_devi > > const struct hid_device_id *id) > > { > > struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > > + struct usb_device *udev = interface_to_usbdev(intf); > > struct dj_receiver_dev *djrcv_dev; > > int retval; > > > > @@ -1078,6 +1079,9 @@ static int logi_dj_probe(struct hid_devi > > goto logi_dj_recv_query_paired_devices_failed; > > } > > > > + /* Keyboards are enabled for wakeup by default */ > > + device_set_wakeup_enable(&udev->dev, 1); > > But this device isn't always a keyboard. For example, mine works with a > mouse. It's a "universal receiver", you can't know what type of HID > device is plugged into it until it connects to it. I don't mind making > it auto wakeup, if that works, but the comment isn't correct. Oh, okay, I didn't realize that. Is there a reasonable way to enable wakeup only when the driver learns that a keyboard is connected? Where would the driver do this? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 22, 2015 at 12:15:36PM -0400, Alan Stern wrote: > On Wed, 22 Apr 2015, Greg KH wrote: > > > > Index: usb-4.0/drivers/hid/hid-logitech-dj.c > > > =================================================================== > > > --- usb-4.0.orig/drivers/hid/hid-logitech-dj.c > > > +++ usb-4.0/drivers/hid/hid-logitech-dj.c > > > @@ -990,6 +990,7 @@ static int logi_dj_probe(struct hid_devi > > > const struct hid_device_id *id) > > > { > > > struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > > > + struct usb_device *udev = interface_to_usbdev(intf); > > > struct dj_receiver_dev *djrcv_dev; > > > int retval; > > > > > > @@ -1078,6 +1079,9 @@ static int logi_dj_probe(struct hid_devi > > > goto logi_dj_recv_query_paired_devices_failed; > > > } > > > > > > + /* Keyboards are enabled for wakeup by default */ > > > + device_set_wakeup_enable(&udev->dev, 1); > > > > But this device isn't always a keyboard. For example, mine works with a > > mouse. It's a "universal receiver", you can't know what type of HID > > device is plugged into it until it connects to it. I don't mind making > > it auto wakeup, if that works, but the comment isn't correct. > > Oh, okay, I didn't realize that. > > Is there a reasonable way to enable wakeup only when the driver learns > that a keyboard is connected? Where would the driver do this? I don't know if the driver ever "knows" this, as you can pair lots of different devices to this same receiver. There's a userspace application that lets you manage the device, called "solaar", that this option could be changed in. But really, putting the device to sleep should work for it no matter if this is a keyboard or a mouse or a joystick, as the wakeup logic is in the receiver, not in the device on the other end of the wireless link. Turning autosuspend works for me for my mouse connected. It doesn't work for one of my "real" USB keyboards when it's connected to the machine, which is why I can't enable autosuspend for it, as it drives me crazy. I don't have a keyboard to test the receiver with at the moment, to see if autosuspend works for both things connected at the same time, or for just the keyboard. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 22 Apr 2015, Greg KH wrote: > > > But this device isn't always a keyboard. For example, mine works with a > > > mouse. It's a "universal receiver", you can't know what type of HID > > > device is plugged into it until it connects to it. I don't mind making > > > it auto wakeup, if that works, but the comment isn't correct. > > > > Oh, okay, I didn't realize that. > > > > Is there a reasonable way to enable wakeup only when the driver learns > > that a keyboard is connected? Where would the driver do this? > > I don't know if the driver ever "knows" this, as you can pair lots of > different devices to this same receiver. There's a userspace > application that lets you manage the device, called "solaar", that this > option could be changed in. > > But really, putting the device to sleep should work for it no matter if > this is a keyboard or a mouse or a joystick, as the wakeup logic is in > the receiver, not in the device on the other end of the wireless link. > > Turning autosuspend works for me for my mouse connected. It doesn't > work for one of my "real" USB keyboards when it's connected to the > machine, which is why I can't enable autosuspend for it, as it drives me > crazy. > > I don't have a keyboard to test the receiver with at the moment, to see > if autosuspend works for both things connected at the same time, or for > just the keyboard. Tom and I have been talking about enabling wakeup, not autosuspend. The question is whether or not the default wakeup setting for the receiver should be "enabled". The kernel's policy is that keyboards should be enabled for wakeup, by default. I think that matches most people's expectations. But when you've got a "universal" receiver, what then? Should it always be enabled by default because a keyboard _might_ be connected? Should it be enabled only when a keyboard is detected? What if multiple devices are connected at the same time? Shucks -- does the receiver even _work_ as a wakeup device? It claims to, but that would require it to remain in wireless contact with the remote keyboard even while it's supposed to be in a low-power state, which rather seems to defeat the purpose. Alan Stern PS: I've got wakeup enabled for the PS/2 keyboard attached to the computer I'm using now, but it doesn't work. I have to press the power button to wake the machine up from suspend. Probably an issue in the BIOS or ACPI -- I haven't bothered to try and track it down. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 22, 2015 at 02:22:44PM -0400, Alan Stern wrote: > On Wed, 22 Apr 2015, Greg KH wrote: > > > > > But this device isn't always a keyboard. For example, mine works with a > > > > mouse. It's a "universal receiver", you can't know what type of HID > > > > device is plugged into it until it connects to it. I don't mind making > > > > it auto wakeup, if that works, but the comment isn't correct. > > > > > > Oh, okay, I didn't realize that. > > > > > > Is there a reasonable way to enable wakeup only when the driver learns > > > that a keyboard is connected? Where would the driver do this? > > > > I don't know if the driver ever "knows" this, as you can pair lots of > > different devices to this same receiver. There's a userspace > > application that lets you manage the device, called "solaar", that this > > option could be changed in. > > > > But really, putting the device to sleep should work for it no matter if > > this is a keyboard or a mouse or a joystick, as the wakeup logic is in > > the receiver, not in the device on the other end of the wireless link. > > > > Turning autosuspend works for me for my mouse connected. It doesn't > > work for one of my "real" USB keyboards when it's connected to the > > machine, which is why I can't enable autosuspend for it, as it drives me > > crazy. > > > > I don't have a keyboard to test the receiver with at the moment, to see > > if autosuspend works for both things connected at the same time, or for > > just the keyboard. > > Tom and I have been talking about enabling wakeup, not autosuspend. > The question is whether or not the default wakeup setting for the > receiver should be "enabled". Oh nevermind, sorry about that. > The kernel's policy is that keyboards should be enabled for wakeup, by > default. I think that matches most people's expectations. But when > you've got a "universal" receiver, what then? > > Should it always be enabled by default because a keyboard _might_ be > connected? Should it be enabled only when a keyboard is detected? > What if multiple devices are connected at the same time? > > Shucks -- does the receiver even _work_ as a wakeup device? It claims > to, but that would require it to remain in wireless contact with the > remote keyboard even while it's supposed to be in a low-power state, > which rather seems to defeat the purpose. I just tried it with my mouse connected, it didn't wake it up. Couldn't tell if it stayed connected to the mouse or not. > PS: I've got wakeup enabled for the PS/2 keyboard attached to the > computer I'm using now, but it doesn't work. I have to press the power > button to wake the machine up from suspend. Probably an issue in the > BIOS or ACPI -- I haven't bothered to try and track it down. Same here for my USB keyboard, it doesn't wake the machine up either... greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > > > Oh, okay, I didn't realize that. > > > > > > Is there a reasonable way to enable wakeup only when the driver > > > learns that a keyboard is connected? Where would the driver do this? > > > > I don't know if the driver ever "knows" this, as you can pair lots of > > different devices to this same receiver. There's a userspace > > application that lets you manage the device, called "solaar", that > > this option could be changed in. > > > > But really, putting the device to sleep should work for it no matter > > if this is a keyboard or a mouse or a joystick, as the wakeup logic is > > in the receiver, not in the device on the other end of the wireless link. > > > > Turning autosuspend works for me for my mouse connected. It doesn't > > work for one of my "real" USB keyboards when it's connected to the > > machine, which is why I can't enable autosuspend for it, as it drives > > me crazy. > > > > I don't have a keyboard to test the receiver with at the moment, to > > see if autosuspend works for both things connected at the same time, > > or for just the keyboard. > > Tom and I have been talking about enabling wakeup, not autosuspend. > The question is whether or not the default wakeup setting for the receiver > should be "enabled". > > The kernel's policy is that keyboards should be enabled for wakeup, by default. > I think that matches most people's expectations. But when you've got a > "universal" receiver, what then? > > Should it always be enabled by default because a keyboard _might_ be > connected? Should it be enabled only when a keyboard is detected? > What if multiple devices are connected at the same time? > From my point, the user option should not depend on kernel default value. If the system you build needs some USB devices as system wakeup source, the developers need to make sure the wakeups are enabled before system enters suspend. Peter > Shucks -- does the receiver even _work_ as a wakeup device? It claims to, but > that would require it to remain in wireless contact with the remote keyboard > even while it's supposed to be in a low-power state, which rather seems to > defeat the purpose. > > Alan Stern > > PS: I've got wakeup enabled for the PS/2 keyboard attached to the computer > I'm using now, but it doesn't work. I have to press the power button to wake > the machine up from suspend. Probably an issue in the BIOS or ACPI -- I > haven't bothered to try and track it down. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body > of a message to majordomo@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I'm not saying that the kernel shouldn't initialize the attributes or have a default. But it should only set the default when the attribute is initialized (It doesn't even matter to me whether it's enabled or disabled). It's just there should not be further manipulation from the kernel (e.g. device_set_wakeup_enable) afterwards because 1. it's brings inconsistency because the function is adopted per driver 2. it's a user preference and responsibilty 3. third it prevent udev to apply a rule properly (regression / bug) P.S. Alan for my case, I don't need a patch for logitech-dj, I just need to remove device_set_enable_wakeup from hid_core.c, then I can enable or disable the attribute with a udev rule happily for both devices. ? Show quoted text On 23 April 2015 at 09:21, Peter Chen <Peter.Chen@freescale.com> wrote: > >> > > >> > > Oh, okay, I didn't realize that. >> > > >> > > Is there a reasonable way to enable wakeup only when the driver >> > > learns that a keyboard is connected? Where would the driver do this? >> > >> > I don't know if the driver ever "knows" this, as you can pair lots of >> > different devices to this same receiver. There's a userspace >> > application that lets you manage the device, called "solaar", that >> > this option could be changed in. >> > >> > But really, putting the device to sleep should work for it no matter >> > if this is a keyboard or a mouse or a joystick, as the wakeup logic is >> > in the receiver, not in the device on the other end of the wireless link. >> > >> > Turning autosuspend works for me for my mouse connected. It doesn't >> > work for one of my "real" USB keyboards when it's connected to the >> > machine, which is why I can't enable autosuspend for it, as it drives >> > me crazy. >> > >> > I don't have a keyboard to test the receiver with at the moment, to >> > see if autosuspend works for both things connected at the same time, >> > or for just the keyboard. >> >> Tom and I have been talking about enabling wakeup, not autosuspend. >> The question is whether or not the default wakeup setting for the receiver >> should be "enabled". >> >> The kernel's policy is that keyboards should be enabled for wakeup, by default. >> I think that matches most people's expectations. But when you've got a >> "universal" receiver, what then? >> >> Should it always be enabled by default because a keyboard _might_ be >> connected? Should it be enabled only when a keyboard is detected? >> What if multiple devices are connected at the same time? >> > > From my point, the user option should not depend on kernel default value. > If the system you build needs some USB devices as system wakeup source, > the developers need to make sure the wakeups are enabled before system > enters suspend. > > Peter > >> Shucks -- does the receiver even _work_ as a wakeup device? It claims to, but >> that would require it to remain in wireless contact with the remote keyboard >> even while it's supposed to be in a low-power state, which rather seems to >> defeat the purpose. >> >> Alan Stern >> >> PS: I've got wakeup enabled for the PS/2 keyboard attached to the computer >> I'm using now, but it doesn't work. I have to press the power button to wake >> the machine up from suspend. Probably an issue in the BIOS or ACPI -- I >> haven't bothered to try and track it down. >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body >> of a message to majordomo@vger.kernel.org More majordomo info at >> http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > I'm not saying that the kernel shouldn't initialize the attributes or have a default. > But it should only set the default when the attribute is initialized (It doesn't > even matter to me whether it's enabled or disabled). > > It's just there should not be further manipulation from the kernel (e.g. > device_set_wakeup_enable) afterwards because 1. it's brings inconsistency > because the function is adopted per driver 2. it's a user preference and > responsibilty 3. third it prevent udev to apply a rule properly (regression / bug) > > P.S. Alan for my case, I don't need a patch for logitech-dj, I just need to remove > device_set_enable_wakeup from hid_core.c, then I can enable or disable the > attribute with a udev rule happily for both devices. > You may apply your choice no matter what the default value is, would you tell us why you can't do that? Peter > ? Show quoted text > On 23 April 2015 at 09:21, Peter Chen <Peter.Chen@freescale.com> wrote: > > > >> > > > >> > > Oh, okay, I didn't realize that. > >> > > > >> > > Is there a reasonable way to enable wakeup only when the driver > >> > > learns that a keyboard is connected? Where would the driver do this? > >> > > >> > I don't know if the driver ever "knows" this, as you can pair lots > >> > of different devices to this same receiver. There's a userspace > >> > application that lets you manage the device, called "solaar", that > >> > this option could be changed in. > >> > > >> > But really, putting the device to sleep should work for it no > >> > matter if this is a keyboard or a mouse or a joystick, as the > >> > wakeup logic is in the receiver, not in the device on the other end of the > wireless link. > >> > > >> > Turning autosuspend works for me for my mouse connected. It > >> > doesn't work for one of my "real" USB keyboards when it's connected > >> > to the machine, which is why I can't enable autosuspend for it, as > >> > it drives me crazy. > >> > > >> > I don't have a keyboard to test the receiver with at the moment, to > >> > see if autosuspend works for both things connected at the same > >> > time, or for just the keyboard. > >> > >> Tom and I have been talking about enabling wakeup, not autosuspend. > >> The question is whether or not the default wakeup setting for the > >> receiver should be "enabled". > >> > >> The kernel's policy is that keyboards should be enabled for wakeup, by > default. > >> I think that matches most people's expectations. But when you've got > >> a "universal" receiver, what then? > >> > >> Should it always be enabled by default because a keyboard _might_ be > >> connected? Should it be enabled only when a keyboard is detected? > >> What if multiple devices are connected at the same time? > >> > > > > From my point, the user option should not depend on kernel default value. > > If the system you build needs some USB devices as system wakeup > > source, the developers need to make sure the wakeups are enabled > > before system enters suspend. > > > > Peter > > > >> Shucks -- does the receiver even _work_ as a wakeup device? It > >> claims to, but that would require it to remain in wireless contact > >> with the remote keyboard even while it's supposed to be in a > >> low-power state, which rather seems to defeat the purpose. > >> > >> Alan Stern > >> > >> PS: I've got wakeup enabled for the PS/2 keyboard attached to the > >> computer I'm using now, but it doesn't work. I have to press the > >> power button to wake the machine up from suspend. Probably an issue > >> in the BIOS or ACPI -- I haven't bothered to try and track it down. > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-usb" > >> in the body of a message to majordomo@vger.kernel.org More majordomo > >> info at http://vger.kernel.org/majordomo-info.html
https://bugs.freedesktop.org/show_bug.cgi?id=90041 Because udev's work would get overrided if there's further manipulation. So if you want your udev rule to work, you have to make sure you run a trigger command after the module is loaded or reloaded. This would happen for all types of devices if their driver make sure of function like device_set_wakeup_enable. P.S. Though there could be exceptions if the module is loaded in early user space. Then the rule will only get overrided if you somehow need to reload the module. On 23 April 2015 at 13:22, Peter Chen <Peter.Chen@freescale.com> wrote: > >> >> I'm not saying that the kernel shouldn't initialize the attributes or have a default. >> But it should only set the default when the attribute is initialized (It doesn't >> even matter to me whether it's enabled or disabled). >> >> It's just there should not be further manipulation from the kernel (e.g. >> device_set_wakeup_enable) afterwards because 1. it's brings inconsistency >> because the function is adopted per driver 2. it's a user preference and >> responsibilty 3. third it prevent udev to apply a rule properly (regression / bug) >> >> P.S. Alan for my case, I don't need a patch for logitech-dj, I just need to remove >> device_set_enable_wakeup from hid_core.c, then I can enable or disable the >> attribute with a udev rule happily for both devices. >> > > You may apply your choice no matter what the default value is, would you tell us > why you can't do that? > > Peter > >> ? Show quoted text >> On 23 April 2015 at 09:21, Peter Chen <Peter.Chen@freescale.com> wrote: >> > >> >> > > >> >> > > Oh, okay, I didn't realize that. >> >> > > >> >> > > Is there a reasonable way to enable wakeup only when the driver >> >> > > learns that a keyboard is connected? Where would the driver do this? >> >> > >> >> > I don't know if the driver ever "knows" this, as you can pair lots >> >> > of different devices to this same receiver. There's a userspace >> >> > application that lets you manage the device, called "solaar", that >> >> > this option could be changed in. >> >> > >> >> > But really, putting the device to sleep should work for it no >> >> > matter if this is a keyboard or a mouse or a joystick, as the >> >> > wakeup logic is in the receiver, not in the device on the other end of the >> wireless link. >> >> > >> >> > Turning autosuspend works for me for my mouse connected. It >> >> > doesn't work for one of my "real" USB keyboards when it's connected >> >> > to the machine, which is why I can't enable autosuspend for it, as >> >> > it drives me crazy. >> >> > >> >> > I don't have a keyboard to test the receiver with at the moment, to >> >> > see if autosuspend works for both things connected at the same >> >> > time, or for just the keyboard. >> >> >> >> Tom and I have been talking about enabling wakeup, not autosuspend. >> >> The question is whether or not the default wakeup setting for the >> >> receiver should be "enabled". >> >> >> >> The kernel's policy is that keyboards should be enabled for wakeup, by >> default. >> >> I think that matches most people's expectations. But when you've got >> >> a "universal" receiver, what then? >> >> >> >> Should it always be enabled by default because a keyboard _might_ be >> >> connected? Should it be enabled only when a keyboard is detected? >> >> What if multiple devices are connected at the same time? >> >> >> > >> > From my point, the user option should not depend on kernel default value. >> > If the system you build needs some USB devices as system wakeup >> > source, the developers need to make sure the wakeups are enabled >> > before system enters suspend. >> > >> > Peter >> > >> >> Shucks -- does the receiver even _work_ as a wakeup device? It >> >> claims to, but that would require it to remain in wireless contact >> >> with the remote keyboard even while it's supposed to be in a >> >> low-power state, which rather seems to defeat the purpose. >> >> >> >> Alan Stern >> >> >> >> PS: I've got wakeup enabled for the PS/2 keyboard attached to the >> >> computer I'm using now, but it doesn't work. I have to press the >> >> power button to wake the machine up from suspend. Probably an issue >> >> in the BIOS or ACPI -- I haven't bothered to try and track it down. >> >> >> >> -- >> >> To unsubscribe from this list: send the line "unsubscribe linux-usb" >> >> in the body of a message to majordomo@vger.kernel.org More majordomo >> >> info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 23 Apr 2015, Tom Yan wrote: > I'm not saying that the kernel shouldn't initialize the attributes or > have a default. But it should only set the default when the attribute > is initialized (It doesn't even matter to me whether it's enabled or > disabled). > > It's just there should not be further manipulation from the kernel > (e.g. device_set_wakeup_enable) afterwards because > 1. it's brings inconsistency because the function is adopted per driver > 2. it's a user preference and responsibilty > 3. third it prevent udev to apply a rule properly (regression / bug) > > P.S. Alan for my case, I don't need a patch for logitech-dj, I just > need to remove device_set_enable_wakeup from hid_core.c, then I can > enable or disable the attribute with a udev rule happily for both > devices. I understand that. But removing device_set_enable_wakeup from hid_core.c would annoy a lot of people. Linus would probably decide that it was a regression and would put the function call back in. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I know. Honestly I never expect it to be fixed. I just want to draw some attention so that people can be aware of this problem (some may be suffering from this without understand what's going on). Though I did filed a bug report on bugzilla some time ago to request making this function a no-op (sounds outrageous doesn't it). Hope that something will be done for this one day anyway. On 23 April 2015 at 22:40, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 23 Apr 2015, Tom Yan wrote: > >> I'm not saying that the kernel shouldn't initialize the attributes or >> have a default. But it should only set the default when the attribute >> is initialized (It doesn't even matter to me whether it's enabled or >> disabled). >> >> It's just there should not be further manipulation from the kernel >> (e.g. device_set_wakeup_enable) afterwards because >> 1. it's brings inconsistency because the function is adopted per driver >> 2. it's a user preference and responsibilty >> 3. third it prevent udev to apply a rule properly (regression / bug) >> >> P.S. Alan for my case, I don't need a patch for logitech-dj, I just >> need to remove device_set_enable_wakeup from hid_core.c, then I can >> enable or disable the attribute with a udev rule happily for both >> devices. > > I understand that. But removing device_set_enable_wakeup from > hid_core.c would annoy a lot of people. Linus would probably decide > that it was a regression and would put the function call back in. > > Alan Stern > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: usb-4.0/drivers/hid/hid-logitech-dj.c =================================================================== --- usb-4.0.orig/drivers/hid/hid-logitech-dj.c +++ usb-4.0/drivers/hid/hid-logitech-dj.c @@ -990,6 +990,7 @@ static int logi_dj_probe(struct hid_devi const struct hid_device_id *id) { struct usb_interface *intf = to_usb_interface(hdev->dev.parent); + struct usb_device *udev = interface_to_usbdev(intf); struct dj_receiver_dev *djrcv_dev; int retval; @@ -1078,6 +1079,9 @@ static int logi_dj_probe(struct hid_devi goto logi_dj_recv_query_paired_devices_failed; } + /* Keyboards are enabled for wakeup by default */ + device_set_wakeup_enable(&udev->dev, 1); + return retval; logi_dj_recv_query_paired_devices_failed: