Message-ID: <CAJZ5v0hDN=iGBQei6XeJ1b3qLiRxPDm+ZFtKU1PcHbBcyxGpZw@mail.gmail.com>
Date:   Wed, 11 May 2022 20:14:00 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Amit Kucheria <amitk@...nel.org>,
        "Zhang, Rui" <rui.zhang@...el.com>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Stable <stable@...r.kernel.org>
Subject: Re: [UPDATE][PATCH] thermal: int340x: Mode setting with new OS handshake

On Tue, May 10, 2022 at 8:22 PM Srinivas Pandruvada
<srinivas.pandruvada@...ux.intel.com> wrote:
>
> With the new OS handshake introduced with the commit: "c7ff29763989
> ("thermal: int340x: Update OS policy capability handshake")",
> thermal zone mode "enabled" doesn't work in the same way as the legacy
> handshake. The mode "enabled" fails with -EINVAL using new handshake.
>
> To address this issue, when the new OS UUID mask is set:
> - When mode is "enabled", return 0 as the firmware already has the
> latest policy mask.
> - When mode is "disabled", update the firmware with UUID mask of zero.
> In this way firmware can take control of the thermal control. Also
> reset the OS UUID mask. This allows user space to update with new
> set of policies.
>
> Fixes: c7ff29763989 ("thermal: int340x: Update OS policy capability handshake")
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> Cc: stable@...r.kernel.org

This is not -stable material yet.

> ---
> update:
> Added Fixes tag
>
>  .../intel/int340x_thermal/int3400_thermal.c   | 48 ++++++++++++-------
>  1 file changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> index d97f496bab9b..1061728ad5a9 100644
> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> @@ -194,12 +194,31 @@ static int int3400_thermal_run_osc(acpi_handle handle, char *uuid_str, int *enab
>         return result;
>  }
>
> +static int set_os_uuid_mask(struct int3400_thermal_priv *priv, u32 mask)
> +{
> +       int cap = 0;
> +
> +       /*
> +        * Capability bits:
> +        * Bit 0: set to 1 to indicate DPTF is active
> +        * Bi1 1: set to 1 to active cooling is supported by user space daemon
> +        * Bit 2: set to 1 to passive cooling is supported by user space daemon
> +        * Bit 3: set to 1 to critical trip is handled by user space daemon
> +        */
> +       if (mask)
> +               cap = ((priv->os_uuid_mask << 1) | 0x01);
> +
> +       return int3400_thermal_run_osc(priv->adev->handle,
> +                                      "b23ba85d-c8b7-3542-88de-8de2ffcfd698",
> +                                      &cap);
> +}
> +
>  static ssize_t current_uuid_store(struct device *dev,
>                                   struct device_attribute *attr,
>                                   const char *buf, size_t count)
>  {
>         struct int3400_thermal_priv *priv = dev_get_drvdata(dev);
> -       int i;
> +       int ret, i;
>
>         for (i = 0; i < INT3400_THERMAL_MAXIMUM_UUID; ++i) {
>                 if (!strncmp(buf, int3400_thermal_uuids[i],
> @@ -231,19 +250,7 @@ static ssize_t current_uuid_store(struct device *dev,
>         }
>
>         if (priv->os_uuid_mask) {
> -               int cap, ret;
> -
> -               /*
> -                * Capability bits:
> -                * Bit 0: set to 1 to indicate DPTF is active
> -                * Bi1 1: set to 1 to active cooling is supported by user space daemon
> -                * Bit 2: set to 1 to passive cooling is supported by user space daemon
> -                * Bit 3: set to 1 to critical trip is handled by user space daemon
> -                */
> -               cap = ((priv->os_uuid_mask << 1) | 0x01);
> -               ret = int3400_thermal_run_osc(priv->adev->handle,
> -                                             "b23ba85d-c8b7-3542-88de-8de2ffcfd698",
> -                                             &cap);
> +               ret = set_os_uuid_mask(priv, priv->os_uuid_mask);
>                 if (ret)
>                         return ret;
>         }
> @@ -469,17 +476,26 @@ static int int3400_thermal_change_mode(struct thermal_zone_device *thermal,
>         if (mode != thermal->mode) {
>                 int enabled;
>
> +               enabled = (mode == THERMAL_DEVICE_ENABLED);
> +
> +               if (priv->os_uuid_mask) {
> +                       if (!enabled) {
> +                               priv->os_uuid_mask = 0;
> +                               result = set_os_uuid_mask(priv, priv->os_uuid_mask);

This change worries me a bit, because it means replaying an already
established _OSC handshake which shouldn't be done by the spec.

But I suppose you have tested this?

> +                       }
> +                       goto eval_odvp;
> +               }
> +
>                 if (priv->current_uuid_index < 0 ||
>                     priv->current_uuid_index >= INT3400_THERMAL_MAXIMUM_UUID)
>                         return -EINVAL;
>
> -               enabled = (mode == THERMAL_DEVICE_ENABLED);
>                 result = int3400_thermal_run_osc(priv->adev->handle,
>                                                  int3400_thermal_uuids[priv->current_uuid_index],
>                                                  &enabled);
>         }
>
> -
> +eval_odvp:
>         evaluate_odvp(priv);
>
>         return result;
> --

Patch applied as 5.18-rc material, but I've removed some unneeded
parens from the new code, so please double check the result in
bleeding-edge.

Thanks!

Powered by blists - more mailing lists