| Message ID | 20241030171813.18941-1-jaroslaw.janik@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix beep notifications by Thinkpad's ACPI firmware | expand |
On Wed, 30 Oct 2024 18:18:11 +0100, Jarosław Janik wrote: > > In Comment 17 of the following bug report: > https://bugzilla.suse.com/show_bug.cgi?id=1228269 > user tigerlily had complained about "audible blip on shutdown" on a > Thinkpad laptop, that led to a commit 4f61c8fe3520 ("ALSA: hda/conexant: > Mute speakers at suspend / shutdown"), which mutes speakers on system > shutdown or whenever HDA controller is suspended by PM. The > aforementioned "blip" is a feature, not a bug however - Thinkpad's ACPI > firmware uses short beeps / beep sequences as notifications in response > to various events (enter/leave suspend or hibernation, AC power > connect/disconnect, low battery, etc.); these can also be triggered by > writing to /proc/acpi/ibm/beep, see details here: > https://www.kernel.org/doc/html/v5.4/admin-guide/laptops/thinkpad-acpi.html#acpi-sounds-proc-acpi-ibm-beep > The firmware doesn't touch any mixer, so depending on current mixer > settings these beeps can be loud, silent or even muted completely, > whatever has been set by the user. > > The patch in question interferes this badly: > - suspend/hibernate/shutdown related events are muted altogether because > HDA controller is in suspend mode when they occur (or snd_hda_intel > module has been closed in the event of system shutdown) > - other events work "randomly", e.g. you can hear AC plug/unplug beep > if something is playing audio at the moment, otherwise the HDA > controller is likely in suspend mode, so you can't hear anything > (unless you disabled PM in snd_hda_intel module). > > That said - let's revert this; this is what 1st commit does, the 2nd > commit is somewhat optional - it removes helpers introduced to implement > this muting, as they are no longer used. As it's a regression, it's OK for the first revert, but the function is useful for other purposes and other devices, so I'd like to keep it. I'm going to apply only the first patch. > PS. Don't forget to have this backported to LTS kernels, please. This should be taken usually automatically as long as Fixes tag points to the right commit. Per reading the patch description, though, the behavior appears to be pretty unreliable, as it depends on the runtime suspend. We may control the runtime PM better if we know that the beep must be submitted; or it's even possible to runtime resume/re-suspend upon beeping, too. But it's maybe too complex than needed. thanks, Takashi
On 31.10.2024 10:15, Takashi Iwai wrote: > Per reading the patch description, though, the behavior appears to be > pretty unreliable, as it depends on the runtime suspend. We may > control the runtime PM better if we know that the beep must be > submitted; or it's even possible to runtime resume/re-suspend upon > beeping, too. But it's maybe too complex than needed. You must have misunderstood that, this unreliable behavior is what happens with your patch applied, once this patch is reverted everything works perfectly fine (again). Perhaps the commit message should be rewritten to indicate it more clearly, e.g. replace: > now those beeps are either muted altogether with > since 4f61c8fe3520 those beeps are either muted altogether ... or perhaps it should be just left as it is... Thanks anyway!
On Thu, 31 Oct 2024 17:12:53 +0100, Jarosław Janik wrote: > > On 31.10.2024 10:15, Takashi Iwai wrote: > > > Per reading the patch description, though, the behavior appears to be > > pretty unreliable, as it depends on the runtime suspend. We may > > control the runtime PM better if we know that the beep must be > > submitted; or it's even possible to runtime resume/re-suspend upon > > beeping, too. But it's maybe too complex than needed. > > You must have misunderstood that, this unreliable behavior is what > happens with your patch applied, once this patch is reverted everything > works perfectly fine (again). Perhaps the commit message should be > rewritten to indicate it more clearly, e.g. replace: > > now those beeps are either muted altogether > with > > since 4f61c8fe3520 those beeps are either muted altogether > > ... or perhaps it should be just left as it is... Hm, so the beep works even if the HD-audio device is runtime-suspended before shutdown? Also, beeping at suspend/resume works no matter whether runtime-suspended or not? thanks, Takashi
On 31.10.2024 17:21, Takashi Iwai wrote: > Hm, so the beep works even if the HD-audio device is runtime-suspended > before shutdown? Also, beeping at suspend/resume works no matter > whether runtime-suspended or not? Well, to what I recall - my (current) Thinkpad has never beeped on shutdown, other than that - without your "extra muting" patch - yes - beeping in every other circumstances works no matter if HDA controller is in PM suspend or not; this is what I've got used to for many years of using Thinkpads. And one more thing - with your patch - beeping is not muted on my laptop completely, you can still hear it if you are in a quiet room, it's just very "weak".
On 31.10.24 10:15, Takashi Iwai wrote: > On Wed, 30 Oct 2024 18:18:11 +0100, > Jarosław Janik wrote: > >> PS. Don't forget to have this backported to LTS kernels, please. > This should be taken usually automatically as long as Fixes tag points > to the right commit. Usually: yes. But a quick reminder: it also might be silently dropped. If you want to ensure it's backported, a stable tag is a must: https://docs.kernel.org/process/stable-kernel-rules.html Ciao, Thorsten
In Comment 17 of the following bug report: https://bugzilla.suse.com/show_bug.cgi?id=1228269 user tigerlily had complained about "audible blip on shutdown" on a Thinkpad laptop, that led to a commit 4f61c8fe3520 ("ALSA: hda/conexant: Mute speakers at suspend / shutdown"), which mutes speakers on system shutdown or whenever HDA controller is suspended by PM. The aforementioned "blip" is a feature, not a bug however - Thinkpad's ACPI firmware uses short beeps / beep sequences as notifications in response to various events (enter/leave suspend or hibernation, AC power connect/disconnect, low battery, etc.); these can also be triggered by writing to /proc/acpi/ibm/beep, see details here: https://www.kernel.org/doc/html/v5.4/admin-guide/laptops/thinkpad-acpi.html#acpi-sounds-proc-acpi-ibm-beep The firmware doesn't touch any mixer, so depending on current mixer settings these beeps can be loud, silent or even muted completely, whatever has been set by the user. The patch in question interferes this badly: - suspend/hibernate/shutdown related events are muted altogether because HDA controller is in suspend mode when they occur (or snd_hda_intel module has been closed in the event of system shutdown) - other events work "randomly", e.g. you can hear AC plug/unplug beep if something is playing audio at the moment, otherwise the HDA controller is likely in suspend mode, so you can't hear anything (unless you disabled PM in snd_hda_intel module). That said - let's revert this; this is what 1st commit does, the 2nd commit is somewhat optional - it removes helpers introduced to implement this muting, as they are no longer used. PS. Don't forget to have this backported to LTS kernels, please. Jarosław Janik (2): Revert "ALSA: hda/conexant: Mute speakers at suspend / shutdown" Revert "ALSA: hda/generic: Add a helper to mute speakers at suspend/shutdown" sound/pci/hda/hda_generic.c | 63 ---------------------------------- sound/pci/hda/hda_generic.h | 1 - sound/pci/hda/patch_conexant.c | 2 -- 3 files changed, 66 deletions(-)