| Message ID | 20231127131651.476795-2-rui.zhang@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (coretemp) Fix core count limitation | expand |
On Mon, Nov 27, 2023 at 09:16:49PM +0800, Zhang Rui wrote: > Introduce enum coretemp_attr_index to better describe the index of each > sensor attribute and the maximum number of basic/possible attributes. > > No functional change. > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/hwmon/coretemp.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c > index ba82d1e79c13..6053ed3761c2 100644 > --- a/drivers/hwmon/coretemp.c > +++ b/drivers/hwmon/coretemp.c > @@ -43,10 +43,18 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius"); > #define BASE_SYSFS_ATTR_NO 2 /* Sysfs Base attr no for coretemp */ > #define NUM_REAL_CORES 128 /* Number of Real cores per cpu */ > #define CORETEMP_NAME_LENGTH 28 /* String Length of attrs */ > -#define MAX_CORE_ATTRS 4 /* Maximum no of basic attrs */ > -#define TOTAL_ATTRS (MAX_CORE_ATTRS + 1) > #define MAX_CORE_DATA (NUM_REAL_CORES + BASE_SYSFS_ATTR_NO) > > +enum coretemp_attr_index { > + ATTR_LABEL, > + ATTR_CRIT_ALARM, > + ATTR_TEMP, > + ATTR_TJMAX, > + ATTR_TTARGET, > + TOTAL_ATTRS, /* Maximum no of possible attrs */ > + MAX_CORE_ATTRS = ATTR_TJMAX + 1 /* Maximum no of basic attrs */ This seems odd. TOTAL_ATTRS being the last entry seems fine, but defining a MAX_CORE_ATTR the way above sounds a bit hacky. > +}; > + > #ifdef CONFIG_SMP > #define for_each_sibling(i, cpu) \ > for_each_cpu(i, topology_sibling_cpumask(cpu)) > -- > 2.34.1 >
On 11/30/23 13:51, Ashok Raj wrote: > On Mon, Nov 27, 2023 at 09:16:49PM +0800, Zhang Rui wrote: >> Introduce enum coretemp_attr_index to better describe the index of each >> sensor attribute and the maximum number of basic/possible attributes. >> >> No functional change. >> >> Signed-off-by: Zhang Rui <rui.zhang@intel.com> >> --- >> drivers/hwmon/coretemp.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c >> index ba82d1e79c13..6053ed3761c2 100644 >> --- a/drivers/hwmon/coretemp.c >> +++ b/drivers/hwmon/coretemp.c >> @@ -43,10 +43,18 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius"); >> #define BASE_SYSFS_ATTR_NO 2 /* Sysfs Base attr no for coretemp */ >> #define NUM_REAL_CORES 128 /* Number of Real cores per cpu */ >> #define CORETEMP_NAME_LENGTH 28 /* String Length of attrs */ >> -#define MAX_CORE_ATTRS 4 /* Maximum no of basic attrs */ >> -#define TOTAL_ATTRS (MAX_CORE_ATTRS + 1) >> #define MAX_CORE_DATA (NUM_REAL_CORES + BASE_SYSFS_ATTR_NO) >> >> +enum coretemp_attr_index { >> + ATTR_LABEL, >> + ATTR_CRIT_ALARM, >> + ATTR_TEMP, >> + ATTR_TJMAX, >> + ATTR_TTARGET, >> + TOTAL_ATTRS, /* Maximum no of possible attrs */ >> + MAX_CORE_ATTRS = ATTR_TJMAX + 1 /* Maximum no of basic attrs */ > > This seems odd. TOTAL_ATTRS being the last entry seems fine, but defining a > MAX_CORE_ATTR the way above sounds a bit hacky. > Complaining is easy. What do you suggest that would be better ? Guenter >> +}; >> + >> #ifdef CONFIG_SMP >> #define for_each_sibling(i, cpu) \ >> for_each_cpu(i, topology_sibling_cpumask(cpu)) >> -- >> 2.34.1 >> >
On Thu, Nov 30, 2023 at 08:14:48PM -0800, Guenter Roeck wrote: > On 11/30/23 13:51, Ashok Raj wrote: > > On Mon, Nov 27, 2023 at 09:16:49PM +0800, Zhang Rui wrote: > > > Introduce enum coretemp_attr_index to better describe the index of each > > > sensor attribute and the maximum number of basic/possible attributes. > > > > > > No functional change. > > > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > > > --- > > > drivers/hwmon/coretemp.c | 12 ++++++++++-- > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c > > > index ba82d1e79c13..6053ed3761c2 100644 > > > --- a/drivers/hwmon/coretemp.c > > > +++ b/drivers/hwmon/coretemp.c > > > @@ -43,10 +43,18 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius"); > > > #define BASE_SYSFS_ATTR_NO 2 /* Sysfs Base attr no for coretemp */ > > > #define NUM_REAL_CORES 128 /* Number of Real cores per cpu */ > > > #define CORETEMP_NAME_LENGTH 28 /* String Length of attrs */ > > > -#define MAX_CORE_ATTRS 4 /* Maximum no of basic attrs */ > > > -#define TOTAL_ATTRS (MAX_CORE_ATTRS + 1) > > > #define MAX_CORE_DATA (NUM_REAL_CORES + BASE_SYSFS_ATTR_NO) > > > +enum coretemp_attr_index { > > > + ATTR_LABEL, > > > + ATTR_CRIT_ALARM, > > > + ATTR_TEMP, > > > + ATTR_TJMAX, > > > + ATTR_TTARGET, > > > + TOTAL_ATTRS, /* Maximum no of possible attrs */ > > > + MAX_CORE_ATTRS = ATTR_TJMAX + 1 /* Maximum no of basic attrs */ > > > > This seems odd. TOTAL_ATTRS being the last entry seems fine, but defining a > > MAX_CORE_ATTR the way above sounds a bit hacky. > > > > Complaining is easy. What do you suggest that would be better ? > Fair enough. How about ATTR_LABEL, ATTR_CRIT_ALARM, ATTR_TEMP, ATTR_TJMAX, MAX_CORE_ATTRS, /* One more than TJMAX */ ATTR_TTARGET = MAX_CORE_ATTRS, TOTAL_ATTRS Each enum can be assigned any value, but this way they are just increasing order.
On Thu, 2023-11-30 at 20:47 -0800, Ashok Raj wrote: > On Thu, Nov 30, 2023 at 08:14:48PM -0800, Guenter Roeck wrote: > > On 11/30/23 13:51, Ashok Raj wrote: > > > On Mon, Nov 27, 2023 at 09:16:49PM +0800, Zhang Rui wrote: > > > > Introduce enum coretemp_attr_index to better describe the index > > > > of each > > > > sensor attribute and the maximum number of basic/possible > > > > attributes. > > > > > > > > No functional change. > > > > > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > > > > --- > > > > drivers/hwmon/coretemp.c | 12 ++++++++++-- > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/hwmon/coretemp.c > > > > b/drivers/hwmon/coretemp.c > > > > index ba82d1e79c13..6053ed3761c2 100644 > > > > --- a/drivers/hwmon/coretemp.c > > > > +++ b/drivers/hwmon/coretemp.c > > > > @@ -43,10 +43,18 @@ MODULE_PARM_DESC(tjmax, "TjMax value in > > > > degrees Celsius"); > > > > #define BASE_SYSFS_ATTR_NO 2 /* Sysfs Base attr no > > > > for coretemp */ > > > > #define NUM_REAL_CORES 128 /* Number of > > > > Real cores per cpu */ > > > > #define CORETEMP_NAME_LENGTH 28 /* String Length of > > > > attrs */ > > > > -#define MAX_CORE_ATTRS 4 /* Maximum no of basic > > > > attrs */ > > > > -#define TOTAL_ATTRS (MAX_CORE_ATTRS + 1) > > > > #define MAX_CORE_DATA (NUM_REAL_CORES + > > > > BASE_SYSFS_ATTR_NO) > > > > +enum coretemp_attr_index { > > > > + ATTR_LABEL, > > > > + ATTR_CRIT_ALARM, > > > > + ATTR_TEMP, > > > > + ATTR_TJMAX, > > > > + ATTR_TTARGET, > > > > + TOTAL_ATTRS, /* Maximum no of > > > > possible attrs */ > > > > + MAX_CORE_ATTRS = ATTR_TJMAX + 1 /* Maximum no of basic > > > > attrs */ > > > > > > This seems odd. TOTAL_ATTRS being the last entry seems fine, but > > > defining a > > > MAX_CORE_ATTR the way above sounds a bit hacky. > > > > > > > Complaining is easy. What do you suggest that would be better ? > > > Fair enough. > > How about > > ATTR_LABEL, > ATTR_CRIT_ALARM, > ATTR_TEMP, > ATTR_TJMAX, > MAX_CORE_ATTRS, /* One more than TJMAX */ > ATTR_TTARGET = MAX_CORE_ATTRS, > TOTAL_ATTRS > > Each enum can be assigned any value, but this way they are just > increasing > order. ATTR_TTARGET is the next attribute after ATTR_TJMAX so it should be right after ATTR_TJMAX. MAX_CORE_ATTRS is the number of basic attributes so it should be ATTR_TJMAX + 1. TOTAL_ATTRS is the number of possible attributes so it should be ATTR_TTARGET + 1 ATTR_LABEL, // 0 ATTR_CRIT_ALARM, // 1 ATTR_TEMP, // 2 ATTR_TJMAX, // 3 ATTR_TTARGET, // 4 MAX_CORE_ATTRS = ATTR_TJMAX + 1, // 4 TOTAL_ATTRS = ATTR_TTARGET + 1, // 5 How about this one? thanks, rui
On Fri, Dec 01, 2023 at 09:29:24AM -0800, Zhang, Rui wrote: [snip] > > > > How about > > > > ATTR_LABEL, > > ATTR_CRIT_ALARM, > > ATTR_TEMP, > > ATTR_TJMAX, > > MAX_CORE_ATTRS, /* One more than TJMAX */ > > ATTR_TTARGET = MAX_CORE_ATTRS, > > TOTAL_ATTRS > > > > Each enum can be assigned any value, but this way they are just > > increasing > > order. > > ATTR_TTARGET is the next attribute after ATTR_TJMAX so it should be > right after ATTR_TJMAX. > MAX_CORE_ATTRS is the number of basic attributes so it should be > ATTR_TJMAX + 1. > TOTAL_ATTRS is the number of possible attributes so it should be > ATTR_TTARGET + 1 > > ATTR_LABEL, // 0 > ATTR_CRIT_ALARM, // 1 > ATTR_TEMP, // 2 > ATTR_TJMAX, // 3 > ATTR_TTARGET, // 4 > MAX_CORE_ATTRS = ATTR_TJMAX + 1, // 4 > TOTAL_ATTRS = ATTR_TTARGET + 1, // 5 > > How about this one? Sorry for the delay... yes, this sounds fine.
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c index ba82d1e79c13..6053ed3761c2 100644 --- a/drivers/hwmon/coretemp.c +++ b/drivers/hwmon/coretemp.c @@ -43,10 +43,18 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius"); #define BASE_SYSFS_ATTR_NO 2 /* Sysfs Base attr no for coretemp */ #define NUM_REAL_CORES 128 /* Number of Real cores per cpu */ #define CORETEMP_NAME_LENGTH 28 /* String Length of attrs */ -#define MAX_CORE_ATTRS 4 /* Maximum no of basic attrs */ -#define TOTAL_ATTRS (MAX_CORE_ATTRS + 1) #define MAX_CORE_DATA (NUM_REAL_CORES + BASE_SYSFS_ATTR_NO) +enum coretemp_attr_index { + ATTR_LABEL, + ATTR_CRIT_ALARM, + ATTR_TEMP, + ATTR_TJMAX, + ATTR_TTARGET, + TOTAL_ATTRS, /* Maximum no of possible attrs */ + MAX_CORE_ATTRS = ATTR_TJMAX + 1 /* Maximum no of basic attrs */ +}; + #ifdef CONFIG_SMP #define for_each_sibling(i, cpu) \ for_each_cpu(i, topology_sibling_cpumask(cpu))
Introduce enum coretemp_attr_index to better describe the index of each sensor attribute and the maximum number of basic/possible attributes. No functional change. Signed-off-by: Zhang Rui <rui.zhang@intel.com> --- drivers/hwmon/coretemp.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)