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

Zhang Rui
2022-12-11T07:24:05Z

> > > + 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)

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

> > > +
> > > + 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)
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.

thanks,
rui
> > > + *
> > > + * Set the TCC Offset value to Tjmax. The effective thermal
> > > throttling or TCC
> > > + * activation temperature equals "Tjmax" - "TCC Offset", in
> > > degree
> > > C.
> > > + *
> > > + * Return: On success returns 0, an error code otherwise
> > > + */
> > > +
> > > +int intel_tcc_set_offset(int cpu, int offset)
> > > +{
> > > + u32 eax, edx;
> > > + int err;
> > > +
> > > + if (offset > 0x3f)
> > > + return -EINVAL;
> > > +
> > > + err = rdmsr_safe_on_cpu(cpu, MSR_IA32_TEMPERATURE_TARGET,
> > > + &eax, &edx);
> > > + if (err)
> > > + return err;
> > > +
> > > + if (eax & BIT(31))
> > > + return -EPERM;
> >
> > Why -EPERM?
>
> Bit 31 set means the MSR is locked, and os software cannot write it.
> should I use -EACCES instead?
>
> > > +
> > > + eax &= ~(0x3f << 24);
> > > + eax |= (offset << 24);
> >
> > The parens are not needed AFAICS.
> >
> Agreed.
>
> > > +
> > > + return wrmsr_safe_on_cpu(cpu,
> > > MSR_IA32_TEMPERATURE_TARGET,
> > > eax, edx);
> >
> > So is any protection against concurrent access needed here? Like
> > what
> > if two different callers invoke this function at the same time for
> > the
> > same CPU?
>
> Given that the tcc offset is the only field that kernel code writes,
> all the other bits won't change, so this is not a problem.
>
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_set_offset, INTEL_TCC);
> > > +
> > > +/**
> > > + * intel_tcc_get_temp() - returns the current temperature
> > > + * @cpu: cpu that the MSR should be run on.
> > > + * @pkg: true: Package Thermal Sensor. false: Core Thermal
> > > Sensor.
> > > + * @temp: a valid pointer to where to store the resulting
> > > temperature
> > > + *
> > > + * Get the current temperature returned by the CPU core/package
> > > level
> > > + * thermal sensor, in degrees C.
> > > + *
> > > + * Return: On success returns 0, an error code otherwise
> > > + */
> > > +int intel_tcc_get_temp(int cpu, bool pkg, int *temp)
> > > +{
> > > + u32 eax, edx;
> > > + u32 msr = pkg ? MSR_IA32_PACKAGE_THERM_STATUS :
> > > MSR_IA32_THERM_STATUS;
> > > + int tjmax, err;
> > > +
> > > + err = intel_tcc_get_tjmax(cpu, &tjmax);
> > > + if (err)
> > > + return err;
> >
> > Well, what if somebody change tjmax on this cpu while this function
> > is running?
>
> tjmax is read only. Only firmware can change its value at runtime,
> say
> this field is updated when SST-PP level is changed.
>
> thanks,
> rui
> > > +
> > > + err = rdmsr_safe_on_cpu(cpu, msr, &eax, &edx);
> > > + if (err)
> > > + return err;
> > > +
> > > + if (eax & 0x80000000) {
> > > + *temp = tjmax - ((eax >> 16) & 0x7f);
> > > + return 0;
> > > + }
> > > + return -EINVAL;
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_temp, INTEL_TCC);
> > > +
> > > diff --git a/include/linux/intel_tcc.h
> > > b/include/linux/intel_tcc.h
> > > new file mode 100644
> > > index 000000000000..94f8ceab5dd0
> > > --- /dev/null
> > > +++ b/include/linux/intel_tcc.h
> > > @@ -0,0 +1,18 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * header for Intel TCC (thermal control circuitry) library
> > > + *
> > > + * Copyright (C) 2022 Intel Corporation.
> > > + */
> > > +
> > > +#ifndef __INTEL_TCC_H__
> > > +#define __INTEL_TCC_H__
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +int intel_tcc_get_tjmax(int cpu, int *tjmax);
> > > +int intel_tcc_get_offset(int cpu, int *offset);
> > > +int intel_tcc_set_offset(int cpu, int offset);
> > > +int intel_tcc_get_temp(int cpu, bool pkg, int *temp);
> > > +
> > > +#endif /* __INTEL_TCC_H__ */
> > > --