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] [day] [month] [year] [list]

Message-ID: <da1959f2-a30e-45b4-bf64-2d413254d7bc@139.com>
Date: Mon, 6 Jan 2025 20:49:20 +0800
From: Jackie Dong <xy-jackie@....com>
To: Takashi Iwai <tiwai@...e.de>, Jackie EG1 Dong <dongeg1@...ovo.com>
Cc: "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 2025/1/3 23:17, Takashi Iwai wrote:
> 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.
> 
Many thanks to Takashi for your detail comments  and sample code, I 
understand it now.

I'll check the logic of the code and update the patch later.

Best Regards,

Jackie Dong
> 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