linux-kernel - Re: [External] Re: [PATCH v2] ALSA: hda: Support for Ideapad hotkey mute LEDs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o70oylir.wl-tiwai@suse.de>
Date: Fri, 03 Jan 2025 16:17:32 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Jackie EG1 Dong <dongeg1@...ovo.com>
Cc: Takashi Iwai <tiwai@...e.de>,
	Jackie Dong <xy-jackie@....com>,
	"perex@...ex.cz" <perex@...ex.cz>,
	"tiwai@...e.com" <tiwai@...e.com>,
	"bo.liu@...arytech.com" <bo.liu@...arytech.com>,
	"kovalev@...linux.org"
	<kovalev@...linux.org>,
	"me@...herl.one" <me@...herl.one>,
	"jaroslaw.janik@...il.com" <jaroslaw.janik@...il.com>,
	"songxiebing@...inos.cn" <songxiebing@...inos.cn>,
	"kailang@...ltek.com"
	<kailang@...ltek.com>,
	"sbinding@...nsource.cirrus.com"
	<sbinding@...nsource.cirrus.com>,
	"simont@...nsource.cirrus.com"
	<simont@...nsource.cirrus.com>,
	"josh@...huagrisham.com"
	<josh@...huagrisham.com>,
	"rf@...nsource.cirrus.com"
	<rf@...nsource.cirrus.com>,
	"linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
	"platform-driver-x86@...r.kernel.org"
	<platform-driver-x86@...r.kernel.org>,
	"linux-sound@...r.kernel.org"
	<linux-sound@...r.kernel.org>,
	"mpearson-lenovo@...ebb.ca"
	<mpearson-lenovo@...ebb.ca>
Subject: Re: [External] Re: [PATCH v2] ALSA: hda: Support for Ideapad hotkey mute LEDs

On Mon, 30 Dec 2024 01:33:01 +0100,
Jackie EG1 Dong wrote:
> 
> > On Tue, 24 Dec 2024 09:33:16 +0100,
>  > Jackie Dong wrote:
>  >>
>  >> --- a/sound/pci/hda/patch_realtek.c
>  >> +++ b/sound/pci/hda/patch_realtek.c
>  >> @@ -6934,6 +6934,16 @@ static void alc_fixup_thinkpad_acpi(struct hda_codec *codec,
>  >>   	hda_fixup_thinkpad_acpi(codec, fix, action);
>  >>   }
>  >>
>  >> +/* for hda_fixup_ideapad_acpi() */
>  >> +#include "ideapad_hotkey_led_helper.c"
>  >> +
>  >> +static void alc_fixup_ideapad_acpi(struct hda_codec *codec,
>  >> +				   const struct hda_fixup *fix, int action)
>  >> +{
>  >> +	alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */
>  >> +	hda_fixup_ideapad_acpi(codec, fix, action);
>  >> +}
>  >
>  > So this unconditionally call alc_fixup_no_shutup(), and this  > introduces another behavior to the existing entry -- i.e. there is a  > chance of breakage.
>  >
>  > If we want to be very conservative, this call should be limited to  > Ideapad.
>  > For alc_fixup_no_shutup(codec, fix, action),
>  I want to keep same behavior with alc_fixup_thinkpad_apci() and alc_fixup_idea_acpi() for one sound card. So, I add alc_fixup_no_shutup() in alc_fixup_ideapad_acpi().
> ----------Related source code of patch_reatek.c are FYR as below.
> static void alc_fixup_thinkpad_acpi(struct hda_codec *codec,
>                                      const struct hda_fixup *fix, int
> action)
> {
>          alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */
>          hda_fixup_thinkpad_acpi(codec, fix, action); }
> 
> /* for hda_fixup_ideapad_acpi() */
> #include "ideapad_hotkey_led_helper.c"
> 
> static void alc_fixup_ideapad_acpi(struct hda_codec *codec,
>                                     const struct hda_fixup *fix, int action) {
>          alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */
>          hda_fixup_ideapad_acpi(codec, fix, action);
> }

Oh yeah, but then it can be bad in other way round; the chain call of
alc_fixup_thinkpad_acpi() contains alc_fixup_no_shutup() and the
alc_fixup_ideadpad_acpi() also contains alc_fixup_no_shutup().
That is, alc_fixup_no_shutup() will be called twice for Thinkpad.

Instead, you can change like:

--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6925,11 +6925,16 @@ static void alc285_fixup_hp_envy_x360(struct hda_codec *codec,
 /* for hda_fixup_thinkpad_acpi() */
 #include "thinkpad_helper.c"
 
-static void alc_fixup_thinkpad_acpi(struct hda_codec *codec,
-				    const struct hda_fixup *fix, int action)
+/* for hda_fixup_ideapad_acpi() */
+#include "ideapad_hotkey_led_helper.c"
+
+/* generic fixup for both Lenovo Thinkpad and Ideapad */
+static void alc_fixup_xpad_acpi(struct hda_codec *codec,
+				const struct hda_fixup *fix, int action)
 {
 	alc_fixup_no_shutup(codec, fix, action); /* reduce click noise */
 	hda_fixup_thinkpad_acpi(codec, fix, action);
+	hda_fixup_ideapad_acpi(codec, fix, action);
 }
 
 /* Fixup for Lenovo Legion 15IMHg05 speaker output on headset removal. */
@@ -8321,7 +8326,7 @@ static const struct hda_fixup alc269_fixups[] = {
 	},
 	[ALC269_FIXUP_THINKPAD_ACPI] = {
 		.type = HDA_FIXUP_FUNC,
-		.v.func = alc_fixup_thinkpad_acpi,
+		.v.func = alc_fixup_xpad_acpi,
 		.chained = true,
 		.chain_id = ALC269_FIXUP_SKU_IGNORE,
 	},

Since hda_fixup_ideapad_acpi() is NOP except for Ideapad, this
shouldn't break other models, while it covers the Ideadpad now.


thanks,

Takashi