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

Rafael J. Wysocki
2022-12-14T16:17:16Z

On Wed, Dec 14, 2022 at 5:15 PM Zhang Rui <rui.zhang@intel.com> wrote:
>
> 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.

Sounds good.