Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

[Zhang Rui <rui.zhang@intel.com>]

On Tue, 2022-12-13 at 16:34 +0100, Rafael J. Wysocki wrote:
> On Tue, Dec 13, 2022 at 2:38 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> > On Mon, 2022-12-12 at 13:11 +0100, Rafael J. Wysocki wrote:
> > > On Sun, Dec 11, 2022 at 8:24 AM Zhang Rui <rui.zhang@intel.com>
> > > wrote:
> > > > > > > +       if (err)
> > > > > > > +               return err;
> > > > > > > +
> > > > > > > +       *tjmax = (eax >> 16) & 0xff;
> > > > > > 
> > > > > > This means that the tjmax value cannot be negative.
> > > > > > 
> > > > > > > +
> > > > > > > +       return *tjmax ? 0 : -EINVAL;
> > > > > > 
> > > > > > So the return value of this function could be tjmax if
> > > > > > positive
> > > > > > or
> > > > > > a
> > > > > > negative error code otherwise.  No return pointers needed.
> > > > > 
> > > > > I tried both. And I think I chose this solution just because
> > > > > it
> > > > > makes
> > > > > the following cleanup patches in this series looks prettier.
> > > > > 
> > > > > I will try your suggestion, and if there is any other reason
> > > > > I
> > > > > wrote
> > > > > it
> > > > > in this way, I will find it out again. :p
> > > > 
> > > > I see.
> > > > Say, we have to use
> > > > 
> > > > intel_tcc_get_temp(int cpu, int *temp)
> > > 
> > > Do we?
> > 
> > temp = tjmax - digital_readout
> > 
> > tjmax and digital_readout are from MSR and they are both positive
> > values, but temp can be negative in theory.
> 
> I know, but is this ever expected to happen?
> 
> > so are you suggesting that we should treat the negative CPU
> > temperature
> > as an error because this won't happen in real world?
> 
> No.
> 
> If it's necessary to handle negative temperatures, an int is needed
> and the additional return value is useful.
> 
> Now there is also the question of what the callers will do with a
> negative temperature value.  Do they care?

Currently, all the callers are from sysfs attribute.
for userspace tools, at least thermald ignores negative CPU
temperature.
So I will return an error for negative temperature in next version.

thanks,
rui