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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> 
> 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!