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

From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Zhang Rui <rui.zhang@intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	rjw@rjwysocki.net, daniel.lezcano@linaro.org,
	linux-pm@vger.kernel.org, srinivas.pandruvada@linux.intel.com
Subject: Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library
Date: Tue, 13 Dec 2022 16:34:11 +0100	[thread overview]
Message-ID: <CAJZ5v0gscS-b+B-uqSFOVX=wA-gABP8f9n+oJ5WMZTRcrs66bQ@mail.gmail.com> (raw)
In-Reply-To: <10cae7a3db86278c39ecb8837ec000b054c51ebc.camel@intel.com>

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?