| Message ID | 20200417201426.24033-1-larsh@apache.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] thinkpad_acpi: Add support for dual fan control on select models | expand |
On Fri, Apr 17, 2020 at 11:15 PM Lars <larsh@apache.org> wrote: > > This patch allows controlling multiple fans as if they were a single fan. > > This adds P52, P72, X1E, and X1E gen2 to dual fan quirks. Both fans are controlled together. > > Tested on an X1 Extreme Gen2. > > The patch is defensive, it adds only specific supported machines, and falls back to the old behavior if both fans cannot be controlled. > However, it does attempt single fan control for all previously white-listed Thinkpads. > > Background: > I tested the BIOS default behavior on my X1E gen2 and both fans are always changed together. > So rather than adding controls for each fan, this controls both fans together as the BIOS would do. > > This was inspired by a discussion on dual fan support for the thinkfan tool (https://github.com/vmatare/thinkfan/issues/58). > (Thanks to Github users voidworker, and civic9.) > > The BIOS ids for P52/P72 and X1E are taken from there. The X1E gen2 id is verified on my machine. > Thanks for an update. I have pushed it to my review and testing queue, thanks! JFYI: there are two issues (I have fixed them, no need to resend) with this. Commit message lines are too long and... > (In the first version my mail client botched the white-spacing - my apologies, this is my first Kernel patch. Used git send-email and gmail this time.) ...this kind of comments should go after cut line ('---' below). > Signed-off-by: Lars <larsh@apache.org> > ---
+Cc: people who lately involved in 2nd fan discussions here and there Lars, also one question regarding the code below. On Fri, Apr 17, 2020 at 11:15 PM Lars <larsh@apache.org> wrote: > > This patch allows controlling multiple fans as if they were a single fan. > > This adds P52, P72, X1E, and X1E gen2 to dual fan quirks. Both fans are controlled together. > > Tested on an X1 Extreme Gen2. > > The patch is defensive, it adds only specific supported machines, and falls back to the old behavior if both fans cannot be controlled. > However, it does attempt single fan control for all previously white-listed Thinkpads. > > Background: > I tested the BIOS default behavior on my X1E gen2 and both fans are always changed together. > So rather than adding controls for each fan, this controls both fans together as the BIOS would do. > > This was inspired by a discussion on dual fan support for the thinkfan tool (https://github.com/vmatare/thinkfan/issues/58). > (Thanks to Github users voidworker, and civic9.) > > The BIOS ids for P52/P72 and X1E are taken from there. The X1E gen2 id is verified on my machine. > > (In the first version my mail client botched the white-spacing - my apologies, this is my first Kernel patch. Used git send-email and gmail this time.) > > Signed-off-by: Lars <larsh@apache.org> > --- > drivers/platform/x86/thinkpad_acpi.c | 33 +++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 6 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 8eaadbaf8ffa..cbc0e85d89d2 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -8324,11 +8324,20 @@ static int fan_set_level(int level) > > switch (fan_control_access_mode) { > case TPACPI_FAN_WR_ACPI_SFAN: > - if (level >= 0 && level <= 7) { > - if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) > - return -EIO; > - } else > + if (((level < 0) || (level > 7))) > return -EINVAL; > + > + if (tp_features.second_fan) { > + if (!fan_select_fan2() || > + !acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) { > + fan_select_fan1(); > + pr_warn("Couldn't set 2nd fan level, disabling support\n"); > + tp_features.second_fan = 0; > + } > + fan_select_fan1(); > + } > + if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) > + return -EIO; > break; > > case TPACPI_FAN_WR_ACPI_FANS: > @@ -8345,6 +8354,16 @@ static int fan_set_level(int level) > else if (level & TP_EC_FAN_AUTO) > level |= 4; /* safety min speed 4 */ > > + if (tp_features.second_fan) { > + if (!fan_select_fan2() || > + !acpi_ec_write(fan_status_offset, level)) { > + fan_select_fan1(); (1) > + pr_warn("Couldn't set 2nd fan level, disabling support\n"); > + tp_features.second_fan = 0; > + } > + fan_select_fan1(); (2) I'm not sure I got a logic behind this. Why do you need to call it twice? > + > + } > if (!acpi_ec_write(fan_status_offset, level)) > return -EIO; > else > @@ -8771,6 +8790,9 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = { > TPACPI_QEC_IBM('7', '0', TPACPI_FAN_Q1), > TPACPI_QEC_LNV('7', 'M', TPACPI_FAN_2FAN), > TPACPI_Q_LNV('N', '1', TPACPI_FAN_2FAN), > + TPACPI_Q_LNV3('N', '2', 'C', TPACPI_FAN_2FAN), /* P52 / P72 */ > + TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2FAN), /* X1 Extreme (1st gen) */ > + TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2FAN), /* X1 Extreme (2nd gen) */ > }; > > static int __init fan_init(struct ibm_init_struct *iibm) > @@ -8812,8 +8834,7 @@ static int __init fan_init(struct ibm_init_struct *iibm) > fan_quirk1_setup(); > if (quirks & TPACPI_FAN_2FAN) { > tp_features.second_fan = 1; > - dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_FAN, > - "secondary fan support enabled\n"); > + pr_info("secondary fan support enabled\n"); > } > } else { > pr_err("ThinkPad ACPI EC access misbehaving, fan status and control unavailable\n"); > -- > 2.25.2 > -- With Best Regards, Andy Shevchenko
On 22.04.20 15:50, Andy Shevchenko wrote: > +Cc: people who lately involved in 2nd fan discussions here and there > > Lars, also one question regarding the code below. > > On Fri, Apr 17, 2020 at 11:15 PM Lars <larsh@apache.org> wrote: >> >> This patch allows controlling multiple fans as if they were a single fan. >> >> This adds P52, P72, X1E, and X1E gen2 to dual fan quirks. Both fans are controlled together. >> >> Tested on an X1 Extreme Gen2. >> >> The patch is defensive, it adds only specific supported machines, and falls back to the old behavior if both fans cannot be controlled. >> However, it does attempt single fan control for all previously white-listed Thinkpads. >> >> Background: >> I tested the BIOS default behavior on my X1E gen2 and both fans are always changed together. >> So rather than adding controls for each fan, this controls both fans together as the BIOS would do. >> >> This was inspired by a discussion on dual fan support for the thinkfan tool (https://github.com/vmatare/thinkfan/issues/58). >> (Thanks to Github users voidworker, and civic9.) >> >> The BIOS ids for P52/P72 and X1E are taken from there. The X1E gen2 id is verified on my machine. >> >> (In the first version my mail client botched the white-spacing - my apologies, this is my first Kernel patch. Used git send-email and gmail this time.) >> >> Signed-off-by: Lars <larsh@apache.org> >> --- >> drivers/platform/x86/thinkpad_acpi.c | 33 +++++++++++++++++++++++----- >> 1 file changed, 27 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index 8eaadbaf8ffa..cbc0e85d89d2 100644 [...] >> @@ -8771,6 +8790,9 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = { >> TPACPI_QEC_IBM('7', '0', TPACPI_FAN_Q1), >> TPACPI_QEC_LNV('7', 'M', TPACPI_FAN_2FAN), >> TPACPI_Q_LNV('N', '1', TPACPI_FAN_2FAN), >> + TPACPI_Q_LNV3('N', '2', 'C', TPACPI_FAN_2FAN), /* P52 / P72 */ >> + TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2FAN), /* X1 Extreme (1st gen) */ >> + TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2FAN), /* X1 Extreme (2nd gen) */ Please add TPACPI_Q_LNV3('N', '1', 'E', TPACPI_FAN_2FAN), /* P50 */ for the P50 in the next revision. I've tested the P50 today. Thanks! Stefan
I agree. Lemme amend the patch. I was confused about function of TPACPI_Q_LNV and TPACPI_Q_LNV3. On Wednesday, April 22, 2020, 7:28:51 AM PDT, Stefan Assmann <sassmann@kpanic.de> wrote: On 22.04.20 15:50, Andy Shevchenko wrote: > +Cc: people who lately involved in 2nd fan discussions here and there > > Lars, also one question regarding the code below. > > On Fri, Apr 17, 2020 at 11:15 PM Lars <larsh@apache.org> wrote: >> >> This patch allows controlling multiple fans as if they were a single fan. >> >> This adds P52, P72, X1E, and X1E gen2 to dual fan quirks. Both fans are controlled together. >> >> Tested on an X1 Extreme Gen2. >> >> The patch is defensive, it adds only specific supported machines, and falls back to the old behavior if both fans cannot be controlled. >> However, it does attempt single fan control for all previously white-listed Thinkpads. >> >> Background: >> I tested the BIOS default behavior on my X1E gen2 and both fans are always changed together. >> So rather than adding controls for each fan, this controls both fans together as the BIOS would do. >> >> This was inspired by a discussion on dual fan support for the thinkfan tool (https://github.com/vmatare/thinkfan/issues/58). >> (Thanks to Github users voidworker, and civic9.) >> >> The BIOS ids for P52/P72 and X1E are taken from there. The X1E gen2 id is verified on my machine. >> >> (In the first version my mail client botched the white-spacing - my apologies, this is my first Kernel patch. Used git send-email and gmail this time.) >> >> Signed-off-by: Lars <larsh@apache.org> >> --- >> drivers/platform/x86/thinkpad_acpi.c | 33 +++++++++++++++++++++++----- >> 1 file changed, 27 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c >> index 8eaadbaf8ffa..cbc0e85d89d2 100644 [...] >> @@ -8771,6 +8790,9 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = { >> TPACPI_QEC_IBM('7', '0', TPACPI_FAN_Q1), >> TPACPI_QEC_LNV('7', 'M', TPACPI_FAN_2FAN), >> TPACPI_Q_LNV('N', '1', TPACPI_FAN_2FAN), >> + TPACPI_Q_LNV3('N', '2', 'C', TPACPI_FAN_2FAN), /* P52 / P72 */ >> + TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2FAN), /* X1 Extreme (1st gen) */ >> + TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2FAN), /* X1 Extreme (2nd gen) */ Please add TPACPI_Q_LNV3('N', '1', 'E', TPACPI_FAN_2FAN), /* P50 */ for the P50 in the next revision. I've tested the P50 today. Thanks! Stefan
The double call is needed because in the case that the call is not supported dual fan support is disabled (tp_features.second_fan = 0;), which in turn makes fan_select_fan1(); a no-op. That is why resetting to fan1 needs to be before disabling second fan support. I will post a new version that does not need that any longer, because I'm separating fan query support from fan control support. On Wednesday, April 22, 2020, 6:50:28 AM PDT, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: +Cc: people who lately involved in 2nd fan discussions here and there Lars, also one question regarding the code below. On Fri, Apr 17, 2020 at 11:15 PM Lars <larsh@apache.org> wrote: > > This patch allows controlling multiple fans as if they were a single fan. > > This adds P52, P72, X1E, and X1E gen2 to dual fan quirks. Both fans are controlled together. > > Tested on an X1 Extreme Gen2. > > The patch is defensive, it adds only specific supported machines, and falls back to the old behavior if both fans cannot be controlled. > However, it does attempt single fan control for all previously white-listed Thinkpads. > > Background: > I tested the BIOS default behavior on my X1E gen2 and both fans are always changed together. > So rather than adding controls for each fan, this controls both fans together as the BIOS would do. > > This was inspired by a discussion on dual fan support for the thinkfan tool (https://github.com/vmatare/thinkfan/issues/58). > (Thanks to Github users voidworker, and civic9.) > > The BIOS ids for P52/P72 and X1E are taken from there. The X1E gen2 id is verified on my machine. > > (In the first version my mail client botched the white-spacing - my apologies, this is my first Kernel patch. Used git send-email and gmail this time.) > > Signed-off-by: Lars <larsh@apache.org> > --- > drivers/platform/x86/thinkpad_acpi.c | 33 +++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 6 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 8eaadbaf8ffa..cbc0e85d89d2 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -8324,11 +8324,20 @@ static int fan_set_level(int level) > > switch (fan_control_access_mode) { > case TPACPI_FAN_WR_ACPI_SFAN: > - if (level >= 0 && level <= 7) { > - if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) > - return -EIO; > - } else > + if (((level < 0) || (level > 7))) > return -EINVAL; > + > + if (tp_features.second_fan) { > + if (!fan_select_fan2() || > + !acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) { > + fan_select_fan1(); > + pr_warn("Couldn't set 2nd fan level, disabling support\n"); > + tp_features.second_fan = 0; > + } > + fan_select_fan1(); > + } > + if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) > + return -EIO; > break; > > case TPACPI_FAN_WR_ACPI_FANS: > @@ -8345,6 +8354,16 @@ static int fan_set_level(int level) > else if (level & TP_EC_FAN_AUTO) > level |= 4; /* safety min speed 4 */ > > + if (tp_features.second_fan) { > + if (!fan_select_fan2() || > + !acpi_ec_write(fan_status_offset, level)) { > + fan_select_fan1(); (1) > + pr_warn("Couldn't set 2nd fan level, disabling support\n"); > + tp_features.second_fan = 0; > + } > + fan_select_fan1(); (2) I'm not sure I got a logic behind this. Why do you need to call it twice? > + > + } > if (!acpi_ec_write(fan_status_offset, level)) > return -EIO; > else > @@ -8771,6 +8790,9 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = { > TPACPI_QEC_IBM('7', '0', TPACPI_FAN_Q1), > TPACPI_QEC_LNV('7', 'M', TPACPI_FAN_2FAN), > TPACPI_Q_LNV('N', '1', TPACPI_FAN_2FAN), > + TPACPI_Q_LNV3('N', '2', 'C', TPACPI_FAN_2FAN), /* P52 / P72 */ > + TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2FAN), /* X1 Extreme (1st gen) */ > + TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2FAN), /* X1 Extreme (2nd gen) */ > }; > > static int __init fan_init(struct ibm_init_struct *iibm) > @@ -8812,8 +8834,7 @@ static int __init fan_init(struct ibm_init_struct *iibm) > fan_quirk1_setup(); > if (quirks & TPACPI_FAN_2FAN) { > tp_features.second_fan = 1; > - dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_FAN, > - "secondary fan support enabled\n"); > + pr_info("secondary fan support enabled\n"); > } > } else { > pr_err("ThinkPad ACPI EC access misbehaving, fan status and control unavailable\n"); > -- > 2.25.2 > -- With Best Regards, Andy Shevchenko
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 8eaadbaf8ffa..cbc0e85d89d2 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -8324,11 +8324,20 @@ static int fan_set_level(int level) switch (fan_control_access_mode) { case TPACPI_FAN_WR_ACPI_SFAN: - if (level >= 0 && level <= 7) { - if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) - return -EIO; - } else + if (((level < 0) || (level > 7))) return -EINVAL; + + if (tp_features.second_fan) { + if (!fan_select_fan2() || + !acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) { + fan_select_fan1(); + pr_warn("Couldn't set 2nd fan level, disabling support\n"); + tp_features.second_fan = 0; + } + fan_select_fan1(); + } + if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) + return -EIO; break; case TPACPI_FAN_WR_ACPI_FANS: @@ -8345,6 +8354,16 @@ static int fan_set_level(int level) else if (level & TP_EC_FAN_AUTO) level |= 4; /* safety min speed 4 */ + if (tp_features.second_fan) { + if (!fan_select_fan2() || + !acpi_ec_write(fan_status_offset, level)) { + fan_select_fan1(); + pr_warn("Couldn't set 2nd fan level, disabling support\n"); + tp_features.second_fan = 0; + } + fan_select_fan1(); + + } if (!acpi_ec_write(fan_status_offset, level)) return -EIO; else @@ -8771,6 +8790,9 @@ static const struct tpacpi_quirk fan_quirk_table[] __initconst = { TPACPI_QEC_IBM('7', '0', TPACPI_FAN_Q1), TPACPI_QEC_LNV('7', 'M', TPACPI_FAN_2FAN), TPACPI_Q_LNV('N', '1', TPACPI_FAN_2FAN), + TPACPI_Q_LNV3('N', '2', 'C', TPACPI_FAN_2FAN), /* P52 / P72 */ + TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2FAN), /* X1 Extreme (1st gen) */ + TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2FAN), /* X1 Extreme (2nd gen) */ }; static int __init fan_init(struct ibm_init_struct *iibm) @@ -8812,8 +8834,7 @@ static int __init fan_init(struct ibm_init_struct *iibm) fan_quirk1_setup(); if (quirks & TPACPI_FAN_2FAN) { tp_features.second_fan = 1; - dbg_printk(TPACPI_DBG_INIT | TPACPI_DBG_FAN, - "secondary fan support enabled\n"); + pr_info("secondary fan support enabled\n"); } } else { pr_err("ThinkPad ACPI EC access misbehaving, fan status and control unavailable\n");
This patch allows controlling multiple fans as if they were a single fan. This adds P52, P72, X1E, and X1E gen2 to dual fan quirks. Both fans are controlled together. Tested on an X1 Extreme Gen2. The patch is defensive, it adds only specific supported machines, and falls back to the old behavior if both fans cannot be controlled. However, it does attempt single fan control for all previously white-listed Thinkpads. Background: I tested the BIOS default behavior on my X1E gen2 and both fans are always changed together. So rather than adding controls for each fan, this controls both fans together as the BIOS would do. This was inspired by a discussion on dual fan support for the thinkfan tool (https://github.com/vmatare/thinkfan/issues/58). (Thanks to Github users voidworker, and civic9.) The BIOS ids for P52/P72 and X1E are taken from there. The X1E gen2 id is verified on my machine. (In the first version my mail client botched the white-spacing - my apologies, this is my first Kernel patch. Used git send-email and gmail this time.) Signed-off-by: Lars <larsh@apache.org> --- drivers/platform/x86/thinkpad_acpi.c | 33 +++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 6 deletions(-)