[PATCH] ibm-acpi-0.8 (was Re: 2.6.10-rc1-mm3)

[PATCH] ibm-acpi-0.8 (was Re: 2.6.10-rc1-mm3)

[Borislav Deianov]

[-- Attachment #1: Type: text/plain, Size: 4225 bytes --]

On Mon, Nov 08, 2004 at 03:30:22PM -0800, Chris Wright wrote:
> 
> The init error cleanup paths are broken in that driver.  It creates the
> /proc/acpi/ibm dir and forgets to clean it up.  Partially that's due to
> returning directly from the macro IBM_HANDLE_INIT_REQ.  This should help.

Yikes. Guilty as charged.

I reworked Chris's patch a bit and tried both the error and non-error
case here. Len, if it looks good, please apply.

Thanks,
Boris

diff -Nur linux-2.6.10-rc1-ibm-acpi.orig/Documentation/ibm-acpi.txt linux-2.6.10-rc1-ibm-acpi/Documentation/ibm-acpi.txt
--- linux-2.6.10-rc1-ibm-acpi.orig/Documentation/ibm-acpi.txt	2004-10-23 00:43:47.000000000 -0700
+++ linux-2.6.10-rc1-ibm-acpi/Documentation/ibm-acpi.txt	2004-11-08 17:11:49.894712632 -0800
@@ -1,7 +1,7 @@
 		    IBM ThinkPad ACPI Extras Driver
 
-                            Version 0.7
-                          23 October 2004
+                            Version 0.8
+                          8 November 2004
 
                Borislav Deianov <borislav@users.sf.net>
 		      http://ibm-acpi.sf.net/
diff -Nur linux-2.6.10-rc1-ibm-acpi.orig/drivers/acpi/ibm_acpi.c linux-2.6.10-rc1-ibm-acpi/drivers/acpi/ibm_acpi.c
--- linux-2.6.10-rc1-ibm-acpi.orig/drivers/acpi/ibm_acpi.c	2004-10-23 00:43:33.000000000 -0700
+++ linux-2.6.10-rc1-ibm-acpi/drivers/acpi/ibm_acpi.c	2004-11-08 17:11:26.715236448 -0800
@@ -43,9 +43,11 @@
  *  2004-10-19	0.6	use acpi_bus_register_driver() to claim HKEY device
  *  2004-10-23	0.7	fix module loading on A21e, A22p, T20, T21, X20
  *			fix LED control on A21e
+ *  2004-11-08	0.8	fix init error case, don't return from a macro
+ *				thanks to Chris Wright <chrisw@osdl.org>
  */
 
-#define IBM_VERSION "0.7"
+#define IBM_VERSION "0.8"
 
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -1130,25 +1132,19 @@
 			return 0;
 	}
 	
+	*handle = NULL;
+
 	if (required) {
 		printk(IBM_ERR "%s object not found\n", name);
 		return -1;
 	}
 
-	*handle = NULL;
-
 	return 0;
 }
 
-#define IBM_HANDLE_INIT_REQ(object) do {                                      \
-        if (ibm_handle_init(#object, &object##_handle, *object##_parent,      \
-		object##_paths, sizeof(object##_paths)/sizeof(char *), 1) < 0)\
-		return -ENODEV;                                               \
-} while (0)
-
-#define IBM_HANDLE_INIT(object)                                           \
-	ibm_handle_init(#object, &object##_handle, *object##_parent,      \
-		object##_paths, sizeof(object##_paths)/sizeof(char *), 0)
+#define IBM_HANDLE_INIT(object, required)				\
+	ibm_handle_init(#object, &object##_handle, *object##_parent,	\
+		object##_paths, sizeof(object##_paths)/sizeof(char*), required)
 
 
 static void ibm_param(char *feature, char *cmd)
@@ -1184,6 +1180,27 @@
 	if (acpi_disabled)
 		return -ENODEV;
 
+	/* these handles are required */
+	if (IBM_HANDLE_INIT(ec,	  1) < 0 ||
+	    IBM_HANDLE_INIT(hkey, 1) < 0 ||
+	    IBM_HANDLE_INIT(vid,  1) < 0 ||
+	    IBM_HANDLE_INIT(beep, 1) < 0)
+		return -ENODEV;
+
+	/* these handles have alternatives */
+	IBM_HANDLE_INIT(lght, 0);
+	if (IBM_HANDLE_INIT(cmos, !lght_handle) < 0)
+		return -ENODEV;
+	IBM_HANDLE_INIT(sysl, 0);
+	if (IBM_HANDLE_INIT(led, !sysl_handle) < 0)
+		return -ENODEV;
+
+	/* these handles are not required */
+	IBM_HANDLE_INIT(dock,  0);
+	IBM_HANDLE_INIT(bay,   0);
+	IBM_HANDLE_INIT(bayej, 0);
+	IBM_HANDLE_INIT(bled,  0);
+
 	proc_dir = proc_mkdir(IBM_DIR, acpi_root_dir);
 	if (!proc_dir) {
 		printk(IBM_ERR "unable to create proc dir %s", IBM_DIR);
@@ -1191,29 +1208,6 @@
 	}
 	proc_dir->owner = THIS_MODULE;
 	
-	IBM_HANDLE_INIT_REQ(ec);
-	IBM_HANDLE_INIT_REQ(hkey);
-	IBM_HANDLE_INIT_REQ(vid);
-	IBM_HANDLE_INIT(cmos);
-	IBM_HANDLE_INIT(lght);
-	IBM_HANDLE_INIT(dock);
-	IBM_HANDLE_INIT(bay);
-	IBM_HANDLE_INIT(bayej);
-	IBM_HANDLE_INIT(led);
-	IBM_HANDLE_INIT(sysl);
-	IBM_HANDLE_INIT(bled);
-	IBM_HANDLE_INIT_REQ(beep);
-
-	if (!cmos_handle && !lght_handle) {
-		printk(IBM_ERR "neither cmos nor lght object found\n");
-		return -ENODEV;
-	}
-
-	if (!led_handle && !sysl_handle) {
-		printk(IBM_ERR "neither led nor sysl object found\n");
-		return -ENODEV;
-	}
-
 	for (i=0; i<NUM_IBMS; i++) {
 		ret = ibm_init(&ibms[i]);
 		if (ret < 0) {

[-- Attachment #2: ibm-acpi-0.8.patch --]
[-- Type: text/plain, Size: 3774 bytes --]

diff -Nur linux-2.6.10-rc1-ibm-acpi.orig/Documentation/ibm-acpi.txt linux-2.6.10-rc1-ibm-acpi/Documentation/ibm-acpi.txt
--- linux-2.6.10-rc1-ibm-acpi.orig/Documentation/ibm-acpi.txt	2004-10-23 00:43:47.000000000 -0700
+++ linux-2.6.10-rc1-ibm-acpi/Documentation/ibm-acpi.txt	2004-11-08 17:11:49.894712632 -0800
@@ -1,7 +1,7 @@
 		    IBM ThinkPad ACPI Extras Driver
 
-                            Version 0.7
-                          23 October 2004
+                            Version 0.8
+                          8 November 2004
 
                Borislav Deianov <borislav@users.sf.net>
 		      http://ibm-acpi.sf.net/
diff -Nur linux-2.6.10-rc1-ibm-acpi.orig/drivers/acpi/ibm_acpi.c linux-2.6.10-rc1-ibm-acpi/drivers/acpi/ibm_acpi.c
--- linux-2.6.10-rc1-ibm-acpi.orig/drivers/acpi/ibm_acpi.c	2004-10-23 00:43:33.000000000 -0700
+++ linux-2.6.10-rc1-ibm-acpi/drivers/acpi/ibm_acpi.c	2004-11-08 17:11:26.715236448 -0800
@@ -43,9 +43,11 @@
  *  2004-10-19	0.6	use acpi_bus_register_driver() to claim HKEY device
  *  2004-10-23	0.7	fix module loading on A21e, A22p, T20, T21, X20
  *			fix LED control on A21e
+ *  2004-11-08	0.8	fix init error case, don't return from a macro
+ *				thanks to Chris Wright <chrisw@osdl.org>
  */
 
-#define IBM_VERSION "0.7"
+#define IBM_VERSION "0.8"
 
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -1130,25 +1132,19 @@
 			return 0;
 	}
 	
+	*handle = NULL;
+
 	if (required) {
 		printk(IBM_ERR "%s object not found\n", name);
 		return -1;
 	}
 
-	*handle = NULL;
-
 	return 0;
 }
 
-#define IBM_HANDLE_INIT_REQ(object) do {                                      \
-        if (ibm_handle_init(#object, &object##_handle, *object##_parent,      \
-		object##_paths, sizeof(object##_paths)/sizeof(char *), 1) < 0)\
-		return -ENODEV;                                               \
-} while (0)
-
-#define IBM_HANDLE_INIT(object)                                           \
-	ibm_handle_init(#object, &object##_handle, *object##_parent,      \
-		object##_paths, sizeof(object##_paths)/sizeof(char *), 0)
+#define IBM_HANDLE_INIT(object, required)				\
+	ibm_handle_init(#object, &object##_handle, *object##_parent,	\
+		object##_paths, sizeof(object##_paths)/sizeof(char*), required)
 
 
 static void ibm_param(char *feature, char *cmd)
@@ -1184,6 +1180,27 @@
 	if (acpi_disabled)
 		return -ENODEV;
 
+	/* these handles are required */
+	if (IBM_HANDLE_INIT(ec,	  1) < 0 ||
+	    IBM_HANDLE_INIT(hkey, 1) < 0 ||
+	    IBM_HANDLE_INIT(vid,  1) < 0 ||
+	    IBM_HANDLE_INIT(beep, 1) < 0)
+		return -ENODEV;
+
+	/* these handles have alternatives */
+	IBM_HANDLE_INIT(lght, 0);
+	if (IBM_HANDLE_INIT(cmos, !lght_handle) < 0)
+		return -ENODEV;
+	IBM_HANDLE_INIT(sysl, 0);
+	if (IBM_HANDLE_INIT(led, !sysl_handle) < 0)
+		return -ENODEV;
+
+	/* these handles are not required */
+	IBM_HANDLE_INIT(dock,  0);
+	IBM_HANDLE_INIT(bay,   0);
+	IBM_HANDLE_INIT(bayej, 0);
+	IBM_HANDLE_INIT(bled,  0);
+
 	proc_dir = proc_mkdir(IBM_DIR, acpi_root_dir);
 	if (!proc_dir) {
 		printk(IBM_ERR "unable to create proc dir %s", IBM_DIR);
@@ -1191,29 +1208,6 @@
 	}
 	proc_dir->owner = THIS_MODULE;
 	
-	IBM_HANDLE_INIT_REQ(ec);
-	IBM_HANDLE_INIT_REQ(hkey);
-	IBM_HANDLE_INIT_REQ(vid);
-	IBM_HANDLE_INIT(cmos);
-	IBM_HANDLE_INIT(lght);
-	IBM_HANDLE_INIT(dock);
-	IBM_HANDLE_INIT(bay);
-	IBM_HANDLE_INIT(bayej);
-	IBM_HANDLE_INIT(led);
-	IBM_HANDLE_INIT(sysl);
-	IBM_HANDLE_INIT(bled);
-	IBM_HANDLE_INIT_REQ(beep);
-
-	if (!cmos_handle && !lght_handle) {
-		printk(IBM_ERR "neither cmos nor lght object found\n");
-		return -ENODEV;
-	}
-
-	if (!led_handle && !sysl_handle) {
-		printk(IBM_ERR "neither led nor sysl object found\n");
-		return -ENODEV;
-	}
-
 	for (i=0; i<NUM_IBMS; i++) {
 		ret = ibm_init(&ibms[i]);
 		if (ret < 0) {

Re: [PATCH] ibm-acpi-0.8 (was Re: 2.6.10-rc1-mm3)

[Chris Wright]

* Borislav Deianov (borislav@users.sourceforge.net) wrote:
> On Mon, Nov 08, 2004 at 03:30:22PM -0800, Chris Wright wrote:
> > 
> > The init error cleanup paths are broken in that driver.  It creates the
> > /proc/acpi/ibm dir and forgets to clean it up.  Partially that's due to
> > returning directly from the macro IBM_HANDLE_INIT_REQ.  This should help.
> 
> Yikes. Guilty as charged.
> 
> I reworked Chris's patch a bit and tried both the error and non-error
> case here. Len, if it looks good, please apply.

Ah, even better.  Thanks Boris.  BTW, you could probably mark ibm_init()
and ibm_handle_init() as __init.

-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] ibm-acpi-0.8 (was Re: 2.6.10-rc1-mm3)

[Borislav Deianov]

On Mon, Nov 08, 2004 at 06:12:24PM -0800, Chris Wright wrote:
> 
> Ah, even better.  Thanks Boris.  BTW, you could probably mark ibm_init()
> and ibm_handle_init() as __init.

Good point, I'll do it in the next version.

Thanks,
Boris

Re: [PATCH] ibm-acpi-0.8 (was Re: 2.6.10-rc1-mm3)

[Len Brown]

On Mon, 2004-11-08 at 20:30, Borislav Deianov wrote:
> On Mon, Nov 08, 2004 at 03:30:22PM -0800, Chris Wright wrote:

> I reworked Chris's patch a bit and tried both the error and non-error
> case here. Len, if it looks good, please apply.

Applied.

thanks,
-Len



-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: [PATCH] ibm-acpi-0.8 (was Re: 2.6.10-rc1-mm3)

[Rusty Russell]

On Mon, 2004-11-08 at 18:31 -0800, Borislav Deianov wrote:
> On Mon, Nov 08, 2004 at 06:12:24PM -0800, Chris Wright wrote:
> > 
> > Ah, even better.  Thanks Boris.  BTW, you could probably mark ibm_init()
> > and ibm_handle_init() as __init.
> 
> Good point, I'll do it in the next version.

You cannot use module_param the way you are trying to: it must be used
at the top level, and it will be called before your module's init
function.

This patch might serve as a starting point...
Rusty.

Name: Fix Parameter Handling in ibm_acpi.c
Status: Untested
Signed-off-by: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>

You can't call module_param et al inside a function.  It doesn't make
sense, and it doesn't work.

Index: linux-2.6.10-rc2-bk11-Module/drivers/acpi/ibm_acpi.c
===================================================================
--- linux-2.6.10-rc2-bk11-Module.orig/drivers/acpi/ibm_acpi.c	2004-11-29
11:30:51.723729720 +1100
+++ linux-2.6.10-rc2-bk11-Module/drivers/acpi/ibm_acpi.c	2004-11-29
11:31:08.757140248 +1100
@@ -1147,21 +1147,26 @@
 		object##_paths, sizeof(object##_paths)/sizeof(char*), required)
 
 
-static void ibm_param(char *feature, char *cmd)
+static int set_ibm_param(const char *val, struct kernel_param *kp)
 {
-	int i;
+	unsigned int i;
+	char arg_with_comma[32];
+
+	if (strlen(val) > 30)
+		return -ENOSPC;
+
+	strcpy(arg_with_comma, val);
+	strcat(arg_with_comma, ",");
 
-	strcat(cmd, ",");
 	for (i=0; i<NUM_IBMS; i++)
-		if (strcmp(ibms[i].name, feature) == 0)
-			ibms[i].write(&ibms[i], cmd);
-}	
-
-#define IBM_PARAM(feature) do {					\
-	static char cmd[32];					\
-	module_param_string(feature, cmd, sizeof(cmd) - 1, 0);	\
-	ibm_param(#feature, cmd);				\
-} while (0)
+		if (strcmp(ibms[i].name, kp->name) == 0)
+			return ibms[i].write(&ibms[i], arg_with_comma);
+	BUG();
+	return -EINVAL;
+} 
+
+#define IBM_PARAM(feature) \
+	module_param_call(feature, set_ibm_param, NULL, NULL, 0)
 
 static void __exit acpi_ibm_exit(void)
 {
@@ -1216,16 +1221,6 @@
 		}
 	}
 
-	IBM_PARAM(hotkey);
-	IBM_PARAM(bluetooth);
-	IBM_PARAM(video);
-	IBM_PARAM(light);
-	IBM_PARAM(dock);
-	IBM_PARAM(bay);
-	IBM_PARAM(cmos);
-	IBM_PARAM(led);
-	IBM_PARAM(beep);
-
 	return 0;
 }
 
@@ -1235,3 +1230,13 @@
 MODULE_AUTHOR("Borislav Deianov");
 MODULE_DESCRIPTION(IBM_DESC);
 MODULE_LICENSE("GPL");
+
+IBM_PARAM(hotkey);
+IBM_PARAM(bluetooth);
+IBM_PARAM(video);
+IBM_PARAM(light);
+IBM_PARAM(dock);
+IBM_PARAM(bay);
+IBM_PARAM(cmos);
+IBM_PARAM(led);
+IBM_PARAM(beep);


-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/