[PATCH] platform/x86: thinkpad_acpi: Correct dual fan probe

[PATCH] platform/x86: thinkpad_acpi: Correct dual fan probe

[Mark Pearson]

There was an issue with the dual fan probe whereby the probe was
failing as it assuming that second_fan support was not available.

Corrected the logic so the probe works correctly. Cleaned up so
quirks only used if 2nd fan not detected.

Tested on X1 Carbon 10 (2 fans), X1 Carbon 9 (2 fans) and T490 (1 fan)

Signed-off-by: Mark Pearson <markpearson@lenovo.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index f385450af864..5eea6651a312 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -8862,24 +8862,27 @@ static int __init fan_init(struct ibm_init_struct *iibm)
 			fan_status_access_mode = TPACPI_FAN_RD_TPEC;
 			if (quirks & TPACPI_FAN_Q1)
 				fan_quirk1_setup();
-			if (quirks & TPACPI_FAN_2FAN) {
-				tp_features.second_fan = 1;
-				pr_info("secondary fan support enabled\n");
-			}
-			if (quirks & TPACPI_FAN_2CTL) {
-				tp_features.second_fan = 1;
-				tp_features.second_fan_ctl = 1;
-				pr_info("secondary fan control enabled\n");
-			}
 			/* Try and probe the 2nd fan */
+			tp_features.second_fan = 1; /* needed for get_speed to work */
 			res = fan2_get_speed(&speed);
 			if (res >= 0) {
 				/* It responded - so let's assume it's there */
 				tp_features.second_fan = 1;
 				tp_features.second_fan_ctl = 1;
 				pr_info("secondary fan control detected & enabled\n");
+			} else {
+				/* Fan not auto-detected */
+				tp_features.second_fan = 0;
+				if (quirks & TPACPI_FAN_2FAN) {
+					tp_features.second_fan = 1;
+					pr_info("secondary fan support enabled\n");
+				}
+				if (quirks & TPACPI_FAN_2CTL) {
+					tp_features.second_fan = 1;
+					tp_features.second_fan_ctl = 1;
+					pr_info("secondary fan control enabled\n");
+				}
 			}
-
 		} else {
 			pr_err("ThinkPad ACPI EC access misbehaving, fan status and control unavailable\n");
 			return -ENODEV;
-- 
2.35.1

Re: [PATCH] platform/x86: thinkpad_acpi: Correct dual fan probe

[Lyude Paul]

Seems to work great on my machine! The one thing it's missing is a Fixes: tag
for the commit that introduced the dual fan probing code originally. With that
fixed:

Reviewed-by: Lyude Paul <lyude@redhat.com>
Tested-by: Lyude Paul <lyude@redhat.com>

On Mon, 2022-05-02 at 15:12 -0400, Mark Pearson wrote:
> There was an issue with the dual fan probe whereby the probe was
> failing as it assuming that second_fan support was not available.
> 
> Corrected the logic so the probe works correctly. Cleaned up so
> quirks only used if 2nd fan not detected.
> 
> Tested on X1 Carbon 10 (2 fans), X1 Carbon 9 (2 fans) and T490 (1 fan)
> 
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index f385450af864..5eea6651a312 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8862,24 +8862,27 @@ static int __init fan_init(struct ibm_init_struct
> *iibm)
>                         fan_status_access_mode = TPACPI_FAN_RD_TPEC;
>                         if (quirks & TPACPI_FAN_Q1)
>                                 fan_quirk1_setup();
> -                       if (quirks & TPACPI_FAN_2FAN) {
> -                               tp_features.second_fan = 1;
> -                               pr_info("secondary fan support enabled\n");
> -                       }
> -                       if (quirks & TPACPI_FAN_2CTL) {
> -                               tp_features.second_fan = 1;
> -                               tp_features.second_fan_ctl = 1;
> -                               pr_info("secondary fan control enabled\n");
> -                       }
>                         /* Try and probe the 2nd fan */
> +                       tp_features.second_fan = 1; /* needed for get_speed
> to work */
>                         res = fan2_get_speed(&speed);
>                         if (res >= 0) {
>                                 /* It responded - so let's assume it's there
> */
>                                 tp_features.second_fan = 1;
>                                 tp_features.second_fan_ctl = 1;
>                                 pr_info("secondary fan control detected &
> enabled\n");
> +                       } else {
> +                               /* Fan not auto-detected */
> +                               tp_features.second_fan = 0;
> +                               if (quirks & TPACPI_FAN_2FAN) {
> +                                       tp_features.second_fan = 1;
> +                                       pr_info("secondary fan support
> enabled\n");
> +                               }
> +                               if (quirks & TPACPI_FAN_2CTL) {
> +                                       tp_features.second_fan = 1;
> +                                       tp_features.second_fan_ctl = 1;
> +                                       pr_info("secondary fan control
> enabled\n");
> +                               }
>                         }
> -
>                 } else {
>                         pr_err("ThinkPad ACPI EC access misbehaving, fan
> status and control unavailable\n");
>                         return -ENODEV;

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH] platform/x86: thinkpad_acpi: Correct dual fan probe

[Thomas Weißschuh]

Thanks!

On 2022-05-02 15:12-0400, Mark Pearson wrote:
> Date: Mon, 2 May 2022 15:12:00 -0400
> From: Mark Pearson <markpearson@lenovo.com>
> To: markpearson@lenovo.com
> CC: hdegoede@redhat.com, markgross@kernel.org,
>  platform-driver-x86@vger.kernel.org, lyude@redhat.com, thomas@t-8ch.de
> Subject: [PATCH] platform/x86: thinkpad_acpi: Correct dual fan probe
> X-Mailer: git-send-email 2.35.1
> 
> There was an issue with the dual fan probe whereby the probe was
> failing as it assuming that second_fan support was not available.
> 
> Corrected the logic so the probe works correctly. Cleaned up so
> quirks only used if 2nd fan not detected.
> 
> Tested on X1 Carbon 10 (2 fans), X1 Carbon 9 (2 fans) and T490 (1 fan)
> 
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index f385450af864..5eea6651a312 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8862,24 +8862,27 @@ static int __init fan_init(struct ibm_init_struct *iibm)
>  			fan_status_access_mode = TPACPI_FAN_RD_TPEC;
>  			if (quirks & TPACPI_FAN_Q1)
>  				fan_quirk1_setup();
> -			if (quirks & TPACPI_FAN_2FAN) {
> -				tp_features.second_fan = 1;
> -				pr_info("secondary fan support enabled\n");
> -			}
> -			if (quirks & TPACPI_FAN_2CTL) {
> -				tp_features.second_fan = 1;
> -				tp_features.second_fan_ctl = 1;
> -				pr_info("secondary fan control enabled\n");
> -			}
>  			/* Try and probe the 2nd fan */
> +			tp_features.second_fan = 1; /* needed for get_speed to work */
>  			res = fan2_get_speed(&speed);
>  			if (res >= 0) {
>  				/* It responded - so let's assume it's there */
>  				tp_features.second_fan = 1;
>  				tp_features.second_fan_ctl = 1;
>  				pr_info("secondary fan control detected & enabled\n");
> +			} else {
> +				/* Fan not auto-detected */
> +				tp_features.second_fan = 0;
> +				if (quirks & TPACPI_FAN_2FAN) {
> +					tp_features.second_fan = 1;
> +					pr_info("secondary fan support enabled\n");
> +				}
> +				if (quirks & TPACPI_FAN_2CTL) {
> +					tp_features.second_fan = 1;
> +					tp_features.second_fan_ctl = 1;
> +					pr_info("secondary fan control enabled\n");
> +				}
>  			}
> -
>  		} else {
>  			pr_err("ThinkPad ACPI EC access misbehaving, fan status and control unavailable\n");
>  			return -ENODEV;
> -- 
> 2.35.1
> 

Tested-by: Thomas Weißschuh <linux@weissschuh.net>
Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>

FYI this now inverts the logic from "platform/x86: thinkpad_acpi: Don't probe 2nd fan if enabled by quirk" [0]
Instead of skipping the probe when there is a quirk the quirk is skipped if the
probe succeeds first.

Going after the rationale in the patch it might be better to turn this around
here, too:

	"If we already know that the system in question has a quirk telling us that
	the system has a second fan, there's no purpose in probing the second fan -
	especially when probing the second fan may not work properly with systems
	relying on quirks."

[0] https://lore.kernel.org/platform-driver-x86/20220429211418.4546-3-lyude@redhat.com/

Re: [External] Re: [PATCH] platform/x86: thinkpad_acpi: Correct dual fan probe

[Mark Pearson]

On 5/4/22 02:11, Thomas Weißschuh wrote:
> Thanks!
> 
> On 2022-05-02 15:12-0400, Mark Pearson wrote:
>> Date: Mon, 2 May 2022 15:12:00 -0400
>> From: Mark Pearson <markpearson@lenovo.com>
>> To: markpearson@lenovo.com
>> CC: hdegoede@redhat.com, markgross@kernel.org,
>>  platform-driver-x86@vger.kernel.org, lyude@redhat.com, thomas@t-8ch.de
>> Subject: [PATCH] platform/x86: thinkpad_acpi: Correct dual fan probe
>> X-Mailer: git-send-email 2.35.1
>>
>> There was an issue with the dual fan probe whereby the probe was
>> failing as it assuming that second_fan support was not available.
>>
>> Corrected the logic so the probe works correctly. Cleaned up so
>> quirks only used if 2nd fan not detected.
>>
>> Tested on X1 Carbon 10 (2 fans), X1 Carbon 9 (2 fans) and T490 (1 fan)
>>
>> Signed-off-by: Mark Pearson <markpearson@lenovo.com>
>> ---
>>  drivers/platform/x86/thinkpad_acpi.c | 23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>> index f385450af864..5eea6651a312 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -8862,24 +8862,27 @@ static int __init fan_init(struct ibm_init_struct *iibm)
>>  			fan_status_access_mode = TPACPI_FAN_RD_TPEC;
>>  			if (quirks & TPACPI_FAN_Q1)
>>  				fan_quirk1_setup();
>> -			if (quirks & TPACPI_FAN_2FAN) {
>> -				tp_features.second_fan = 1;
>> -				pr_info("secondary fan support enabled\n");
>> -			}
>> -			if (quirks & TPACPI_FAN_2CTL) {
>> -				tp_features.second_fan = 1;
>> -				tp_features.second_fan_ctl = 1;
>> -				pr_info("secondary fan control enabled\n");
>> -			}
>>  			/* Try and probe the 2nd fan */
>> +			tp_features.second_fan = 1; /* needed for get_speed to work */
>>  			res = fan2_get_speed(&speed);
>>  			if (res >= 0) {
>>  				/* It responded - so let's assume it's there */
>>  				tp_features.second_fan = 1;
>>  				tp_features.second_fan_ctl = 1;
>>  				pr_info("secondary fan control detected & enabled\n");
>> +			} else {
>> +				/* Fan not auto-detected */
>> +				tp_features.second_fan = 0;
>> +				if (quirks & TPACPI_FAN_2FAN) {
>> +					tp_features.second_fan = 1;
>> +					pr_info("secondary fan support enabled\n");
>> +				}
>> +				if (quirks & TPACPI_FAN_2CTL) {
>> +					tp_features.second_fan = 1;
>> +					tp_features.second_fan_ctl = 1;
>> +					pr_info("secondary fan control enabled\n");
>> +				}
>>  			}
>> -
>>  		} else {
>>  			pr_err("ThinkPad ACPI EC access misbehaving, fan status and control unavailable\n");
>>  			return -ENODEV;
>> -- 
>> 2.35.1
>>
> 
> Tested-by: Thomas Weißschuh <linux@weissschuh.net>
> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> FYI this now inverts the logic from "platform/x86: thinkpad_acpi: Don't probe 2nd fan if enabled by quirk" [0]
> Instead of skipping the probe when there is a quirk the quirk is skipped if the
> probe succeeds first.
> 
> Going after the rationale in the patch it might be better to turn this around
> here, too:
> 
> 	"If we already know that the system in question has a quirk telling us that
> 	the system has a second fan, there's no purpose in probing the second fan -
> 	especially when probing the second fan may not work properly with systems
> 	relying on quirks."
> 
> [0] https://lore.kernel.org/platform-driver-x86/20220429211418.4546-3-lyude@redhat.com/>
Thanks Thomas,

Fair enough - I'll look to rewrite it and get a v2 out shortly.

I had deliberately done it this was as the logic was cleaner this way
with setting/clearing the second_fan setting but I'm good with putting
the order back as it was and doing the quirks first.

I'd love to get rid of the quirks completely but looking at the list of
platforms there's some I'm not going to be able to get hold of to test
so it's moot.

Mark

Re: [External] Re: [PATCH] platform/x86: thinkpad_acpi: Correct dual fan probe

[Lyude Paul]

So - no promises, but which laptops in particular did you need access to? I
should have at least:

P50 (I think??? would have to double check this one), P1 2nd gen, X1 Extreme
2nd gen, and I think I may have access to a P51/P52.

As well, I only have a few old thinkpads (there may actually be a bunch in the
boston office though). However, given how nice the older thinkpads are it's
not too unlikely I could poke around my friends who still use ancient
thinkpads and see if any of them have access to these. Problem is though the
older IBM models seem to be the ones missing comments with the model numbers,
so I'd probably need to know what those are. However, given how old these
machines are feel free not to bother with it if identifying the model numbers
looks to be too much work.

On Wed, 2022-05-04 at 21:57 -0400, Mark Pearson wrote:
> I had deliberately done it this was as the logic was cleaner this way
> with setting/clearing the second_fan setting but I'm good with putting
> the order back as it was and doing the quirks first.
> 
> I'd love to get rid of the quirks completely but looking at the list of
> platforms there's some I'm not going to be able to get hold of to test
> so it's moot.
> 
> Mark

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [External] Re: [PATCH] platform/x86: thinkpad_acpi: Correct dual fan probe

[Mark Pearson]

Hi Lyude,

On 5/5/22 13:32, Lyude Paul wrote:
> So - no promises, but which laptops in particular did you need access to? I
> should have at least:
> 
> P50 (I think??? would have to double check this one), P1 2nd gen, X1 Extreme
> 2nd gen, and I think I may have access to a P51/P52.
> 
> As well, I only have a few old thinkpads (there may actually be a bunch in the
> boston office though). However, given how nice the older thinkpads are it's
> not too unlikely I could poke around my friends who still use ancient
> thinkpads and see if any of them have access to these. Problem is though the
> older IBM models seem to be the ones missing comments with the model numbers,
> so I'd probably need to know what those are. However, given how old these
> machines are feel free not to bother with it if identifying the model numbers
> looks to be too much work.
> 

From the list:

	TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1), - no idea what this is
	TPACPI_QEC_IBM('7', '8', TPACPI_FAN_Q1), - ditto
	TPACPI_QEC_IBM('7', '6', TPACPI_FAN_Q1), - ditto
	TPACPI_QEC_IBM('7', '0', TPACPI_FAN_Q1), - ditto
	TPACPI_QEC_LNV('7', 'M', TPACPI_FAN_2FAN), - ditto
	TPACPI_Q_LNV('N', '1', TPACPI_FAN_2FAN), - ditto
	TPACPI_Q_LNV3('N', '1', 'D', TPACPI_FAN_2CTL),	/* P70 */ - I don't have
& not in lab
	TPACPI_Q_LNV3('N', '1', 'E', TPACPI_FAN_2CTL),	/* P50 */ - I don't have
- in lab
	TPACPI_Q_LNV3('N', '1', 'T', TPACPI_FAN_2CTL),	/* P71 */ - I don't have
& not in lab
	TPACPI_Q_LNV3('N', '1', 'U', TPACPI_FAN_2CTL),	/* P51 */ - I don't have
- in lab
	TPACPI_Q_LNV3('N', '2', 'C', TPACPI_FAN_2CTL),	/* P52 / P72 */ - have
P52 but not P72 (is in lab)
	TPACPI_Q_LNV3('N', '2', 'N', TPACPI_FAN_2CTL),	/* P53 / P73 */ - don't
have - in lab
	TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (1st
gen) */ - don't have - in lab
	TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL),	/* P1 / X1 Extreme (2nd
gen) */ - don't have - in lab
	TPACPI_Q_LNV3('N', '3', '0', TPACPI_FAN_2CTL),	/* P15 (1st gen) / P15v
(1st gen) */ - have P15, but not P15v (in lab)
	TPACPI_Q_LNV3('N', '3', '7', TPACPI_FAN_2CTL),  /* T15g (2nd gen) */ -
don't have - in lab
	TPACPI_Q_LNV3('N', '1', 'O', TPACPI_FAN_NOFAN),	/* X1 Tablet (2nd gen)
*/ - don't have

For the ones in the US lab so I can get one of my US colleagues to chew
thru them on a quiet day (whenever that happens...). We may be able to
'borrow' systems from the Windows teams for the P70, P71 and maybe X1
Tablet - but they do get a bit annoyed with us because we keep returning
them with a superior OS installed.

I figure given I can't reasonably fix the early platforms I should
refactor the code anyway - and then fixing the ones that are still there
becomes a low priority exercise for a rainy day. At least the list will
stop growing.

I thought I had too many systems - but going thru the list now I'm not
so sure :)

Mark

Re: [PATCH] platform/x86: thinkpad_acpi: Correct dual fan probe

[Hans de Goede]

Hi,

On 5/2/22 21:12, Mark Pearson wrote:
> There was an issue with the dual fan probe whereby the probe was
> failing as it assuming that second_fan support was not available.
> 
> Corrected the logic so the probe works correctly. Cleaned up so
> quirks only used if 2nd fan not detected.
> 
> Tested on X1 Carbon 10 (2 fans), X1 Carbon 9 (2 fans) and T490 (1 fan)
> 
> Signed-off-by: Mark Pearson <markpearson@lenovo.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
>  drivers/platform/x86/thinkpad_acpi.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index f385450af864..5eea6651a312 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -8862,24 +8862,27 @@ static int __init fan_init(struct ibm_init_struct *iibm)
>  			fan_status_access_mode = TPACPI_FAN_RD_TPEC;
>  			if (quirks & TPACPI_FAN_Q1)
>  				fan_quirk1_setup();
> -			if (quirks & TPACPI_FAN_2FAN) {
> -				tp_features.second_fan = 1;
> -				pr_info("secondary fan support enabled\n");
> -			}
> -			if (quirks & TPACPI_FAN_2CTL) {
> -				tp_features.second_fan = 1;
> -				tp_features.second_fan_ctl = 1;
> -				pr_info("secondary fan control enabled\n");
> -			}
>  			/* Try and probe the 2nd fan */
> +			tp_features.second_fan = 1; /* needed for get_speed to work */
>  			res = fan2_get_speed(&speed);
>  			if (res >= 0) {
>  				/* It responded - so let's assume it's there */
>  				tp_features.second_fan = 1;
>  				tp_features.second_fan_ctl = 1;
>  				pr_info("secondary fan control detected & enabled\n");
> +			} else {
> +				/* Fan not auto-detected */
> +				tp_features.second_fan = 0;
> +				if (quirks & TPACPI_FAN_2FAN) {
> +					tp_features.second_fan = 1;
> +					pr_info("secondary fan support enabled\n");
> +				}
> +				if (quirks & TPACPI_FAN_2CTL) {
> +					tp_features.second_fan = 1;
> +					tp_features.second_fan_ctl = 1;
> +					pr_info("secondary fan control enabled\n");
> +				}
>  			}
> -
>  		} else {
>  			pr_err("ThinkPad ACPI EC access misbehaving, fan status and control unavailable\n");
>  			return -ENODEV;