Re: [PATCH v2] platform/x86: thinkpad_acpi: disable ACPI fan access for T495* and E560

[Ilpo Järvinen]

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@xxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
> Fixes: 57d0557dfa49 ("platform/x86: thinkpad_acpi: Add Thinkpad Edge E531 fan support")
> Reported-by: Vlastimil Holer <vlastimil.holer@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxx

ss it's a bit more logical.

> Tested-by: crok <crok.bic@xxxxxxxxx>
> Tested-by: Alireza Elikahi <scr0lll0ck1s4b0v3h0m3k3y@xxxxxxxxx>

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@xxxxxxxxx>
> Co-developed-by: Seyediman Seyedarab <ImanDevel@xxxxxxxxx>
> Signed-off-by: Seyediman Seyedarab <ImanDevel@xxxxxxxxx>
> ---
> Changes in v2:
> - Added the From: tag for the original author
> - Replaced the Co-authored-by tag with Co-developed-by
> - Cc'd stable@xxxxxxxxxxxxxxx
> - 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.

-- 
 i.