On Sun, Jan 08, 2012 at 04:45:02PM -0500, Jean Delvare wrote:
> Hi Guenter,
>
> Sorry for the late review.
>
> On Sun, 6 Nov 2011 14:46:22 -0800, Guenter Roeck wrote:
> > Add support for new sysfs attributes to libsensors and to the sensors command.
> >
> > v2: Use defines for array sizes. Some of the resulting arrays are larger than necessary,
> > but that is better than missing an array size update if the number of attributes
> > changes.
>
> I don't get the logic. I understand the point of computing the sizes
> automatically to avoid an overflow [1], but then why don't you compute
> the right values? It's only a matter of adding a "- 1" after every call
> to ARRAY_SIZE(), this seems fairly easy and cheap.
>
> Or maybe you were referring to the fact that some attributes are
> mutually exclusive per Documentation/hwmon/sysfs-interface (in
> particular, general alarm flag vs. limit-specific alarm flags.) It was
> probably a bad idea to rely on this in the first place anyway, so I
> have no objection in changing this.
>
Yes, correct, though I had mostly the power sensors in mind if I recall correctly.
> [1] Note that the overflows couldn't really happen, BTW, as we have
> code to catch them in get_sensor_limit_data(). Maybe these checks can
> go away if the array sizes are now guaranteed to be sufficient by
> construction?
>
> This should really be a separate patch, BTW, as this is not related to
> the original purpose of the patch.
>
Sure, no problem.
> > Index: doc/libsensors-API.txt
> > ===================================================================
> > --- doc/libsensors-API.txt (revision 5996)
> > +++ doc/libsensors-API.txt (working copy)
> > @@ -7,6 +7,16 @@
> > given new feature.
> >
> > 0x431 lm-sensors 3.3.0 to 3.3.1
> > +* Added support for new sysfs attributes
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_AVERAGE
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_LOWEST
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_IN_HIGHEST
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_AVERAGE
>
> I don't see this one in Documentation/hwmon/sysfs-interface.
>
Me not either, nor am I aware of any sensors implementing it.
No idea where I got that idea from.
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_LOWEST
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_TEMP_HIGHEST
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_AVERAGE
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_LOWEST
> > + enum sensors_subfeature_type SENSORS_SUBFEATURE_CURR_HIGHEST
> > * Added support for intrusion detection
> > enum sensors_feature_type SENSORS_FEATURE_INTRUSION
> > enum sensors_subfeature_type SENSORS_SUBFEATURE_INTRUSION_ALARM
> > Index: lib/sensors.h
> > ===================================================================
> > --- lib/sensors.h (revision 5996)
> > +++ lib/sensors.h (working copy)
> > @@ -157,6 +157,9 @@
> > SENSORS_SUBFEATURE_IN_MAX,
> > SENSORS_SUBFEATURE_IN_LCRIT,
> > SENSORS_SUBFEATURE_IN_CRIT,
> > + SENSORS_SUBFEATURE_IN_AVERAGE,
> > + SENSORS_SUBFEATURE_IN_LOWEST,
> > + SENSORS_SUBFEATURE_IN_HIGHEST,
> > SENSORS_SUBFEATURE_IN_ALARM = (SENSORS_FEATURE_IN << 8) | 0x80,
> > SENSORS_SUBFEATURE_IN_MIN_ALARM,
> > SENSORS_SUBFEATURE_IN_MAX_ALARM,
> > @@ -181,6 +184,9 @@
> > SENSORS_SUBFEATURE_TEMP_LCRIT,
> > SENSORS_SUBFEATURE_TEMP_EMERGENCY,
> > SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST,
> > + SENSORS_SUBFEATURE_TEMP_AVERAGE,
> > + SENSORS_SUBFEATURE_TEMP_LOWEST,
> > + SENSORS_SUBFEATURE_TEMP_HIGHEST,
> > SENSORS_SUBFEATURE_TEMP_ALARM = (SENSORS_FEATURE_TEMP << 8) | 0x80,
> > SENSORS_SUBFEATURE_TEMP_MAX_ALARM,
> > SENSORS_SUBFEATURE_TEMP_MIN_ALARM,
> > @@ -215,6 +221,9 @@
> > SENSORS_SUBFEATURE_CURR_MAX,
> > SENSORS_SUBFEATURE_CURR_LCRIT,
> > SENSORS_SUBFEATURE_CURR_CRIT,
> > + SENSORS_SUBFEATURE_CURR_AVERAGE,
> > + SENSORS_SUBFEATURE_CURR_LOWEST,
> > + SENSORS_SUBFEATURE_CURR_HIGHEST,
> > SENSORS_SUBFEATURE_CURR_ALARM = (SENSORS_FEATURE_CURR << 8) | 0x80,
> > SENSORS_SUBFEATURE_CURR_MIN_ALARM,
> > SENSORS_SUBFEATURE_CURR_MAX_ALARM,
> > Index: lib/sysfs.c
> > ===================================================================
> > --- lib/sysfs.c (revision 5996)
> > +++ lib/sysfs.c (working copy)
> > @@ -233,6 +233,9 @@
> > { "lcrit", SENSORS_SUBFEATURE_TEMP_LCRIT },
> > { "emergency", SENSORS_SUBFEATURE_TEMP_EMERGENCY },
> > { "emergency_hyst", SENSORS_SUBFEATURE_TEMP_EMERGENCY_HYST },
> > + { "average", SENSORS_SUBFEATURE_TEMP_AVERAGE },
> > + { "lowest", SENSORS_SUBFEATURE_TEMP_LOWEST },
> > + { "highest", SENSORS_SUBFEATURE_TEMP_HIGHEST },
> > { "alarm", SENSORS_SUBFEATURE_TEMP_ALARM },
> > { "min_alarm", SENSORS_SUBFEATURE_TEMP_MIN_ALARM },
> > { "max_alarm", SENSORS_SUBFEATURE_TEMP_MAX_ALARM },
> > @@ -252,6 +255,9 @@
> > { "max", SENSORS_SUBFEATURE_IN_MAX },
> > { "lcrit", SENSORS_SUBFEATURE_IN_LCRIT },
> > { "crit", SENSORS_SUBFEATURE_IN_CRIT },
> > + { "average", SENSORS_SUBFEATURE_IN_AVERAGE },
> > + { "lowest", SENSORS_SUBFEATURE_IN_LOWEST },
> > + { "highest", SENSORS_SUBFEATURE_IN_HIGHEST },
> > { "alarm", SENSORS_SUBFEATURE_IN_ALARM },
> > { "min_alarm", SENSORS_SUBFEATURE_IN_MIN_ALARM },
> > { "max_alarm", SENSORS_SUBFEATURE_IN_MAX_ALARM },
> > @@ -302,6 +308,9 @@
> > { "max", SENSORS_SUBFEATURE_CURR_MAX },
> > { "lcrit", SENSORS_SUBFEATURE_CURR_LCRIT },
> > { "crit", SENSORS_SUBFEATURE_CURR_CRIT },
> > + { "average", SENSORS_SUBFEATURE_CURR_AVERAGE },
> > + { "lowest", SENSORS_SUBFEATURE_CURR_LOWEST },
> > + { "highest", SENSORS_SUBFEATURE_CURR_HIGHEST },
> > { "alarm", SENSORS_SUBFEATURE_CURR_ALARM },
> > { "min_alarm", SENSORS_SUBFEATURE_CURR_MIN_ALARM },
> > { "max_alarm", SENSORS_SUBFEATURE_CURR_MAX_ALARM },
> > Index: prog/sensors/chips.c
> > ===================================================================
> > --- prog/sensors/chips.c (revision 5996)
> > +++ prog/sensors/chips.c (working copy)
> > @@ -280,15 +280,26 @@
> > { SENSORS_SUBFEATURE_TEMP_CRIT, temp_crit_sensors, 0, "crit" },
> > { SENSORS_SUBFEATURE_TEMP_EMERGENCY, temp_emergency_sensors, 0,
> > "emerg" },
> > + { SENSORS_SUBFEATURE_TEMP_AVERAGE, NULL, 0, "avg" },
> > + { SENSORS_SUBFEATURE_TEMP_LOWEST, NULL, 0, "lowest" },
> > + { SENSORS_SUBFEATURE_TEMP_HIGHEST, NULL, 0, "highest" },
> > { -1, NULL, 0, NULL }
> > };
> >
> > +#define NUM_TEMP_ALARMS 6
> > +#define NUM_TEMP_SENSORS (ARRAY_SIZE(temp_sensors) \
> > + + ARRAY_SIZE(temp_max_sensors) \
> > + + ARRAY_SIZE(temp_crit_sensors) \
> > + + ARRAY_SIZE(temp_emergency_sensors) \
> > + - NUM_TEMP_ALARMS)
> > +
> > +
> > static void print_chip_temp(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > {
> > - struct sensor_subfeature_data sensors[8];
> > - struct sensor_subfeature_data alarms[5];
> > + struct sensor_subfeature_data sensors[NUM_TEMP_SENSORS];
> > + struct sensor_subfeature_data alarms[NUM_TEMP_ALARMS];
> > int sensor_count, alarm_count;
> > const sensors_subfeature *sf;
> > double val;
> > @@ -365,17 +376,23 @@
> > { SENSORS_SUBFEATURE_IN_MIN, NULL, 0, "min" },
> > { SENSORS_SUBFEATURE_IN_MAX, NULL, 0, "max" },
> > { SENSORS_SUBFEATURE_IN_CRIT, NULL, 0, "crit max" },
> > + { SENSORS_SUBFEATURE_IN_AVERAGE, NULL, 0, "avg" },
> > + { SENSORS_SUBFEATURE_IN_LOWEST, NULL, 0, "lowest" },
> > + { SENSORS_SUBFEATURE_IN_HIGHEST, NULL, 0, "highest" },
> > { -1, NULL, 0, NULL }
> > };
> >
> > +#define NUM_IN_ALARMS 5
> > +#define NUM_IN_SENSORS (ARRAY_SIZE(voltage_sensors) - NUM_IN_ALARMS)
> > +
> > static void print_chip_in(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > {
> > const sensors_subfeature *sf;
> > char *label;
> > - struct sensor_subfeature_data sensors[4];
> > - struct sensor_subfeature_data alarms[4];
> > + struct sensor_subfeature_data sensors[NUM_IN_SENSORS];
> > + struct sensor_subfeature_data alarms[NUM_IN_ALARMS];
> > int sensor_count, alarm_count;
> > double val;
> >
> > @@ -504,6 +521,7 @@
> > };
> >
> > static const struct sensor_subfeature_list power_inst_sensors[] = {
> > + { SENSORS_SUBFEATURE_POWER_AVERAGE, NULL, 0, "avg" },
>
> I'm confused. This one was missing on purpose. For power sensors we
> consider instantaneous and averaging sensors as different types. The
> code in function print_chip_power is pretty clear about this. So, just
> as all _INPUT subfeatures aren't listed in limit arrays,
> SENSORS_SUBFEATURE_POWER_AVERAGE must not be listed.
>
LM25066 reports instantaneous, average, and peak power, and I wanted to be able
to display it all.
> > { SENSORS_SUBFEATURE_POWER_INPUT_LOWEST, NULL, 0, "lowest" },
> > { SENSORS_SUBFEATURE_POWER_INPUT_HIGHEST, NULL, 0, "highest" },
> > { -1, NULL, 0, NULL }
> > @@ -517,14 +535,20 @@
> > { -1, NULL, 0, NULL }
> > };
> >
> > +#define NUM_POWER_ALARMS 4
> > +#define NUM_POWER_SENSORS (ARRAY_SIZE(power_common_sensors) \
> > + + ARRAY_SIZE(power_inst_sensors) \
> > + + ARRAY_SIZE(power_avg_sensors) \
> > + - NUM_POWER_ALARMS)
>
> Due to the specificity of the power sensors described above, the
> computed value is larger than needed. We'll use power_inst_sensors or
> power_avg_sensors but never both. You could define a MAX() macro to
> select the right one for to compute NUM_POWER_SENSORS.
>
Yes, I know, thus the comment at the very beginning.
> If you want to change the code to display both types, this can be
> discussed, but please keep the definition of NUM_POWER_SENSORS in line
> with the code that uses it, for clarity.
>
> > +
> > static void print_chip_power(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > {
> > double val;
> > const sensors_subfeature *sf;
> > - struct sensor_subfeature_data sensors[6];
> > - struct sensor_subfeature_data alarms[3];
> > + struct sensor_subfeature_data sensors[NUM_POWER_SENSORS];
> > + struct sensor_subfeature_data alarms[NUM_POWER_ALARMS];
> > int sensor_count, alarm_count;
> > char *label;
> > const char *unit;
> > @@ -653,9 +677,15 @@
> > { SENSORS_SUBFEATURE_CURR_MIN, NULL, 0, "min" },
> > { SENSORS_SUBFEATURE_CURR_MAX, NULL, 0, "max" },
> > { SENSORS_SUBFEATURE_CURR_CRIT, NULL, 0, "crit max" },
> > + { SENSORS_SUBFEATURE_CURR_AVERAGE, NULL, 0, "avg" },
> > + { SENSORS_SUBFEATURE_CURR_LOWEST, NULL, 0, "lowest" },
> > + { SENSORS_SUBFEATURE_CURR_HIGHEST, NULL, 0, "highest" },
> > { -1, NULL, 0, NULL }
> > };
> >
> > +#define NUM_CURR_ALARM 5
> > +#define NUM_CURR_SENSORS (ARRAY_SIZE(current_sensors) - NUM_CURR_ALARM)
> > +
> > static void print_chip_curr(const sensors_chip_name *name,
> > const sensors_feature *feature,
> > int label_size)
> > @@ -663,8 +693,8 @@
> > const sensors_subfeature *sf;
> > double val;
> > char *label;
> > - struct sensor_subfeature_data sensors[4];
> > - struct sensor_subfeature_data alarms[4];
> > + struct sensor_subfeature_data sensors[NUM_CURR_SENSORS];
> > + struct sensor_subfeature_data alarms[NUM_CURR_ALARM];
> > int sensor_count, alarm_count;
> >
> > if (!(label = sensors_get_label(name, feature))) {
> > Index: CHANGES
> > ===================================================================
> > --- CHANGES (revision 5996)
> > +++ CHANGES (working copy)
> > @@ -2,6 +2,10 @@
> > -----------------------
> >
> > SVN HEAD
> > + libsensors: Added support for new sysfs attributes
> > + sensors: Added support for new sysfs attributes
> > + For power sensors, display both instantaneous and average data
> > + if available
>
> Your patch doesn't actually implement that second change. Which is
> good, BTW, this unrelated change would really belong to a separate
> patch.
>
Hmm - I thought it did. I'll take it out for now and have another look.
Thanks a lot for the review.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
