Re: [PATCH] ALSA: hda/realtek: Use inverted mute LED quirk for HP ZBook 8 G2a

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ALSA: hda/realtek: inverted quirk masks pre-probe hp_pin bug, drops LED cdev
- [Medium] ALSA: hda/realtek: inverted mute LED fixup forces LED off on init and resume
--

commit 23741cb49591f5a79d39e1a48d8231c4b0817ad6
Author: Chris Chiu <chris.chiu@canonical.com>

ALSA: hda/realtek: Use inverted mute LED quirk for HP ZBook 8 G2a

This commit switches four HP ZBook 8 G2a PCI SSIDs (0x103c:0x8f40,
0x103c:0x8f41, 0x103c:0x8f42, and 0x103c:0x8f62) from
ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED to
ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED_INVERTED to fix reversed speaker mute LED
behavior.

> diff --git a/sound/hda/codecs/realtek/alc269.c b/sound/hda/codecs/realtek/alc269.c
> index 23329d5887a56..7335b930387e9 100644
> --- a/sound/hda/codecs/realtek/alc269.c
> +++ b/sound/hda/codecs/realtek/alc269.c
> @@ -8106,11 +8106,11 @@ static const struct hda_quirk alc269_fixup_tbl[] = {
>  	SND_PCI_QUIRK(0x103c, 0x8f3a, "HP EliteBook 6 G2i", ALC236_FIXUP_HP_GPIO_LED),
>  	SND_PCI_QUIRK(0x103c, 0x8f3c, "HP EliteBook 6 G2a", ALC236_FIXUP_HP_MUTE_LED_MICMUTE_VREF),
>  	SND_PCI_QUIRK(0x103c, 0x8f3d, "HP EliteBook 6 G2a", ALC236_FIXUP_HP_MUTE_LED_MICMUTE_VREF),
> -	SND_PCI_QUIRK(0x103c, 0x8f40, "HP ZBook 8 G2a 14", ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED),
> -	SND_PCI_QUIRK(0x103c, 0x8f41, "HP ZBook 8 G2a 16", ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED),
> -	SND_PCI_QUIRK(0x103c, 0x8f42, "HP ZBook 8 G2a 14W", ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED),
> +	SND_PCI_QUIRK(0x103c, 0x8f40, "HP ZBook 8 G2a 14", ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED_INVERTED),
> +	SND_PCI_QUIRK(0x103c, 0x8f41, "HP ZBook 8 G2a 16", ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED_INVERTED),
> +	SND_PCI_QUIRK(0x103c, 0x8f42, "HP ZBook 8 G2a 14W", ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED_INVERTED),
>  	SND_PCI_QUIRK(0x103c, 0x8f57, "HP Trekker G7JC", ALC287_FIXUP_CS35L41_I2C_2),
> -	SND_PCI_QUIRK(0x103c, 0x8f62, "HP ZBook 8 G2a 16W", ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED),
> +	SND_PCI_QUIRK(0x103c, 0x8f62, "HP ZBook 8 G2a 16W", ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED_INVERTED),

[Severity: Medium]
Does switching these HP ZBook 8 G2a laptops to
ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED_INVERTED drop registration of the
/sys/class/leds/hda::mute LED classdev while working around a pre-probe bug in
alc245_tas2781_i2c_hp_fixup_muteled()?

In commit 9afbbf490322 ("ALSA: hda/realtek - Enable Mute LED and Tas2781 for
HP platform"), alc245_tas2781_i2c_hp_fixup_muteled() unconditionally called
alc245_fixup_hp_mute_led_coefbit():

sound/hda/codecs/realtek/alc269.c:alc245_fixup_hp_mute_led_coefbit() {
	if (action == HDA_FIXUP_ACT_PRE_PROBE) {
		spec->mute_led_polarity = 0;
		spec->mute_led_coef.idx = 0x0b;
		spec->mute_led_coef.mask = 3 << 2;
		spec->mute_led_coef.on = 2 << 2;
		spec->mute_led_coef.off = 1 << 2;
		snd_hda_gen_add_mute_led_cdev(codec, coef_mute_led_set);
	}
}

With snd-ctl-led, muting sets brightness to 1 (writing 0x08, LED on) and
unmuting sets brightness to 0 (writing 0x04, LED off), which already matches
the HP ZBook 8 G2a COEF polarity.

Later, commit d3be95efc6a1e0 ("ALSA: hda/realtek - Fixed Speaker Mute LED for
HP EliteBoard G1a platform") modified alc245_tas2781_i2c_hp_fixup_muteled() to
branch on alc_get_hp_pin(spec):

sound/hda/codecs/realtek/alc269.c:alc245_tas2781_i2c_hp_fixup_muteled() {
	struct alc_spec *spec = codec->spec;
	hda_nid_t hp_pin = alc_get_hp_pin(spec);
	static const hda_nid_t conn[] = { 0x02 };

	switch (action) {
	case HDA_FIXUP_ACT_PRE_PROBE:
		if (!hp_pin) {
			spec->gen.vmaster_mute.hook = alc245_hp_spk_mute_led_update;
			spec->gen.vmaster_mute_led = 1;
		}
	...
	if (hp_pin)
		alc245_fixup_hp_mute_led_coefbit(codec, fix, action);
}

However, alc269_probe() runs HDA_FIXUP_ACT_PRE_PROBE before
alc269_parse_auto_config() populates spec->gen.autocfg:

sound/hda/codecs/realtek/alc269.c:alc269_probe() {
	...
	snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_PRE_PROBE);
	...
	/* automatic parse from the BIOS config */
	err = alc269_parse_auto_config(codec);
}

Because spec->gen.autocfg is still zeroed during HDA_FIXUP_ACT_PRE_PROBE,
hp_pin is always 0 at pre-probe even on laptops with a headphone jack, and
since alc245_fixup_hp_mute_led_coefbit() only acts on HDA_FIXUP_ACT_PRE_PROBE,
it is never executed. Every board on ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED
falls into the !hp_pin branch and installs alc245_hp_spk_mute_led_update() as
spec->gen.vmaster_mute.hook:

sound/hda/codecs/realtek/alc269.c:alc245_hp_spk_mute_led_update() {
	struct hda_codec *codec = private_data;
	unsigned int val;

	val = enabled ? 0x08 : 0x04; /* 0x08 led on, 0x04 led off */
	alc_update_coef_idx(codec, 0x0b, 0x0c, val);
}

Since vmaster_mute.hook receives the Master Playback Switch state (enabled = 1
when unmuted, 0 when muted), alc245_hp_spk_mute_led_update() writes 0x08 when
unmuted and 0x04 when muted.

Reassigning these laptops to ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED_INVERTED
bypasses snd_hda_gen_add_mute_led_cdev() so no hda::mute LED classdev is
created, and leaves alc245_tas2781_i2c_hp_fixup_muteled() broken for the
remaining boards (0x103c:0x8e8a and 0x103c:0x8f7e). Should
alc245_tas2781_i2c_hp_fixup_muteled() and its Fixes tag (d3be95efc6a1e0) be
addressed instead?

[Severity: Medium]
Will assigning these SSIDs to ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED_INVERTED
also cause the speaker mute LED to be forced off after runtime PM resume or
system suspend/resume while Master Playback Switch is muted?

In alc245_tas2781_i2c_hp_fixup_muteled_inverted(), HDA_FIXUP_ACT_INIT
unconditionally updates the LED using !spec->gen.master_mute:

sound/hda/codecs/realtek/alc269.c:alc245_tas2781_i2c_hp_fixup_muteled_inverted() {
	...
	case HDA_FIXUP_ACT_INIT:
		alc245_hp_spk_mute_led_update_inverted(codec, !spec->gen.master_mute);
		break;
}

In the Realtek driver, spec->gen.master_mute is not updated by the Master
Playback Switch control and remains 0, so !spec->gen.master_mute is always 1.
During codec init and resume, alc_init() runs HDA_FIXUP_ACT_INIT after
snd_hda_gen_init() has synced spec->gen.vmaster_mute:

sound/hda/codecs/realtek/realtek.c:alc_init() {
	...
	snd_hda_gen_init(codec);
	alc_fix_pll(codec);
	alc_auto_init_amp(codec, spec->init_amp);
	snd_hda_apply_verbs(codec); /* apply verbs here after own init */

	snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_INIT);
}

When Master Playback Switch is muted, snd_hda_gen_init() syncs
spec->gen.vmaster_mute (setting COEF 0x0b bits 0x0c to 0x08, LED on), and then
snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_INIT) calls
alc245_hp_spk_mute_led_update_inverted(codec, 1), which writes 0x04 and turns
the mute LED off on every runtime or system resume. Previously, under
ALC245_FIXUP_HP_TAS2781_I2C_MUTE_LED, the HDA_FIXUP_ACT_INIT call was guarded
by if (!hp_pin), which evaluated to false at init time once
alc269_parse_auto_config() had populated spec->gen.autocfg.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105025647.1116662-1-chris.chiu@canonical.com?part=1