Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models

Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models

[Andy Shevchenko]

On Fri, Apr 24, 2020 at 12:57 AM Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org> wrote:
>
> This adds dual fan control for the following models:
> P50, P51, P52, P70, P71, P72, P1 gen1, X1E gen1, P2 gen2, and X1E gen2.
>
> Both fans are controlled together as if they were a single fan.
>
> Tested on an X1 Extreme Gen1, an X1 Extreme Gen2, and a P50.
>
> The patch is defensive, it adds only specific supported machines, and falls
> back to the old behavior if both fans cannot be controlled.
>
> 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
> [1].
> All BIOS ids are taken from there. The X1E gen2 id is verified on my machine.
>
> Thanks to GitHub users voidworker and civic9 for the earlier patches and BIOS
> ids, and to users peter-stoll and sassman for testing the patch on their
> machines.
>

Pushed to my review and testing queue, thank you!

> [1]: https://github.com/vmatare/thinkfan/issues/58
>
> Signed-off-by: Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 43 ++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index da794dcfdd92..9e0f65e567be 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -318,6 +318,7 @@ static struct {
>         u32 uwb:1;
>         u32 fan_ctrl_status_undef:1;
>         u32 second_fan:1;
> +       u32 second_fan_ctl:1;
>         u32 beep_needs_two_args:1;
>         u32 mixer_no_level_control:1;
>         u32 battery_force_primary:1;
> @@ -8325,11 +8326,19 @@ 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_ctl) {
> +                       if (!fan_select_fan2() ||
> +                           !acpi_evalf(sfan_handle, NULL, NULL, "vd", level)) {
> +                               pr_warn("Couldn't set 2nd fan level, disabling support\n");
> +                               tp_features.second_fan_ctl = 0;
> +                       }
> +                       fan_select_fan1();
> +               }
> +               if (!acpi_evalf(sfan_handle, NULL, NULL, "vd", level))
> +                       return -EIO;
>                 break;
>
>         case TPACPI_FAN_WR_ACPI_FANS:
> @@ -8346,6 +8355,15 @@ 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_ctl) {
> +                       if (!fan_select_fan2() ||
> +                           !acpi_ec_write(fan_status_offset, level)) {
> +                               pr_warn("Couldn't set 2nd fan level, disabling support\n");
> +                               tp_features.second_fan_ctl = 0;
> +                       }
> +                       fan_select_fan1();
> +
> +               }
>                 if (!acpi_ec_write(fan_status_offset, level))
>                         return -EIO;
>                 else
> @@ -8764,6 +8782,7 @@ static const struct attribute_group fan_attr_group = {
>
>  #define TPACPI_FAN_Q1  0x0001          /* Unitialized HFSP */
>  #define TPACPI_FAN_2FAN        0x0002          /* EC 0x31 bit 0 selects fan2 */
> +#define TPACPI_FAN_2CTL        0x0004          /* selects fan2 control */
>
>  static const struct tpacpi_quirk fan_quirk_table[] __initconst = {
>         TPACPI_QEC_IBM('1', 'Y', TPACPI_FAN_Q1),
> @@ -8772,6 +8791,13 @@ 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', '1', 'D', TPACPI_FAN_2CTL),  /* P70 */
> +       TPACPI_Q_LNV3('N', '1', 'E', TPACPI_FAN_2CTL),  /* P50 */
> +       TPACPI_Q_LNV3('N', '1', 'T', TPACPI_FAN_2CTL),  /* P71 */
> +       TPACPI_Q_LNV3('N', '1', 'U', TPACPI_FAN_2CTL),  /* P51 */
> +       TPACPI_Q_LNV3('N', '2', 'C', TPACPI_FAN_2CTL),  /* P52 / P72 */
> +       TPACPI_Q_LNV3('N', '2', 'E', TPACPI_FAN_2CTL),  /* P1 / X1 Extreme (1st gen) */
> +       TPACPI_Q_LNV3('N', '2', 'O', TPACPI_FAN_2CTL),  /* P1 / X1 Extreme (2nd gen) */
>  };
>
>  static int __init fan_init(struct ibm_init_struct *iibm)
> @@ -8789,6 +8815,7 @@ static int __init fan_init(struct ibm_init_struct *iibm)
>         fan_watchdog_maxinterval = 0;
>         tp_features.fan_ctrl_status_undef = 0;
>         tp_features.second_fan = 0;
> +       tp_features.second_fan_ctl = 0;
>         fan_control_desired_level = 7;
>
>         if (tpacpi_is_ibm()) {
> @@ -8813,8 +8840,12 @@ 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");
> +                       }
> +                       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");
> --
> 2.25.3
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models

[Andy Shevchenko]

On Fri, Apr 24, 2020 at 12:57 AM Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org> wrote:
>
> This adds dual fan control for the following models:
> P50, P51, P52, P70, P71, P72, P1 gen1, X1E gen1, P2 gen2, and X1E gen2.
>
> Both fans are controlled together as if they were a single fan.
>
> Tested on an X1 Extreme Gen1, an X1 Extreme Gen2, and a P50.
>
> The patch is defensive, it adds only specific supported machines, and falls
> back to the old behavior if both fans cannot be controlled.
>
> 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
> [1].
> All BIOS ids are taken from there. The X1E gen2 id is verified on my machine.
>
> Thanks to GitHub users voidworker and civic9 for the earlier patches and BIOS
> ids, and to users peter-stoll and sassman for testing the patch on their
> machines.
>
> [1]: https://github.com/vmatare/thinkfan/issues/58
>
> Signed-off-by: Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>

One question though, is Lars your real name here? [1]

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models

[Andy Shevchenko]

On Mon, Apr 27, 2020 at 10:44 AM larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org> wrote:
>
>
> Hi Andy,
>
> my full name is Lars Hofhansl.
> Should I send a new post?
>
> Just in case, I hereby:
>
> Signed-off-by: Lars Hofhansl <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>

No need for this one, I will update locally, thanks!

>
> -- Lars
>
> On Friday, April 24, 2020, 4:12:05 AM PDT, Andy Shevchenko <andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>
>
>
>
> On Fri, Apr 24, 2020 at 12:57 AM Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org> wrote:
> >
> > This adds dual fan control for the following models:
> > P50, P51, P52, P70, P71, P72, P1 gen1, X1E gen1, P2 gen2, and X1E gen2.
> >
> > Both fans are controlled together as if they were a single fan.
> >
> > Tested on an X1 Extreme Gen1, an X1 Extreme Gen2, and a P50.
> >
> > The patch is defensive, it adds only specific supported machines, and falls
> > back to the old behavior if both fans cannot be controlled.
> >
> > 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
> > [1].
> > All BIOS ids are taken from there. The X1E gen2 id is verified on my machine.
> >
> > Thanks to GitHub users voidworker and civic9 for the earlier patches and BIOS
> > ids, and to users peter-stoll and sassman for testing the patch on their
> > machines.
> >
> > [1]: https://github.com/vmatare/thinkfan/issues/58
> >
> > Signed-off-by: Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org>
>
> One question though, is Lars your real name here? [1]
>
>
> [1]:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
>
> --
> With Best Regards,
> Andy Shevchenko
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models

[Dario Messina]

On Thu, Apr 23, 2020 at 23:57:59 CEST, Lars <larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org> wrote:
> This adds dual fan control for the following models:
> P50, P51, P52, P70, P71, P72, P1 gen1, X1E gen1, P2 gen2, and X1E gen2.
> 
> Both fans are controlled together as if they were a single fan.
> [...]
> 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.
Hi Lars, why have you chosen to control multiple fans in this way?
I know that BIOS controls both fans together, but the EC has the capabilities 
to control both fans independently, so maybe it can be convenient to expose 
this feature.


Distinti saluti/Best regards,
Dario Messina

Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models

[civic9]

pon., 27 kwi 2020 o 20:41 Dario Messina <nanodario@gmail.com> napisał(a):
>
> On Thu, Apr 23, 2020 at 23:57:59 CEST, Lars <larsh@apache.org> wrote:
> > This adds dual fan control for the following models:
> > P50, P51, P52, P70, P71, P72, P1 gen1, X1E gen1, P2 gen2, and X1E gen2.
> >
> > Both fans are controlled together as if they were a single fan.
> > [...]
> > 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.
> Hi Lars, why have you chosen to control multiple fans in this way?
> I know that BIOS controls both fans together, but the EC has the capabilities
> to control both fans independently, so maybe it can be convenient to expose
> this feature.

+1
Previous version of the patch [1] allows to control both fans independently.
However some software like thinkfan is not ready to control two fans.
But I also think this feature should be at least optionally exposed.

[1] https://github.com/civic9/thinkpad_acpi.2ndfan.patch/blob/master/thinkpad_acpi.2ndfan.patch/thinkpad_acpi.2ndfan.patch


_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel

Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models

[Henrique de Moraes Holschuh]

On Wed, 29 Apr 2020, Stefan Assmann wrote:
> On 29.04.20 02:20, larsh-1oDqGaOF3Lkdnm+yROfE0A@public.gmane.org wrote:
> > Do you have a use case for that behavior?
> > 
> > The previous patch broke the /proc interface, didn't not work with the current version of thinkfan
> > (but a a version with multi-fan support is in the works), and it had hard to track internal mutable state.
> > 
> > The proposed change is clean on all these fronts.
> > 
> > I'm not a fan of surprising the user with unnecessarily complex behavior (but perhaps this can be added as an option in the future.)
> 
> I concur to keep the patch as is. Any additional functionality could be
> added later on, if deemed necessary.

Yes, but let's avoid changing exposed APIs as much as possible...

Anyway, the correct interface *when exposing both fans* is:

1. Use the "hwmon" sysfs interface to expose each fan separately, and
control them separately.

1a. it is quite acceptable to control them in group by default, and have
a module parameter to select grouped, or separate behavior.

2. /proc/acpi/ibm/fan shall control both of them at the same time, and
report data from the first fan.  THIS INTERFACE IS FROZEN, LET'S NOT
MESS WITH IT.

Also, "fail-safe" for this is to have the two fans on automatic, and to
enable both fans.

All of that said, *it is very much acceptable* to merge a first set of
patches that controls both fans simultaneously and exposes the fan group
as if it were just the main fan, and later improve on the patch to
expose the second fan separately (provided safe group behavior is
maintained, see above).

-- 
  Henrique Holschuh

Re: [PATCH v4] thinkpad_acpi: Add support for dual fan control on select models

[civic9]

śr., 29 kwi 2020 o 02:21 larsh@apache.org <larsh@apache.org> napisał(a):
>
> Do you have a use case for that behavior?

I work very often with just one fan enabled at the lowest level. It is
inaudible for me and it does its job for not too heavy usage. If the
second one is also enabled I can hear them. We love Linux for such
small extra features too.


_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel