| 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
| ||
|
Message-ID: <001a740b-d025-4dcf-8b3b-5d9a28eac037@139.com>
Date: Thu, 16 Jan 2025 00:06:45 +0800
From: Jackie Dong <xy-jackie@....com>
To: Takashi Iwai <tiwai@...e.de>
Cc: 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 1/14/25 23:47, Jackie Dong wrote:
> On 1/14/25 16:28, Takashi Iwai wrote:
>> On Tue, 14 Jan 2025 08:19:17 +0100,
>> Takashi Iwai wrote:
>>>
>>> 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.
> Hi Takashi,
> You're right.
> I commented two lines in [ALC269_FIXUP_LENOVO_XPAD_ACPI] and got the
> result of previous mail. I try to look for which funcion call
> alc_fixup_thinkpad_acpi() after add below patch. And I hope to impeleted
> the function with minimum changes.
> Many thanks,
> -----------------------------------------------------------------------
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -6126,6 +6126,7 @@ static void alc_fixup_no_shutup(struct hda_codec
> *codec,
> struct alc_spec *spec = codec->spec;
> spec->no_shutup_pins = 1;
> }
> + printk("This is from alc_fixup_no_shutup+
> +444444444444444444444444444444+-.\n");//Deg
> }
>
> static void alc_fixup_disable_aamix(struct hda_codec *codec,
> @@ -6930,10 +6931,22 @@ static void alc285_fixup_hp_envy_x360(struct
> hda_codec *codec,
> 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 */
> + alc_fixup_no_shutup(codec, fix, action); /* reduce click noise
> */ //Deg
> + printk("This is from alc_fixup_no_shutup+
> +TTTTTTTTTTTTTTTTTTTTTTTTTTTTTTT+-.\n");//Deg
> 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
> */ //Deg
> + printk("This is from alc_fixup_no_shutup+
> +IIIIIIIIIIIIIIIIIIIIIIIIIIIIII+-.\n");//Deg
> + hda_fixup_ideapad_acpi(codec, fix, action);
> +}
> +
> /* Fixup for Lenovo Legion 15IMHg05 speaker output on headset removal. */
> static void alc287_fixup_legion_15imhg05_speakers(struct hda_codec
> *codec,
> const struct
> hda_fixup *fix,
> @@ -7556,6 +7569,7 @@ enum {
> ALC290_FIXUP_SUBWOOFER,
> ALC290_FIXUP_SUBWOOFER_HSJACK,
> ALC269_FIXUP_THINKPAD_ACPI,
> + ALC269_FIXUP_LENOVO_XPAD_ACPI,
> ALC269_FIXUP_DMIC_THINKPAD_ACPI,
> ALC269VB_FIXUP_INFINIX_ZERO_BOOK_13,
> ALC269VC_FIXUP_INFINIX_Y4_MAX,
> @@ -8327,6 +8341,12 @@ static const struct hda_fixup alc269_fixups[] = {
> .chained = true,
> .chain_id = ALC269_FIXUP_SKU_IGNORE,
> },
> + [ALC269_FIXUP_LENOVO_XPAD_ACPI] = {
> + .type = HDA_FIXUP_FUNC,
> + .v.func = alc_fixup_ideapad_acpi,
> +// .chained = true,
> +// .chain_id = ALC269_FIXUP_THINKPAD_ACPI,
> + },
> [ALC269_FIXUP_DMIC_THINKPAD_ACPI] = {
> .type = HDA_FIXUP_FUNC,
> .v.func = alc_fixup_inv_dmic,
> @@ -11065,7 +11085,7 @@ static const struct hda_quirk
> alc269_fixup_vendor_tbl[] = {
> SND_PCI_QUIRK_VENDOR(0x1025, "Acer Aspire", ALC271_FIXUP_DMIC),
> SND_PCI_QUIRK_VENDOR(0x103c, "HP", ALC269_FIXUP_HP_MUTE_LED),
> SND_PCI_QUIRK_VENDOR(0x104d, "Sony VAIO", ALC269_FIXUP_SONY_VAIO),
> - SND_PCI_QUIRK_VENDOR(0x17aa, "Thinkpad",
> ALC269_FIXUP_THINKPAD_ACPI),
> + SND_PCI_QUIRK_VENDOR(0x17aa, "Lenovo XPAD",
> ALC269_FIXUP_LENOVO_XPAD_ACPI),
> SND_PCI_QUIRK_VENDOR(0x19e5, "Huawei Matebook",
> ALC255_FIXUP_MIC_MUTE_LED),
> {}
> };
> @@ -11130,6 +11150,7 @@ static const struct hda_model_fixup
> alc269_fixup_models[] = {
> {.id = ALC290_FIXUP_MONO_SPEAKERS_HSJACK, .name = "mono-
> speakers"},
> {.id = ALC290_FIXUP_SUBWOOFER_HSJACK, .name = "alc290-subwoofer"},
> {.id = ALC269_FIXUP_THINKPAD_ACPI, .name = "thinkpad"},
> + {.id = ALC269_FIXUP_LENOVO_XPAD_ACPI, .name = "lenovo xpad led"},
> {.id = ALC269_FIXUP_DMIC_THINKPAD_ACPI, .name = "dmic-thinkpad"},
> {.id = ALC255_FIXUP_ACER_MIC_NO_PRESENCE, .name = "alc255-acer"},
> {.id = ALC255_FIXUP_ASUS_MIC_NO_PRESENCE, .name = "alc255-asus"},
Hi Takashi,
I study for the logic of ALC269_FIXUP_THINKPAD_ACPI call and test
many times. From my view, only removed alc_fixup_no_shutup() from
alc_fixup_ideapad_acpi() is the best way which has no impact for other
functions call ALC269_FIXUP_THINKPAD_ACPI.
I have submitted the v3 patch which only removed
alc_fixup_no_shutup() from alc_fixup_ideapad_acpi() to make
sure alc_fixup_no_shutup() only be called once for Thinkpad and
Ideapad.
Pls review it again and give your comments.
Best Regards,
Jackie Dong
Powered by blists - more mailing lists