Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rules: remove all power management from udev #353

Merged
merged 1 commit into from
Jun 26, 2015
Merged

rules: remove all power management from udev #353

merged 1 commit into from
Jun 26, 2015

Conversation

kaysievers
Copy link
Contributor

It is not udev's task to apply any of these setting that way, or
from udev rules files. Things need to be sortet out in the kernel,
or explicit whitelist can possibly be added to the hardware database.
Until that is sorted out, and general agreement, udev is not
willing to maintain any such lists or power management settings
in general.

"Thanks for digging this out! I thought my Kinesis keyboard got broken
and ordered a new one, only to find out that the new one doesn't work
as well. I'm not sure whether we should start collecting a blacklist
of keyboards which don't work with USB autosuspend, or rather a
whitelist? Or revert this wholesale?"

#340

It is not udev's task to apply any of these setting that way, or
from udev rules files. Things need to be sortet out in the kernel,
or explicit whitelist can possibly be added to the hardware database.
Until that is sorted out, and general agreement, udev is not
willing to maintain any such lists or power management settings
in general.

"Thanks for digging this out! I thought my Kinesis keyboard got broken
and ordered a new one, only to find out that the new one doesn't work
as well. I'm not sure whether we should start collecting a blacklist
of keyboards which don't work with USB autosuspend, or rather a
whitelist? Or revert this wholesale?"

  #340
@martinpitt
Copy link
Contributor

The rules before commit 64713f9 were working fine, and obviously helped for at least some hardware. So removing this entirely seems to be unnecesarily aggressive? IMHO udev rules/hwdb is a much better place to maintain/test these kinds of lists than the kernel.

@kaysievers
Copy link
Contributor Author

They were "helping" with what exactly? I suspect the ones who were fixed just did not do anything before, and as soon as they have been "fixed", they broke things. We already removed other rules from that file, because they broke some hardware.

@martinpitt
Copy link
Contributor

OK, fair enough. I assumed that Matthew Garrett had tested the rules on some hardware before submitting this..

@teg
Copy link
Contributor

teg commented Jun 26, 2015

I agree with @kaysievers that this definitely looks like something the kernel drivers can and should be handling themselves. So it should not be udev's role to deal with at all, unless we have evidence to the contrary of course (i.e., if there is some compelling reason this cannot be done in the kernel).

Moreover, the impact of this change seems much more limited than the commit message implies ;-) What is removed is a very short white-list of USB HID devices, together with the rule that "all internal USB HID devices support autosuspend". The latter obviously applies to a lot of devices, but should be trivial to do in the kernel instead, right?

I'll ping mjg as he seems to be the main person pushing for this.

@gregkh
Copy link
Contributor

gregkh commented Jun 26, 2015

We (the kernel USB developers) have always said that the whitelist for turning this on should be in userspace, as it's easier to control and update, and it is going to get huge (see the current whitelist in Windows for such an example.) So I recommend doing that here as well, but that's not what this patch does, this patch breaks working systems and should be changed. Just because a device says it can autosuspend, doesn't mean that it actually can, otherwise we wouldn't need a whitelist :)

@gregkh
Copy link
Contributor

gregkh commented Jun 26, 2015

Sorry, this patch fixes the issue, got confused as to which request I was responding to.

This looks good to me, I'm all for merging it.

@gregkh
Copy link
Contributor

gregkh commented Jun 26, 2015

One final note, doing autosuspend for internal devices should be ok, but the kernel can do that directly itself, no need for a udev script for that. If someone cares, they can send a kernel patch for it (hint...)

gregkh added a commit that referenced this pull request Jun 26, 2015
rules: remove all power management from udev
@gregkh gregkh merged commit 57a2bf2 into systemd:master Jun 26, 2015
@teg
Copy link
Contributor

teg commented Jun 26, 2015

Kernel patch sent, but no hardware for testing, so any feedback appreciated: https://lkml.org/lkml/2015/6/26/508.

@devZer0
Copy link

devZer0 commented Jul 15, 2020

The rules before commit 64713f9 were working fine, and obviously helped for at least some hardware.
So removing this entirely seems to be unnecesarily aggressive? IMHO udev rules/hwdb is a much
better place to maintain/test these kinds of lists than the kernel.

Moreover, the impact of this change seems much more limited than the commit message implies ;-)
What is removed is a very short white-list of USB HID devices, together with the rule that "all internal
USB HID devices support autosuspend". The latter obviously applies to a lot of devices, but should
be trivial to do in the kernel instead, right?

i can understand the decision to remove this, but please mind, that "42-usb-hid-pm.rules" also enabled auto-supend for "QEMU USB Tablet" device.

if that is missing, it's causing unecessary CPU consumption in KVM guests (i guess we have millions of those in this world)

apparently, in 2020 every major distro (tested 3 of those) now is burning some extra CPU by default, when installed in KVM (with default of i440fx virtual chipset).

so, you are right if you tell "it does not belong in udev/systemd", but removing something which you cared for, which worked before and others could count/rely on - should always be handled with care. please look twice before removal.

the following statement is untrue in 2020 and untrue for 5 years now. it WAS true in 2014:

https://www.kraxel.org/blog/2014/03/qemu-and-usb-tablet-cpu-consumtion/
"For linux guests the problem is long solved. udev got some rules (see 42-usb-hid-pm.rules) to enable remote wakeup for the usb hid devices emulated by qemu a few years ago, so on any recent linux distro the usb tablet will be suspended when idle."

came across that via:
https://lists.proxmox.com/pipermail/pve-user/2020-July/171868.html

i guess nobody did find this issue before, because recent cpu's are multicore , quite fast and it's unlikely that somebody takes a look if his VM idles at 1% or 3% cpu...

for me, that power-setting makes a difference of 20% cpu load on my older mini-pc with proxmox (which is the reason why i dig into the problem), so please be careful when removing something which had been added for a reason....

@keszybz
Copy link
Member

keszybz commented Jul 15, 2020

See #16476.

keszybz added a commit to keszybz/systemd that referenced this pull request Jul 15, 2020
Partially reverts "rules: remove all power management from udev" /
e2452ee.

The rules for emulated QEMU hardware are restored. It seems that they
were removed in one fell swoop with other rules which were causing problems,
despite the qemu rules working properly (and being adjusted through patches
over time).

systemd#353 (comment)
keszybz added a commit to keszybz/systemd that referenced this pull request Jul 16, 2020
This effectively partially reverts "rules: remove all power management from
udev" / e2452ee. The rules for emulated QEMU
hardware were removed in one fell swoop with other rules which were causing
problems. But the qemu rules were working properly (and were adjusted through
patches over time). Nowadays we have a hwdb for this, so add hwdb entries using
the new detailed modalias.

systemd#353 (comment)
keszybz added a commit to keszybz/systemd that referenced this pull request Jul 16, 2020
This effectively partially reverts "rules: remove all power management from
udev" / e2452ee. The rules for emulated QEMU
hardware were removed in one fell swoop with other rules which were causing
problems. But the qemu rules were working properly (and were adjusted through
patches over time). Nowadays we have a hwdb for this, so add hwdb entries using
the new detailed modalias.

systemd#353 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

7 participants