linux-kernel - Re: [External] Re: [PATCH v2] ALSA: hda: Support for Ideapad hotkeymute 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: <87h661277u.wl-tiwai@suse.de>
Date: Tue, 14 Jan 2025 08:19:17 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Jackie Dong <xy-jackie@....com>
Cc: Takashi Iwai <tiwai@...e.de>,
	Jackie EG1 Dong <dongeg1@...ovo.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 hotkeymute LEDs

On Tue, 14 Jan 2025 07:54:01 +0100,
Jackie Dong wrote:
> 
> On 1/6/25 20:49, Jackie Dong wrote:
> > 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
> 
> Hi Takashi,
>   For this function, I added three debug message in patch_realtek.c as
> below. I find alc_fixup_no_shutup() only run once, no matter it's in
> alc_fixup_thinkpad_acpi(), or it's in alc_fixup_ideadpad_acpi(). Some
> kernel log for your reference.
>   So, I think the patch is ok for this concern.
>   If you have any other concern for the patch, let me know.
>   Thanks for your comment and guide in past.

That's really weird.  Are you testing your v2 patch, right?
(That is, the ALC269_FIXUP_LENOVO_XPAD_ACPI entry calls
alc_fixup_ideadpad_acpi() and is chained with
ALC269_FIXUP_THINKPAD_ACPI.  If this entry is really used, it *must*
call the alc_fixup_thinkpad_acpi() as well.

Please double-check.


Takashi