[UPDATE][PATCH] thermal: int340x: Mode setting with new OS handshake

[UPDATE][PATCH] thermal: int340x: Mode setting with new OS handshake

[Srinivas Pandruvada]

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@linux.intel.com>
Cc: stable@vger.kernel.org
---
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);
+			}
+			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;
-- 
2.31.1

Re: [UPDATE][PATCH] thermal: int340x: Mode setting with new OS handshake

[Rafael J. Wysocki]

On Tue, May 10, 2022 at 8:22 PM Srinivas Pandruvada
<srinivas.pandruvada@linux.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@linux.intel.com>
> Cc: stable@vger.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!

Re: [UPDATE][PATCH] thermal: int340x: Mode setting with new OS handshake

[srinivas pandruvada]

On Wed, 2022-05-11 at 20:14 +0200, Rafael J. Wysocki wrote:
> On Tue, May 10, 2022 at 8:22 PM Srinivas Pandruvada
> <srinivas.pandruvada@linux.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@linux.intel.com>
> > Cc: stable@vger.kernel.org
> 
> This is not -stable material yet.
I thought it will wait for 5.19 merge window.

> 
> > ---
> > update:
> > Added Fixes tag
> > 
> > 

[...]

> > +               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.
> 
I checked with the firmware team. The _OSC changes dynamically is
validated and is recommended when enable/disable user space thermal
control.
Looking at ACPI Spec
"OSPM may evaluate _OSC multiple times to indicate changes in OSPM
capability to the device but this may be precluded by specific device
requirements"


> But I suppose you have tested this?
I tested on TigerLake system.

> 
> > +                       }
> > +    

[...]

> 
> 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.
I tested the patch from bleeding edge.
Works fine.

Thanks,
Srinivas

> 
> Thanks!

Re: [UPDATE][PATCH] thermal: int340x: Mode setting with new OS handshake

[Rafael J. Wysocki]

On Wed, May 11, 2022 at 9:56 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Wed, 2022-05-11 at 20:14 +0200, Rafael J. Wysocki wrote:
> > On Tue, May 10, 2022 at 8:22 PM Srinivas Pandruvada
> > <srinivas.pandruvada@linux.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@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> >
> > This is not -stable material yet.
> I thought it will wait for 5.19 merge window.

It is better to avoid making a major release with a known-broken
interface if possible.

> >
> > > ---
> > > update:
> > > Added Fixes tag
> > >
> > >
>
> [...]
>
> > > +               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.
> >
> I checked with the firmware team. The _OSC changes dynamically is
> validated and is recommended when enable/disable user space thermal
> control.

OK

> Looking at ACPI Spec
> "OSPM may evaluate _OSC multiple times to indicate changes in OSPM
> capability to the device but this may be precluded by specific device
> requirements"

Well, different paragraphs say different things, but it is fine as
long as the firmware and the OS are on the same page in the given use
case.

> > But I suppose you have tested this?
> I tested on TigerLake system.

OK

> >
> > > +                       }
> > > +
>
> [...]
>
> >
> > 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.
> I tested the patch from bleeding edge.
> Works fine.

Good.

I'll queue it up for -rc7 then (I will be offline for the next few
days, most likely, and I'm not ready to send a pull request today).

Thanks!