20-audio-pm.rules: fix filtering Mains, empty state file, hardcoded BAT0 - #245
20-audio-pm.rules: fix filtering Mains, empty state file, hardcoded BAT0#245droserasprout wants to merge 1 commit into
Conversation
|
Changes themselves are fine, but I think it would be better if we move all inlined shell code to a seperate script, because reading all of this is actually painful. You can call it |
| SUBSYSTEM=="power_supply", ENV{POWER_SUPPLY_ONLINE}=="0", TEST=="/sys/module/snd_hda_intel", \ | ||
| RUN+="/usr/bin/bash -c 'echo $$(cat /run/udev/snd-hda-intel-powersave 2>/dev/null || \ | ||
| echo 10) > /sys/module/snd_hda_intel/parameters/power_save'" | ||
| ACTION=="add|change", SUBSYSTEM=="power_supply", ENV{POWER_SUPPLY_TYPE}=="Mains", ENV{POWER_SUPPLY_ONLINE}=="0", TEST=="/sys/module/snd_hda_intel", \ |
There was a problem hiding this comment.
I'm also not sure if ACTION=="add" is the right event here.
There was a problem hiding this comment.
systemd-udev-trigger.service calls udevadm trigger --type=all --action=add to coldplug devices, so I think yes, it should be included to apply PM settings on boot.
9566c1f to
d1defad
Compare
ventureoo
left a comment
ventureoo
left a comment
There was a problem hiding this comment.
Could you please take each action handler in a particular function and use local variables for it?
d1defad to
9362d1e
Compare
| @@ -0,0 +1,67 @@ | |||
| #!/usr/bin/bash | |||
There was a problem hiding this comment.
I think we should put that script rather in /usr/bin.
There was a problem hiding this comment.
I would leave it in /usr/lib for internal use only, since a user who discovered this script and invoked it manually would be fighting with udev.
There was a problem hiding this comment.
Then it should be placed in /usr/lib/udev by convention.
There was a problem hiding this comment.
Pull request overview
Fixes audio power-management handling across AC/battery transitions.
Changes:
- Filters udev events to Mains supplies.
- Moves power-state logic into a helper script.
- Adds state restoration with a default value.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
usr/lib/udev/rules.d/20-audio-pm.rules |
Delegates filtered power events to the helper. |
usr/lib/audio-pm-ctl |
Implements AC detection and power-saving state management. |
Suppressed comments (1)
usr/lib/audio-pm-ctl:42
- An offline event from one Mains device reaches this function even if another Mains supply is still online, so it restores power saving while the machine remains on AC. Recheck the aggregate supply state before applying the battery value.
do_battery() {
local v
v=$(cat "$STATE" 2>/dev/null)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # True if at least one mains supply is currently online. | ||
| is_on_ac() { | ||
| local d type online | ||
| for d in /sys/class/power_supply/*; do | ||
| type=$(cat "$d/type" 2>/dev/null) | ||
| [[ $type == Mains ]] || continue | ||
| online=$(cat "$d/online" 2>/dev/null) | ||
| [[ $online == 1 ]] && return 0 | ||
| done | ||
| return 1 | ||
| } |
|
Sorry, I won't go into another round of tuning this ugly script to perfection, primarily because I have removed it completely from my system after finding out who's messing with sound and opening this PR 😆 I couldn't find reliable data on how audio powersave affects real power consumption, and I haven't observed any difference personally. But audio glitches are very real on my hardware. Have you considered simply dropping this hack? |
The reason why we add these rules was exactly because we wanted to fix some audio crackling issues, enabling the power saving only when working from a battery. Do you want to say that using these rules also leads to audio cracks? |
Hi! I brought you some bugfixes. Skipping the Issue step because these changes do not change behavior or settings, hope it's okay.
Problem
Three defects in the
snd_hda_intelpower-management rules, confirmed on a live CachyOS laptop (ASUS G14 GA401IV, AC0 + BAT0 + a USB-C source PSY):USB-C ports re-enable codec power saving on AC. Rules 2 and 3 matched any
power_supplyuevent withPOWER_SUPPLY_ONLINE=0|1and no type filter. A USB-C port PSY reportsONLINE=0and fires a uevent on every plug/unplug, each writingpower_save=10while on AC mains — reintroducing the audio crackle this file exists to prevent.Empty state file when booting on battery.
touchwas unconditional but the value was only written on AC, so a battery boot left the file empty. Thecat file || echo 10fallback was dead code (caton an empty file succeeds), writing a bare newline that sysfs rejected with EINVAL. The intended default of 10 never applied.BAT0hardcoded. Machines exposing onlyBAT1(some ThinkPads, Framework, Surface) read empty —!= "Discharging"— so they were treated as AC-powered and lost power saving on battery.Fix
ENV{POWER_SUPPLY_TYPE}=="Mains"to bothpower_supplyrules so only the AC adapter toggles power saving.touch; write the saved value only on AC and only when non-zero, and have the restore path test for a non-empty value. Absent/empty state now falls back to 10.BAT0with a glob over/sys/class/power_supply/BAT*/status, so any battery name works and a batteryless desktop is treated as always-on-AC.Testing
udevadm verifypasses.power_savestays0across USB-C plug/unplug on AC, flips to10on unplugging mains, returns to0on reconnect. Battery-boot path no longer logs an EINVAL error.