| 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: <bfec0774-6c74-46ad-a802-b9361638df17@139.com>
Date: Tue, 14 Jan 2025 23:47:28 +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 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"},
--
2.43.0
>
> On the second thought, alc_fixup_no_shutup() itself is mostly harmless
> even if it's called multiple times, as it sets only the flag.
> But, unifying the quirk function makes more sense as it results in
> smaller changes.
>
> In anyway, the check of the alc_fixup_no_shutup() should be done
> again; if a test is negative, it doesn't mean it's OK but it's
> something wrong.
>
>
> thanks,
>
> Takashi
Powered by blists - more mailing lists