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@xxxxxxxxxx>
>> To: markpearson@xxxxxxxxxx
>> CC: hdegoede@xxxxxxxxxx, markgross@xxxxxxxxxx,
>> platform-driver-x86@xxxxxxxxxxxxxxx, lyude@xxxxxxxxxx, thomas@xxxxxxxx
>> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxx>
> Reviewed-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx/>
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
