[lm-sensors] [PATCH] libsensors: Compute MAX_SUBFEATURES dynamically

[lm-sensors] [PATCH] libsensors: Compute MAX_SUBFEATURES dynamically

[Jean Delvare]

Reference: http://www.lm-sensors.org/ticket/2378

This is a candidate patch to let libsensors compute MAX_SUBFEATURES
dynamically. This should avoid accidental overflows when we add new
subfeatures.

An alternative is to keep it a constant and add code to check for
overflows. I had a patch ready, but if we are going to add code, I'd
rather add code that get things right than code which only spots when
things are wrong.

Then we can discuss what do to with the other constants.

MAX_MAIN_SENSOR_TYPES and MAX_OTHER_SENSOR_TYPES could be easily
computed in the same loops which now compute MAX_SUBFEATURES. It's only
a few code lines to add. OTOH it can be discussed whether they are
worth the runtime cost, given that adding a new feature is a rare
event, so we should be able to deal with it. I would like to hear
opinions about this.

Lastly, making MAX_SENSORS_PER_TYPE dynamic would be very nice, as it
would avoid allocating more memory than we need (and supporting
virtually unlimited channel numbers, if people have really big sensor
chips.) But this means reworking the discovery loop significantly, as
we would need a first pass to find out the maximum channel number. This
will come at a runtime cost which we want to minimize. And I do not
have the time to work on this at the moment.

---
 lib/sysfs.c |   36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

--- lm-sensors.orig/lib/sysfs.c	2010-12-13 10:50:42.000000000 +0100
+++ lm-sensors/lib/sysfs.c	2010-12-13 11:40:05.000000000 +0100
@@ -139,8 +139,8 @@ char sensors_sysfs_mount[NAME_MAX];
 #define MAX_MAIN_SENSOR_TYPES	6
 #define MAX_OTHER_SENSOR_TYPES	2
 #define MAX_SENSORS_PER_TYPE	24
-#define MAX_SUBFEATURES		8
-#define FEATURE_SIZE		(MAX_SUBFEATURES * 2)
+/* max_subfeatures is now computed dynamically */
+#define FEATURE_SIZE		(max_subfeatures * 2)
 #define FEATURE_TYPE_SIZE	(MAX_SENSORS_PER_TYPE * FEATURE_SIZE)
 
 /* Room for all 6 main types (in, fan, temp, power, energy, current) and 2
@@ -337,6 +337,31 @@ sensors_subfeature_type sensors_subfeatu
 	return SENSORS_SUBFEATURE_UNKNOWN;
 }
 
+static void sensors_compute_max(int *max_subfeatures)
+{
+	int i, j, offset;
+	const struct subfeature_type_match *submatches;
+	sensors_feature_type ftype;
+
+	for (i = 0; i < ARRAY_SIZE(matches); i++) {
+		submatches = matches[i].submatches;
+		for (j = 0; submatches[j].name != NULL; j++) {
+			ftype = submatches[j].type >> 8;
+
+			if (ftype < SENSORS_FEATURE_VID) {
+				offset = submatches[j].type & 0x7F;
+				if (offset >= *max_subfeatures)
+					*max_subfeatures = offset + 1;
+			} else {
+				offset = submatches[j].type & 0xFF;
+				if (offset >= *max_subfeatures * 2)
+					*max_subfeatures = ((offset + 1) + 1)
+							   / 2;
+			}
+		}
+	}
+}
+
 static int sensors_get_attr_mode(const char *device, const char *attr)
 {
 	char path[NAME_MAX];
@@ -357,6 +382,7 @@ static int sensors_read_dynamic_chip(sen
 				     const char *dev_path)
 {
 	int i, fnum = 0, sfnum = 0, prev_slot;
+	static int max_subfeatures;
 	DIR *dir;
 	struct dirent *ent;
 	sensors_subfeature *all_subfeatures;
@@ -368,6 +394,10 @@ static int sensors_read_dynamic_chip(sen
 	if (!(dir = opendir(dev_path)))
 		return -errno;
 
+	/* Dynamically figure out the max number of subfeatures */
+	if (!max_subfeatures)
+		sensors_compute_max(&max_subfeatures);
+
 	/* We use a large sparse table at first to store all found
 	   subfeatures, so that we can store them sorted at type and index
 	   and then later create a dense sorted table. */
@@ -430,7 +460,7 @@ static int sensors_read_dynamic_chip(sen
 		default:
 			i = ftype * FEATURE_TYPE_SIZE +
 			    nr * FEATURE_SIZE +
-			    ((sftype & 0x80) >> 7) * MAX_SUBFEATURES +
+			    ((sftype & 0x80) >> 7) * max_subfeatures +
 			    (sftype & 0x7F);
 		}
 


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] libsensors: Compute MAX_SUBFEATURES

[Guenter Roeck]

On Mon, Dec 13, 2010 at 07:58:42AM -0500, Jean Delvare wrote:
> Reference: http://www.lm-sensors.org/ticket/2378
> 
> This is a candidate patch to let libsensors compute MAX_SUBFEATURES
> dynamically. This should avoid accidental overflows when we add new
> subfeatures.
> 
> An alternative is to keep it a constant and add code to check for
> overflows. I had a patch ready, but if we are going to add code, I'd
> rather add code that get things right than code which only spots when
> things are wrong.
> 
> Then we can discuss what do to with the other constants.
> 
> MAX_MAIN_SENSOR_TYPES and MAX_OTHER_SENSOR_TYPES could be easily
> computed in the same loops which now compute MAX_SUBFEATURES. It's only
> a few code lines to add. OTOH it can be discussed whether they are
> worth the runtime cost, given that adding a new feature is a rare
> event, so we should be able to deal with it. I would like to hear
> opinions about this.
> 
Added runtime cost is really negligible, so that isn't really an argument for me.
But it is a really rare event, so it doesn't seem to be worth the effort - at least
as long as compilation fails if the limit is exceeded.

> Lastly, making MAX_SENSORS_PER_TYPE dynamic would be very nice, as it
> would avoid allocating more memory than we need (and supporting
> virtually unlimited channel numbers, if people have really big sensor
> chips.) But this means reworking the discovery loop significantly, as
> we would need a first pass to find out the maximum channel number. This
> will come at a runtime cost which we want to minimize. And I do not
> have the time to work on this at the moment.
> 
Not sure if that is really worth it. I would not do it.

> ---
>  lib/sysfs.c |   36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> --- lm-sensors.orig/lib/sysfs.c	2010-12-13 10:50:42.000000000 +0100
> +++ lm-sensors/lib/sysfs.c	2010-12-13 11:40:05.000000000 +0100
> @@ -139,8 +139,8 @@ char sensors_sysfs_mount[NAME_MAX];
>  #define MAX_MAIN_SENSOR_TYPES	6
>  #define MAX_OTHER_SENSOR_TYPES	2
>  #define MAX_SENSORS_PER_TYPE	24
> -#define MAX_SUBFEATURES		8
> -#define FEATURE_SIZE		(MAX_SUBFEATURES * 2)
> +/* max_subfeatures is now computed dynamically */
> +#define FEATURE_SIZE		(max_subfeatures * 2)
>  #define FEATURE_TYPE_SIZE	(MAX_SENSORS_PER_TYPE * FEATURE_SIZE)
>  
>  /* Room for all 6 main types (in, fan, temp, power, energy, current) and 2
> @@ -337,6 +337,31 @@ sensors_subfeature_type sensors_subfeatu
>  	return SENSORS_SUBFEATURE_UNKNOWN;
>  }
>  
> +static void sensors_compute_max(int *max_subfeatures)
> +{

Maybe personal preference, but I think it would be better (and a bit more efficient)
to return the result as function result, and not write it into a pointer.

Otherwise, code looks good.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] libsensors: Compute MAX_SUBFEATURES

[Jean Delvare]

On Mon, 13 Dec 2010 07:20:56 -0800, Guenter Roeck wrote:
> On Mon, Dec 13, 2010 at 07:58:42AM -0500, Jean Delvare wrote:
> > Reference: http://www.lm-sensors.org/ticket/2378
> > 
> > This is a candidate patch to let libsensors compute MAX_SUBFEATURES
> > dynamically. This should avoid accidental overflows when we add new
> > subfeatures.
> > 
> > An alternative is to keep it a constant and add code to check for
> > overflows. I had a patch ready, but if we are going to add code, I'd
> > rather add code that get things right than code which only spots when
> > things are wrong.
> > 
> > Then we can discuss what do to with the other constants.
> > 
> > MAX_MAIN_SENSOR_TYPES and MAX_OTHER_SENSOR_TYPES could be easily
> > computed in the same loops which now compute MAX_SUBFEATURES. It's only
> > a few code lines to add. OTOH it can be discussed whether they are
> > worth the runtime cost, given that adding a new feature is a rare
> > event, so we should be able to deal with it. I would like to hear
> > opinions about this.
> > 
> Added runtime cost is really negligible, so that isn't really an argument for me.
> But it is a really rare event, so it doesn't seem to be worth the effort - at least
> as long as compilation fails if the limit is exceeded.

Unfortunately, no, the compilation doesn't fail, and this is the
problem. A bug would lead to an overflow at run-time.

Adding a run-time check preventing the overflow (and basically ignoring
the new features) could be done. But then again, if we add code, we
might as well add code which computes things dynamically, rather than
code to work around our mistakes.

At the moment, when one adds a entry to enum sensors_feature_type in
sensors.h, he or she has to think of increasing MAX_MAIN_SENSOR_TYPES
or MAX_OTHER_SENSOR_TYPES in sysfs.c. We could put a note in sensors.h
about it, but OTOH I don't like to mention internal implementation
details in a public header file.

But looking at sensors.h... maybe I have an idea how to solve this
elegantly. Let me give it a try.

> > Lastly, making MAX_SENSORS_PER_TYPE dynamic would be very nice, as it
> > would avoid allocating more memory than we need (and supporting
> > virtually unlimited channel numbers, if people have really big sensor
> > chips.) But this means reworking the discovery loop significantly, as
> > we would need a first pass to find out the maximum channel number. This
> > will come at a runtime cost which we want to minimize. And I do not
> > have the time to work on this at the moment.
>
> Not sure if that is really worth it. I would not do it.

We had to increase MAX_SENSORS_PER_TYPE recently, for the W83785G/ADG.
Users of these chips have to use lm-sensors 3.2.0 or later for proper
support. We tried hard to make it possible to run old versions of
lm-sensors with recent hardware, and this is an example of failure.

It isn't possible to say if 24 will be OK in the future. 4 years ago I
would have laughed at you if you had claimed that devices supporting
more than 16 of a given sensor type would ever exist. To play it safe,
we would increase MAX_SENSORS_PER_TYPE to 32 or even 64, but then think
of the memory we allocate on all systems which don't need it. I know
the allocation is temporary, but still. Things get worse as we add new
sensor types and new subfeatures. While each device only has a limited
set of channels, the memory allocation we do keeps growing. I don't
like this feeling that we ask for more and more memory and we don't
really control it. Making MAX_SENSORS_PER_TYPE dynamic would be one
possible answer.

But again, this isn't trivial, and I don't have the time to do it now.
So for now I'll follow your advice ;)

> 
> > ---
> >  lib/sysfs.c |   36 +++++++++++++++++++++++++++++++++---
> >  1 file changed, 33 insertions(+), 3 deletions(-)
> > 
> > --- lm-sensors.orig/lib/sysfs.c	2010-12-13 10:50:42.000000000 +0100
> > +++ lm-sensors/lib/sysfs.c	2010-12-13 11:40:05.000000000 +0100
> > @@ -139,8 +139,8 @@ char sensors_sysfs_mount[NAME_MAX];
> >  #define MAX_MAIN_SENSOR_TYPES	6
> >  #define MAX_OTHER_SENSOR_TYPES	2
> >  #define MAX_SENSORS_PER_TYPE	24
> > -#define MAX_SUBFEATURES		8
> > -#define FEATURE_SIZE		(MAX_SUBFEATURES * 2)
> > +/* max_subfeatures is now computed dynamically */
> > +#define FEATURE_SIZE		(max_subfeatures * 2)
> >  #define FEATURE_TYPE_SIZE	(MAX_SENSORS_PER_TYPE * FEATURE_SIZE)
> >  
> >  /* Room for all 6 main types (in, fan, temp, power, energy, current) and 2
> > @@ -337,6 +337,31 @@ sensors_subfeature_type sensors_subfeatu
> >  	return SENSORS_SUBFEATURE_UNKNOWN;
> >  }
> >  
> > +static void sensors_compute_max(int *max_subfeatures)
> > +{
> 
> Maybe personal preference, but I think it would be better (and a bit more efficient)
> to return the result as function result, and not write it into a pointer.

My first attempt did exactly that. I changed my mind in preparation of
two additional pointers being passed for MAX_MAIN_SENSOR_TYPES and
MAX_OTHER_SENSOR_TYPES. But if this doesn't happen, I'll revert to the
original implementation which you'll prefer.

> Otherwise, code looks good.

Thanks for the review.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] libsensors: Compute MAX_SUBFEATURES

[Guenter Roeck]

On Mon, Dec 13, 2010 at 10:51:32AM -0500, Jean Delvare wrote:
> On Mon, 13 Dec 2010 07:20:56 -0800, Guenter Roeck wrote:
> > On Mon, Dec 13, 2010 at 07:58:42AM -0500, Jean Delvare wrote:
> > > Reference: http://www.lm-sensors.org/ticket/2378
> > > 
> > > This is a candidate patch to let libsensors compute MAX_SUBFEATURES
> > > dynamically. This should avoid accidental overflows when we add new
> > > subfeatures.
> > > 
> > > An alternative is to keep it a constant and add code to check for
> > > overflows. I had a patch ready, but if we are going to add code, I'd
> > > rather add code that get things right than code which only spots when
> > > things are wrong.
> > > 
> > > Then we can discuss what do to with the other constants.
> > > 
> > > MAX_MAIN_SENSOR_TYPES and MAX_OTHER_SENSOR_TYPES could be easily
> > > computed in the same loops which now compute MAX_SUBFEATURES. It's only
> > > a few code lines to add. OTOH it can be discussed whether they are
> > > worth the runtime cost, given that adding a new feature is a rare
> > > event, so we should be able to deal with it. I would like to hear
> > > opinions about this.
> > > 
> > Added runtime cost is really negligible, so that isn't really an argument for me.
> > But it is a really rare event, so it doesn't seem to be worth the effort - at least
> > as long as compilation fails if the limit is exceeded.
> 
> Unfortunately, no, the compilation doesn't fail, and this is the
> problem. A bug would lead to an overflow at run-time.
> 
> Adding a run-time check preventing the overflow (and basically ignoring
> the new features) could be done. But then again, if we add code, we
> might as well add code which computes things dynamically, rather than
> code to work around our mistakes.
> 
> At the moment, when one adds a entry to enum sensors_feature_type in
> sensors.h, he or she has to think of increasing MAX_MAIN_SENSOR_TYPES
> or MAX_OTHER_SENSOR_TYPES in sysfs.c. We could put a note in sensors.h
> about it, but OTOH I don't like to mention internal implementation
> details in a public header file.
> 
> But looking at sensors.h... maybe I have an idea how to solve this
> elegantly. Let me give it a try.
> 
If that doesn't work, or ends up being complicated, I'd suggest to go with
runtime detection after all. Or, in other words, KISS should be the deciding
factor.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] libsensors: Compute MAX_SUBFEATURES

[Jean Delvare]

On Mon, 13 Dec 2010 16:51:32 +0100, Jean Delvare wrote:
> But looking at sensors.h... maybe I have an idea how to solve this
> elegantly. Let me give it a try.

This is what I came up with. What do you think? This lets
MAX_MAIN_SENSOR_TYPES and MAX_OTHER_SENSOR_TYPES be computed at build
time automatically, with no additional cost at run-time. The only
drawback I see is that SENSORS_FEATURE_MAX_MAIN and
SENSORS_FEATURE_MAX_OTHER will be public, while we know their values
will change over time, by design. But I can't think of a valid reason
why an application would use them, so I'd say it is OK.

---
 lib/sensors.h |    2 ++
 lib/sysfs.c   |    4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

--- lm-sensors.orig/lib/sensors.h	2010-12-12 22:44:30.000000000 +0100
+++ lm-sensors/lib/sensors.h	2010-12-13 18:21:22.000000000 +0100
@@ -140,8 +140,10 @@ typedef enum sensors_feature_type {
 	SENSORS_FEATURE_POWER		= 0x03,
 	SENSORS_FEATURE_ENERGY		= 0x04,
 	SENSORS_FEATURE_CURR		= 0x05,
+	SENSORS_FEATURE_MAX_MAIN,
 	SENSORS_FEATURE_VID		= 0x10,
 	SENSORS_FEATURE_INTRUSION	= 0x11,
+	SENSORS_FEATURE_MAX_OTHER,
 	SENSORS_FEATURE_BEEP_ENABLE	= 0x18,
 	SENSORS_FEATURE_UNKNOWN		= INT_MAX,
 } sensors_feature_type;
--- lm-sensors.orig/lib/sysfs.c	2010-12-13 11:40:05.000000000 +0100
+++ lm-sensors/lib/sysfs.c	2010-12-13 18:22:20.000000000 +0100
@@ -136,8 +136,8 @@ static int sysfs_foreach_busdev(const ch
 
 char sensors_sysfs_mount[NAME_MAX];
 
-#define MAX_MAIN_SENSOR_TYPES	6
-#define MAX_OTHER_SENSOR_TYPES	2
+#define MAX_MAIN_SENSOR_TYPES	(SENSORS_FEATURE_MAX_MAIN - SENSORS_FEATURE_IN)
+#define MAX_OTHER_SENSOR_TYPES	(SENSORS_FEATURE_MAX_OTHER - SENSORS_FEATURE_VID)
 #define MAX_SENSORS_PER_TYPE	24
 /* max_subfeatures is now computed dynamically */
 #define FEATURE_SIZE		(max_subfeatures * 2)

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] libsensors: Compute MAX_SUBFEATURES

[Guenter Roeck]

On Mon, 2010-12-13 at 14:18 -0500, Jean Delvare wrote:
> On Mon, 13 Dec 2010 16:51:32 +0100, Jean Delvare wrote:
> > But looking at sensors.h... maybe I have an idea how to solve this
> > elegantly. Let me give it a try.
> 
> This is what I came up with. What do you think? This lets
> MAX_MAIN_SENSOR_TYPES and MAX_OTHER_SENSOR_TYPES be computed at build
> time automatically, with no additional cost at run-time. The only
> drawback I see is that SENSORS_FEATURE_MAX_MAIN and
> SENSORS_FEATURE_MAX_OTHER will be public, while we know their values
> will change over time, by design. But I can't think of a valid reason
> why an application would use them, so I'd say it is OK.
> 
> ---
>  lib/sensors.h |    2 ++
>  lib/sysfs.c   |    4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> --- lm-sensors.orig/lib/sensors.h	2010-12-12 22:44:30.000000000 +0100
> +++ lm-sensors/lib/sensors.h	2010-12-13 18:21:22.000000000 +0100
> @@ -140,8 +140,10 @@ typedef enum sensors_feature_type {
>  	SENSORS_FEATURE_POWER		= 0x03,
>  	SENSORS_FEATURE_ENERGY		= 0x04,
>  	SENSORS_FEATURE_CURR		= 0x05,
> +	SENSORS_FEATURE_MAX_MAIN,
>  	SENSORS_FEATURE_VID		= 0x10,
>  	SENSORS_FEATURE_INTRUSION	= 0x11,
> +	SENSORS_FEATURE_MAX_OTHER,
>  	SENSORS_FEATURE_BEEP_ENABLE	= 0x18,
>  	SENSORS_FEATURE_UNKNOWN		= INT_MAX,
>  } sensors_feature_type;
> --- lm-sensors.orig/lib/sysfs.c	2010-12-13 11:40:05.000000000 +0100
> +++ lm-sensors/lib/sysfs.c	2010-12-13 18:22:20.000000000 +0100
> @@ -136,8 +136,8 @@ static int sysfs_foreach_busdev(const ch
>  
>  char sensors_sysfs_mount[NAME_MAX];
>  
> -#define MAX_MAIN_SENSOR_TYPES	6
> -#define MAX_OTHER_SENSOR_TYPES	2
> +#define MAX_MAIN_SENSOR_TYPES	(SENSORS_FEATURE_MAX_MAIN - SENSORS_FEATURE_IN)
> +#define MAX_OTHER_SENSOR_TYPES	(SENSORS_FEATURE_MAX_OTHER - SENSORS_FEATURE_VID)
>  #define MAX_SENSORS_PER_TYPE	24
>  /* max_subfeatures is now computed dynamically */
>  #define FEATURE_SIZE		(max_subfeatures * 2)
> 
Simple enough. Looks ok to me.

Guenter




_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors