PATCH:

Here is a patch adding some text to the sysfs interface documentation on how settings written to sysfs attributes should be handled, focussing mainly on error handling. This version incorperates Jean's latest comments.

Signed-off-by [email protected]

Hi Hans,

Quoted 4 lines · Hans de Goede

Here is a patch adding some text to the sysfs interface documentation on how settings written to sysfs attributes should be handled, focussing mainly on error handling. This version incorperates Jean's latest comments.

Quoted 1 line

Signed-off-by [email protected]

Thanks for doing this.

Acked-by Jean Delvare <[email protected]>

+/* write v to register */
+--- end code ---

Hi Hans,

Quoted 11 lines · Jean Delvare

Hi Hans,

Quoted 5 lines · Hans de Goede

Here is a patch adding some text to the sysfs interface documentation on how settings written to sysfs attributes should be handled, focussing mainly on error handling. This version incorperates Jean's latest comments.

Signed-off-by [email protected]

Thanks for doing this.

Acked-by Jean Delvare <[email protected]>

While reviewing your new fschmd driver, I found some mistakes which also appear in the code examples of this documentation update:

Documentation/hwmon/sysfs-interface line 448

Quoted 3 lines

+--- begin code ---

Documentation/hwmon/sysfs-interface renamed from Documentation.orig/hwmon/sysfs-interface +2 -0

+long v = simple_strtol(buf, NULL, 10) / 1000;
+SENSORS_LIMIT(v, -128, 127);

This is a no-op, should be v = ...

Documentation/hwmon/sysfs-interface line 462

Quoted 13 lines

Documentation/hwmon/sysfs-interface renamed from Documentation.orig/hwmon/sysfs-interface +12 -0

+/* write v to register */
+--- end code ---
 
+--- begin code ---
+unsigned long v = simple_strtoul(buf, NULL, 10);
+
+switch (v) {
+       case 2: v = 1; break;
+       case 4: v = 2; break;
+       case 8: v = 3; break;
+       default:
+               return -EINVAL;
+}

Indentation doesn't comply with CodingStyle.

Quoted 2 lines

+/* write v to register */
+--- end code ---

Additionally, I think that the examples are a bit difficult to read, one level of indentation would probably be better than the begin/end code markers. Thus, I propose the following incremental patch:

 Documentation/hwmon/sysfs-interface |   30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

Documentation/hwmon/sysfs-interface +4 -16

@@ -450,22 +450,20 @@ continuous like for example a tempX_type
 written, -EINVAL should be returned.
 
 Example1, temp1_max, register is a signed 8 bit value (-128 - 127 degrees):
---- begin code ---
-long v = simple_strtol(buf, NULL, 10) / 1000;
-SENSORS_LIMIT(v, -128, 127);
-/* write v to register */
---- end code ---
+
+	long v = simple_strtol(buf, NULL, 10) / 1000;
+	v = SENSORS_LIMIT(v, -128, 127);
+	/* write v to register */
 
 Example2, fan divider setting, valid values 2, 4 and 8:
---- begin code ---
-unsigned long v = simple_strtoul(buf, NULL, 10);
 
-switch (v) {
-       case 2: v = 1; break;
-       case 4: v = 2; break;
-       case 8: v = 3; break;
-       default:
-               return -EINVAL;
-}
-/* write v to register */
---- end code ---

Hi Hans,

Quoted 4 lines · Hans de Goede

Here is a patch adding some text to the sysfs interface documentation on how settings written to sysfs attributes should be handled, focussing mainly on error handling. This version incorperates Jean's latest comments.

Quoted 1 line

Signed-off-by [email protected]

Thanks for doing this.

Acked-by Jean Delvare <[email protected]>

Quoted 6 lines · Jean Delvare

Additionally, I think that the examples are a bit difficult to read, one level of indentation would probably be better than the begin/end code markers. Thus, I propose the following incremental patch:

 Documentation/hwmon/sysfs-interface |   30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

I agree with all changes, Mark pleaee apply:

Acked-by Hans de Goede <[email protected]>

Notice that this is what happens when you make a function look like a macro by giving it all caps names. Note: I'm not trying to talk my mistake right, just trying to explain how I came to the mistake. Maybe we should rename SENSORS_LIMIT?

Although, I suppose it would be easier for you and maybe Jean if I just publish a patch series myself. It shouldn't be too hard to script that up; I'll look into it.

Hi All,

As discussed on the lm-sensors list, using the fixed scaling factors from the fscpos datasheet for the fscher and newer models as wel, was nood a good idea, as the actual scaling factors may be different and can be read from OEM DMI entries.

This patch changes the behaviour of the merged FSC familiy fschmd driver to be the same as the old standalone fscher driver for fscher and newer models, leaving the scaling to userspace.

This also preserves compatibility of existing sensors.conf files.

Signed-of-by Hans de Goede <[email protected]>

---

Jean, can you review this please?
Mark, can you send this to Linus for 2.6.24 please, otherwise we will have one official kernel release with different scaling for the voltage inputs of the fschmd compared to the next releases.

And yes I tested my patch this time.

Quoted 2 lines · Jean Delvare

Assuming that you definitely excluded the possibility to retrieve the DMI data from the kernel

Definitely is a big word, I've looked at the DMI parser currently in the kernel, tried to come up with a way to get the info there which would be more generic then just being an fschmd specific hack and couldn't. Then I slept a night on it and still couldn't come up with something clean, esp since there are many type 185 entries in the DMI table of FSC machines, of which we need only one.

Hi Hans,

Quoted 9 lines · Hans de Goede

Quoted 2 lines · Jean Delvare

Assuming that you definitely excluded the possibility to retrieve the DMI data from the kernel

Definitely is a big word, I've looked at the DMI parser currently in the kernel, tried to come up with a way to get the info there which would be more generic then just being an fschmd specific hack and couldn't. Then I slept a night on it and still couldn't come up with something clean, esp since there are many type 185 entries in the DMI table of FSC machines, of which we need only one.

Please take a look at this completely untested patch. It might need some cleanups before it's ready for submission, but is there any reason why something like this wouldn't do the job?

Good work Jean, yes that should do the job, I stared myself blind at adding a clean way to store the data for later retrieval to the existing __init parser, your way indeed will work and is much more generic.

I'll compile a kernel with the dmi_scan.c and dmi.h parts patched in and then start working on integrating this into fschmd.c

Question, what do we do if the dmi data doesn't get found, use some defaults I guess?

Quoted 4 lines · Hans de Goede

Good work Jean, yes that should do the job, I stared myself blind at adding a clean way to store the data for later retrieval to the existing __init parser, your way indeed will work and is much more generic.

Quoted 2 lines

I'll compile a kernel with the dmi_scan.c and dmi.h parts patched in and then start working on integrating this into fschmd.c

Here's an updated version of my patch, with no code duplication this time. This should be easier to get this accepted upstream. To make the code even smaller, dmi_table() could be merged into dmi_present() but that's about it.

Quoted 2 lines

Question, what do we do if the dmi data doesn't get found, use some defaults I guess?

Don't export the voltage values at all? I'd rather not export anything than silently export values which may not be correct. But hopefully it will never happen so it doesn't really matter what we decide here.

* * * * *

Subject: dmi: Let drivers walk the DMI table

Let drivers walk the DMI table for their own needs. Some drivers need data stored in OEM-specific DMI records for proper operation.

Signed-off-by Jean Delvare <[email protected]>

---
 drivers/firmware/dmi_scan.c |   59 +++++++++++++++++++++++++++++++++----------
 include/linux/dmi.h         |    3 ++
 2 files changed, 49 insertions(+), 13 deletions(-)

drivers/firmware/dmi_scan.c +46 -13

@@ -36,18 +36,12 @@ static char * __init dmi_string(const st
  *	We have to be cautious here. We have seen BIOSes with DMI pointers
  *	pointing to completely the wrong place for example
  */
-static int __init dmi_table(u32 base, int len, int num,
-			    void (*decode)(const struct dmi_header *))
+static void __dmi_table(u8 *buf, int len, int num,
+			void (*decode)(const struct dmi_header *))
 {
-	u8 *buf, *data;
+	u8 *data = buf;
 	int i = 0;
 
-	buf = dmi_ioremap(base, len);
-	if (buf = NULL)
-		return -1;
-
-	data = buf;
-
 	/*
 	 *	Stop when we see all the items the table claimed to have
 	 *	OR we run off the end of the table (also happens)

@@ -68,6 +62,19 @@ static int __init dmi_table(u32 base, in
 		data += 2;
 		i++;
 	}
+}
+
+static int __init dmi_table(u32 base, int len, int num,
+			    void (*decode)(const struct dmi_header *))
+{
+	u8 *buf;
+
+	buf = dmi_ioremap(base, len);
+	if (buf = NULL)
+		return -1;
+
+	__dmi_table(buf, len, num, decode);
+
 	dmi_iounmap(buf, len);
 	return 0;
 }

@@ -86,6 +93,9 @@ static int __init dmi_checksum(const u8 
 static char *dmi_ident[DMI_STRING_MAX];
 static LIST_HEAD(dmi_devices);
 int dmi_available;
+static u32 dmi_base;
+static u16 dmi_len;
+static u16 dmi_num;
 
 /*
  *	Save a DMI string

@@ -273,9 +283,9 @@ static int __init dmi_present(const char
 
 	memcpy_fromio(buf, p, 15);
 	if ((memcmp(buf, "_DMI_", 5) = 0) && dmi_checksum(buf)) {
-		u16 num = (buf[13] << 8) | buf[12];
-		u16 len = (buf[7] << 8) | buf[6];
-		u32 base = (buf[11] << 24) | (buf[10] << 16) |
+		dmi_num = (buf[13] << 8) | buf[12];
+		dmi_len = (buf[7] << 8) | buf[6];
+		dmi_base = (buf[11] << 24) | (buf[10] << 16) |
 			(buf[9] << 8) | buf[8];
 
 		/*

@@ -287,7 +297,7 @@ static int __init dmi_present(const char
 			       buf[14] >> 4, buf[14] & 0xF);
 		else
 			printk(KERN_INFO "DMI present.\n");
-		if (dmi_table(base,len, num, dmi_decode) = 0)
+		if (dmi_table(dmi_base, dmi_len, dmi_num, dmi_decode) = 0)
 			return 0;
 	}
 	return 1;

@@ -470,3 +480,26 @@ int dmi_get_year(int field)
 	return year;
 }
 
+/**
+ *	dmi_walk - Walk the DMI table and get called back for every record
+ *	@decode: Callback function
+ *
+ *	Returns -1 when the DMI table can't be reached, 0 on success.
+ */
+int dmi_walk(void (*decode)(const struct dmi_header *))
+{
+	u8 *buf;
+
+	if (!dmi_available)
+		return -1;
+
+	buf = ioremap(dmi_base, dmi_len);
+	if (buf = NULL)
+		return -1;
+
+	__dmi_table(buf, dmi_len, dmi_num, decode);
+
+	iounmap(buf);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dmi_walk);

include/linux/dmi.h +1 -0

@@ -78,6 +78,7 @@ extern const struct dmi_device * dmi_fin
 extern void dmi_scan_machine(void);
 extern int dmi_get_year(int field);
 extern int dmi_name_in_vendors(const char *str);
+extern int dmi_walk(void (*decode)(const struct dmi_header *));
 
 #else
 

Quoted 5 lines · Jean Delvare

Quoted 4 lines · Hans de Goede

Good work Jean, yes that should do the job, I stared myself blind at adding a clean way to store the data for later retrieval to the existing __init parser, your way indeed will work and is much more generic.

Quoted 7 lines

Quoted 2 lines

I'll compile a kernel with the dmi_scan.c and dmi.h parts patched in and then start working on integrating this into fschmd.c

Here's an updated version of my patch, with no code duplication this time. This should be easier to get this accepted upstream. To make the code even smaller, dmi_table() could be merged into dmi_present() but that's about it.

Looks good, I found inconsistency though, in essence the dmi_table() and dmi_walk() are the same, (except for dmi_ioremap versus ioremap ofcourse) but for one you just use the base, len and number of items stored in global variables, while for the other you pass those same global variables through parameters, for consistency sake I think you should to both the same.

Quoted 7 lines

Quoted 2 lines

Question, what do we do if the dmi data doesn't get found, use some defaults I guess?

Don't export the voltage values at all? I'd rather not export anything than silently export values which may not be correct. But hopefully it will never happen so it doesn't really matter what we decide here.

I would rather just printk a warning and use some sane defaults in this scenario. We must definetively alert the user, that I agree with. The problem with a printk is ofcourse that it will hardly be noticed.

Hi Hans,

Quoted 12 lines · Hans de Goede

Quoted 11 lines · Jean Delvare

Quoted 6 lines · Hans de Goede

Good work Jean, yes that should do the job, I stared myself blind at adding a clean way to store the data for later retrieval to the existing __init parser, your way indeed will work and is much more generic.

I'll compile a kernel with the dmi_scan.c and dmi.h parts patched in and then start working on integrating this into fschmd.c

Here's an updated version of my patch, with no code duplication this time. This should be easier to get this accepted upstream. To make the code even smaller, dmi_table() could be merged into dmi_present() but that's about it.

Quoted 5 lines

Looks good, I found inconsistency though, in essence the dmi_table() and dmi_walk() are the same, (except for dmi_ioremap versus ioremap ofcourse) but for one you just use the base, len and number of items stored in global variables, while for the other you pass those same global variables through parameters, for consistency sake I think you should to both the same.

That's an asymmetry rather than an inconsistency. There are two reasons for it, firstly I avoid using global variables when I don't have to, secondly this happened to make the patch smaller. It is of course possible to use the global variables dmi_base, dmi_len and dmi_num in dmi_table() rather than passing them as parameters; doing so makes the binary code slightly smaller. I'm attaching a version of the patch that does this, together with function renames to make it look better. If you like this one, I could send it upstream for review and comments.

Subject: dmi: Let drivers walk the DMI table

Let drivers walk the DMI table for their own needs. Some drivers need data stored in OEM-specific DMI records for proper operation.

Signed-off-by Jean Delvare <[email protected]>

---
 drivers/firmware/dmi_scan.c |   61 +++++++++++++++++++++++++++++++++----------
 include/linux/dmi.h         |    3 ++
 2 files changed, 50 insertions(+), 14 deletions(-)

drivers/firmware/dmi_scan.c +47 -14

@@ -36,18 +36,12 @@ static char * __init dmi_string(const st
  *	We have to be cautious here. We have seen BIOSes with DMI pointers
  *	pointing to completely the wrong place for example
  */
-static int __init dmi_table(u32 base, int len, int num,
-			    void (*decode)(const struct dmi_header *))
+static void dmi_table(u8 *buf, int len, int num,
+		      void (*decode)(const struct dmi_header *))
 {
-	u8 *buf, *data;
+	u8 *data = buf;
 	int i = 0;
 
-	buf = dmi_ioremap(base, len);
-	if (buf = NULL)
-		return -1;
-
-	data = buf;
-
 	/*
 	 *	Stop when we see all the items the table claimed to have
 	 *	OR we run off the end of the table (also happens)

@@ -68,7 +62,23 @@ static int __init dmi_table(u32 base, in
 		data += 2;
 		i++;
 	}
-	dmi_iounmap(buf, len);
+}
+
+static u32 dmi_base;
+static u16 dmi_len;
+static u16 dmi_num;
+
+static int __init dmi_walk_early(void (*decode)(const struct dmi_header *))
+{
+	u8 *buf;
+
+	buf = dmi_ioremap(dmi_base, dmi_len);
+	if (buf = NULL)
+		return -1;
+
+	dmi_table(buf, dmi_len, dmi_num, decode);
+
+	dmi_iounmap(buf, dmi_len);
 	return 0;
 }
 

@@ -273,9 +283,9 @@ static int __init dmi_present(const char
 
 	memcpy_fromio(buf, p, 15);
 	if ((memcmp(buf, "_DMI_", 5) = 0) && dmi_checksum(buf)) {
-		u16 num = (buf[13] << 8) | buf[12];
-		u16 len = (buf[7] << 8) | buf[6];
-		u32 base = (buf[11] << 24) | (buf[10] << 16) |
+		dmi_num = (buf[13] << 8) | buf[12];
+		dmi_len = (buf[7] << 8) | buf[6];
+		dmi_base = (buf[11] << 24) | (buf[10] << 16) |
 			(buf[9] << 8) | buf[8];
 
 		/*

@@ -287,7 +297,7 @@ static int __init dmi_present(const char
 			       buf[14] >> 4, buf[14] & 0xF);
 		else
 			printk(KERN_INFO "DMI present.\n");
-		if (dmi_table(base,len, num, dmi_decode) = 0)
+		if (dmi_walk_early(dmi_decode) = 0)
 			return 0;
 	}
 	return 1;

@@ -470,3 +480,26 @@ int dmi_get_year(int field)
 	return year;
 }
 
+/**
+ *	dmi_walk - Walk the DMI table and get called back for every record
+ *	@decode: Callback function
+ *
+ *	Returns -1 when the DMI table can't be reached, 0 on success.
+ */
+int dmi_walk(void (*decode)(const struct dmi_header *))
+{
+	u8 *buf;
+
+	if (!dmi_available)
+		return -1;
+
+	buf = ioremap(dmi_base, dmi_len);
+	if (buf = NULL)
+		return -1;
+
+	dmi_table(buf, dmi_len, dmi_num, decode);
+
+	iounmap(buf);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dmi_walk);

include/linux/dmi.h +1 -0

@@ -78,6 +78,7 @@ extern const struct dmi_device * dmi_fin
 extern void dmi_scan_machine(void);
 extern int dmi_get_year(int field);
 extern int dmi_name_in_vendors(const char *str);
+extern int dmi_walk(void (*decode)(const struct dmi_header *));
 
 #else
 

I finally found / made some time to try out using your below patch in the fschmd driver. Your patch works as advertised, I'll send a mail with a patch to the fschmd driver using its functionality right after this mail. I guess this is 2.6.25 material and that we should queue both patches for 2.6.25.

Hi Jean,

<Trying to win the competition for the patch with the longest name :) >

This patch fixes a number of cases where things were not properly cleaned up when acpi_check_resource_conflict() returned an error, causing oopses such as the one reported here:
https://bugzilla.redhat.com/show_bug.cgi?idH3208

Signed-off-by Hans de Goede <[email protected]>