| Message ID | 20220502191200.63470-1-markpearson@lenovo.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: thinkpad_acpi: Correct dual fan probe | expand |
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;
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/
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
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
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
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;
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;
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(-)