On Wed, May 11, 2022 at 9:56 PM srinivas pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> 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@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.
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!
