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

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

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.
so are you suggesting that we should treat the negative CPU temperature
as an error because this won't happen in real world?

thanks,
rui

> 
> > so I keep the same format for all the intel_tcc_get_XXX APIs
> > 
> > intel_tcc_get_tjmax(int cpu, int *tjmax)
> > intel_tcc_get_offset(int cpu, int *offset)
> > 
> > so that the return value is decoded in the same way. This helps
> > avoid
> > return value check mistakes for the callers.
> > 
> > surely I can remove the return pointer if you still prefer that. :p
> 
> Using a function value directly is very much preferred unless there
> really are two values to return.
> 
> In these particular cases a negative return value can be clearly
> interpreted as an error condition, so using the return value directly
> is sufficient.
> 
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_offset, INTEL_TCC);
> > > > > +
> > > > > +/**
> > > > > + * intel_tcc_set_offset() - set the TCC offset value to
> > > > > Tjmax
> > > > > + * @cpu: cpu that the MSR should be run on.
> > > > > + * @offset: TCC offset value in degree C
> > > > 
> > > > I think that this cannot be negative, so maybe say "in K"
> > > > instead
> > > > of
> > > > "in degree C"?
> > > 
> > > degree C is the unit used in the Intel SDM, so better to keep
> > > this
> > > comment aligned.
> > > 
> > > > And maybe it's better to pass u8 here?
> > > 
> > > sounds good, will do in next version.
> > > 
> > to keep consistent, we can use either
> > int intel_tcc_get_offset(int cpu)
> > int intel_tcc_set_offset(int cpu, int offset)
> 
> What about intel_tcc_set_offset(int cpu, u8 offset)?
> 
> > or
> > int intel_tcc_get_offset(int cpu, u8 *offset)
> > int intel_tcc_set_offset(int cpu, u8 offset)
> > 
> > or else callers need to declare 'offset' but with different type,
> > when
> > getting and setting it, which looks strange.
> 
> Well, one can surely pass an int as a u8 argument and assign a u8
> return value to an int variable.