| Message ID | 20250324012911.68343-1-ImanDevel@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] platform/x86: thinkpad_acpi: disable ACPI fan access for T495* and E560 | expand |
On Sun Mar 23, 2025 at 10:29 PM -03, Seyediman Seyedarab wrote: > From: Eduard Christian Dumitrescu <eduard.c.dumitrescu@gmail.com> > > The bug was introduced in commit 57d0557dfa49 ("platform/x86: > thinkpad_acpi: Add Thinkpad Edge E531 fan support") which adds a new > fan control method via the FANG and FANW ACPI methods. > > T945, T495s, and E560 laptops have the FANG+FANW ACPI methods (therefore > fang_handle and fanw_handle are not NULL) but they do not actually work, > which results in the dreaded "No such device or address" error. Fan access > and control is restored after forcing the legacy non-ACPI fan control > method by setting both fang_handle and fanw_handle to NULL. > > The DSDT table code for the FANG+FANW methods doesn't seem to do anything > special regarding the fan being secondary. > > This patch adds a quirk for T495, T495s, and E560 to make them avoid the > FANG/FANW methods. Reviewed-by: Kurt Borja <kuurtb@gmail.com> > > Cc: stable@vger.kernel.org > Fixes: 57d0557dfa49 ("platform/x86: thinkpad_acpi: Add Thinkpad Edge E531 fan support") > Reported-by: Vlastimil Holer <vlastimil.holer@gmail.com> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219643 > Tested-by: crok <crok.bic@gmail.com> > Tested-by: Alireza Elikahi <scr0lll0ck1s4b0v3h0m3k3y@gmail.com> > Signed-off-by: Eduard Christian Dumitrescu <eduard.c.dumitrescu@gmail.com> > Co-developed-by: Seyediman Seyedarab <ImanDevel@gmail.com> > Signed-off-by: Seyediman Seyedarab <ImanDevel@gmail.com> > --- > Changes in v2: > - Added the From: tag for the original author > - Replaced the Co-authored-by tag with Co-developed-by > - Cc'd stable@vger.kernel.org > - Removed the extra space inside the comment > - Dropped nullification of sfan/gfan_handle, as it's unrelated to > the current fix > > Kindest Regards, > Seyediman > > drivers/platform/x86/thinkpad_acpi.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index d8df1405edfa..27fd67a2f2d1 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -8793,6 +8793,7 @@ static const struct attribute_group fan_driver_attr_group = { > #define TPACPI_FAN_NS 0x0010 /* For EC with non-Standard register addresses */ > #define TPACPI_FAN_DECRPM 0x0020 /* For ECFW's with RPM in register as decimal */ > #define TPACPI_FAN_TPR 0x0040 /* Fan speed is in Ticks Per Revolution */ > +#define TPACPI_FAN_NOACPI 0x0080 /* Don't use ACPI methods even if detected */ > > static const struct tpacpi_quirk fan_quirk_table[] __initconst = { > TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1), > @@ -8823,6 +8824,9 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = { > TPACPI_Q_LNV3('N', '1', 'O', TPACPI_FAN_NOFAN), /* X1 Tablet (2nd gen) */ > TPACPI_Q_LNV3('R', '0', 'Q', TPACPI_FAN_DECRPM),/* L480 */ > TPACPI_Q_LNV('8', 'F', TPACPI_FAN_TPR), /* ThinkPad x120e */ > + TPACPI_Q_LNV3('R', '0', '0', TPACPI_FAN_NOACPI),/* E560 */ > + TPACPI_Q_LNV3('R', '1', '2', TPACPI_FAN_NOACPI),/* T495 */ > + TPACPI_Q_LNV3('R', '1', '3', TPACPI_FAN_NOACPI),/* T495s */ > }; > > static int __init fan_init(struct ibm_init_struct *iibm) > @@ -8874,6 +8878,13 @@ static int __init fan_init(struct ibm_init_struct *iibm) > tp_features.fan_ctrl_status_undef = 1; > } > > + if (quirks & TPACPI_FAN_NOACPI) { > + /* E560, T495, T495s */ > + pr_info("Ignoring buggy ACPI fan access method\n"); > + fang_handle = NULL; > + fanw_handle = NULL; > + } > + > if (gfan_handle) { > /* 570, 600e/x, 770e, 770x */ > fan_status_access_mode = TPACPI_FAN_RD_ACPI_GFAN;
On Sun, 23 Mar 2025, Seyediman Seyedarab wrote: Hi, Thanks for looking into reaching conclusion for this issue. > From: Eduard Christian Dumitrescu <eduard.c.dumitrescu@gmail.com> > > The bug was introduced in commit 57d0557dfa49 ("platform/x86: the commit > thinkpad_acpi: Add Thinkpad Edge E531 fan support") which adds a new > fan control method via the FANG and FANW ACPI methods. > > T945, T495s, and E560 laptops have the FANG+FANW ACPI methods (therefore > fang_handle and fanw_handle are not NULL) but they do not actually work, > which results in the dreaded "No such device or address" error. Please put the first paragraph about the commit 57d0557dfa49 here now that you've state what the problem/bug is first. You can make them to be one joined paragraph. > Fan access > and control is restored after forcing the legacy non-ACPI fan control > method by setting both fang_handle and fanw_handle to NULL. > > The DSDT table code for the FANG+FANW methods doesn't seem to do anything > special regarding the fan being secondary. > > This patch adds a quirk for T495, T495s, and E560 to make them avoid the > FANG/FANW methods. We avoid phrases like "This patch ...", so start with "Add a quirk ..." Also, this should be before "Fan access ..." sentence to make this description more logical (again make them to be part of a same paragraph). > Cc: stable@vger.kernel.org > Fixes: 57d0557dfa49 ("platform/x86: thinkpad_acpi: Add Thinkpad Edge E531 fan support") > Reported-by: Vlastimil Holer <vlastimil.holer@gmail.com> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219643 I'd prefer these tags to be in this order: Reported-by: Fixes: Closes: Cc: stable@vger.kernel.org ss it's a bit more logical. > Tested-by: crok <crok.bic@gmail.com> > Tested-by: Alireza Elikahi <scr0lll0ck1s4b0v3h0m3k3y@gmail.com> Did these two person either give you those Tested-by tags or gave you green light to add theirs Tested-by tags? I don't see the tags given in the bugzilla entry. If they didn't yet, please ask if they're fine with you adding their Tested-by tags. In kernel development, in general, we're more than happy credit people who invested their time in testing changes but that being said, we don't invent Tested-by tags just from the fact that we know somebody has tested a patch. When respinning v3, please remember to add Kurt's Reviewed-by tag too as maintainer's tools only automatically collect them for the most recent version so when sending the next version, you need to add them into the submission. > Signed-off-by: Eduard Christian Dumitrescu <eduard.c.dumitrescu@gmail.com> > Co-developed-by: Seyediman Seyedarab <ImanDevel@gmail.com> > Signed-off-by: Seyediman Seyedarab <ImanDevel@gmail.com> > --- > Changes in v2: > - Added the From: tag for the original author > - Replaced the Co-authored-by tag with Co-developed-by > - Cc'd stable@vger.kernel.org > - Removed the extra space inside the comment > - Dropped nullification of sfan/gfan_handle, as it's unrelated to > the current fix > > Kindest Regards, > Seyediman > > drivers/platform/x86/thinkpad_acpi.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index d8df1405edfa..27fd67a2f2d1 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -8793,6 +8793,7 @@ static const struct attribute_group fan_driver_attr_group = { > #define TPACPI_FAN_NS 0x0010 /* For EC with non-Standard register addresses */ > #define TPACPI_FAN_DECRPM 0x0020 /* For ECFW's with RPM in register as decimal */ > #define TPACPI_FAN_TPR 0x0040 /* Fan speed is in Ticks Per Revolution */ > +#define TPACPI_FAN_NOACPI 0x0080 /* Don't use ACPI methods even if detected */ > > static const struct tpacpi_quirk fan_quirk_table[] __initconst = { > TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1), > @@ -8823,6 +8824,9 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = { > TPACPI_Q_LNV3('N', '1', 'O', TPACPI_FAN_NOFAN), /* X1 Tablet (2nd gen) */ > TPACPI_Q_LNV3('R', '0', 'Q', TPACPI_FAN_DECRPM),/* L480 */ > TPACPI_Q_LNV('8', 'F', TPACPI_FAN_TPR), /* ThinkPad x120e */ > + TPACPI_Q_LNV3('R', '0', '0', TPACPI_FAN_NOACPI),/* E560 */ > + TPACPI_Q_LNV3('R', '1', '2', TPACPI_FAN_NOACPI),/* T495 */ > + TPACPI_Q_LNV3('R', '1', '3', TPACPI_FAN_NOACPI),/* T495s */ > }; > > static int __init fan_init(struct ibm_init_struct *iibm) > @@ -8874,6 +8878,13 @@ static int __init fan_init(struct ibm_init_struct *iibm) > tp_features.fan_ctrl_status_undef = 1; > } > > + if (quirks & TPACPI_FAN_NOACPI) { > + /* E560, T495, T495s */ > + pr_info("Ignoring buggy ACPI fan access method\n"); > + fang_handle = NULL; > + fanw_handle = NULL; > + } > + > if (gfan_handle) { > /* 570, 600e/x, 770e, 770x */ > fan_status_access_mode = TPACPI_FAN_RD_ACPI_GFAN; > The code change itself looks fine.
Hi, Thank you for your response. > > Tested-by: crok <crok.bic@gmail.com> > > Tested-by: Alireza Elikahi <scr0lll0ck1s4b0v3h0m3k3y@gmail.com> > > Did these two person either give you those Tested-by tags or gave > you green light to add theirs Tested-by tags? I don't see the tags given > in the bugzilla entry. If they didn't yet, please ask if they're fine with > you adding their Tested-by tags. Alireza Elikahi gave me the green light to add his Tested-by tag after testing on the E560. I haven't reached out to crok yet, but I'll send PATCH v3 as soon as I hear back from them and get their permission. Kindest Regards, Seyediman
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index d8df1405edfa..27fd67a2f2d1 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -8793,6 +8793,7 @@ static const struct attribute_group fan_driver_attr_group = { #define TPACPI_FAN_NS 0x0010 /* For EC with non-Standard register addresses */ #define TPACPI_FAN_DECRPM 0x0020 /* For ECFW's with RPM in register as decimal */ #define TPACPI_FAN_TPR 0x0040 /* Fan speed is in Ticks Per Revolution */ +#define TPACPI_FAN_NOACPI 0x0080 /* Don't use ACPI methods even if detected */ static const struct tpacpi_quirk fan_quirk_table[] __initconst = { TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1), @@ -8823,6 +8824,9 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = { TPACPI_Q_LNV3('N', '1', 'O', TPACPI_FAN_NOFAN), /* X1 Tablet (2nd gen) */ TPACPI_Q_LNV3('R', '0', 'Q', TPACPI_FAN_DECRPM),/* L480 */ TPACPI_Q_LNV('8', 'F', TPACPI_FAN_TPR), /* ThinkPad x120e */ + TPACPI_Q_LNV3('R', '0', '0', TPACPI_FAN_NOACPI),/* E560 */ + TPACPI_Q_LNV3('R', '1', '2', TPACPI_FAN_NOACPI),/* T495 */ + TPACPI_Q_LNV3('R', '1', '3', TPACPI_FAN_NOACPI),/* T495s */ }; static int __init fan_init(struct ibm_init_struct *iibm) @@ -8874,6 +8878,13 @@ static int __init fan_init(struct ibm_init_struct *iibm) tp_features.fan_ctrl_status_undef = 1; } + if (quirks & TPACPI_FAN_NOACPI) { + /* E560, T495, T495s */ + pr_info("Ignoring buggy ACPI fan access method\n"); + fang_handle = NULL; + fanw_handle = NULL; + } + if (gfan_handle) { /* 570, 600e/x, 770e, 770x */ fan_status_access_mode = TPACPI_FAN_RD_ACPI_GFAN;