[KJ] [PATCH] apm: clean up module initalization

[KJ] [PATCH] apm: clean up module initalization

[Neil Horman]

Patch to clean up module initalization for apm.c.  I had started by auditing for
proper return code checks in misc_register, but I found that in the event of an
initalization failure, a proc file and a kernel thread were left hanging out.
this patch properly cleans up those loose ends on any initalization failure.

Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 apm.c |   23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)


diff --git a/arch/i386/kernel/apm.c b/arch/i386/kernel/apm.c
index 8591f2f..75d8bde 100644
--- a/arch/i386/kernel/apm.c
+++ b/arch/i386/kernel/apm.c
@@ -2339,16 +2339,22 @@ #endif
 	ret = kernel_thread(apm, NULL, CLONE_KERNEL | SIGCHLD);
 	if (ret < 0) {
 		printk(KERN_ERR "apm: disabled - Unable to start kernel thread.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_proc;
 	}
 
 	if (num_online_cpus() > 1 && !smp ) {
 		printk(KERN_NOTICE
 		   "apm: disabled - APM is not SMP safe (power off active).\n");
-		return 0;
+		ret = -EOPNOTSUPP;
+		goto out_thread;
 	}
 
-	misc_register(&apm_device);
+	if (misc_register(&apm_device)) {
+		printk(KERN_ERR "apm: Unable to register misc device\n");
+		ret = -ENOMEM;
+		goto out_thread;
+	}
 
 	if (HZ != 100)
 		idle_period = (idle_period * HZ) / 100;
@@ -2357,8 +2363,17 @@ #endif
 		pm_idle  = apm_cpu_idle;
 		set_pm_idle = 1;
 	}
+	
+	return  0;
 
-	return 0;
+out_thread:
+	exit_kapmd = 1;
+	while (kapmd_running)
+		schedule();
+out_proc:
+	remove_proc_entry("apm", NULL);
+
+	return ret;
 }
 
 static void __exit apm_exit(void)

Re: [KJ] [PATCH] apm: clean up module initalization

[Alexey Dobriyan]

On Tue, Aug 01, 2006 at 12:25:48PM -0400, Neil Horman wrote:
> Patch to clean up module initalization for apm.c.  I had started by auditing for
> proper return code checks in misc_register, but I found that in the event of an
> initalization failure, a proc file and a kernel thread were left hanging out.
> this patch properly cleans up those loose ends on any initalization failure.

> --- a/arch/i386/kernel/apm.c
> +++ b/arch/i386/kernel/apm.c

> -	misc_register(&apm_device);
> +	if (misc_register(&apm_device)) {
> +		printk(KERN_ERR "apm: Unable to register misc device\n");
> +		ret = -ENOMEM;
> +		goto out_thread;

misc_register() can fail for reasons other than ENOMEM.

Re: [KJ] [PATCH] apm: clean up module initalization

[Stephen Rothwell]

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

Hi Neil,

On Tue, 1 Aug 2006 12:25:48 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:
>
> diff --git a/arch/i386/kernel/apm.c b/arch/i386/kernel/apm.c
> index 8591f2f..75d8bde 100644
> --- a/arch/i386/kernel/apm.c
> +++ b/arch/i386/kernel/apm.c
> @@ -2339,16 +2339,22 @@ #endif
>  	ret = kernel_thread(apm, NULL, CLONE_KERNEL | SIGCHLD);
>  	if (ret < 0) {
>  		printk(KERN_ERR "apm: disabled - Unable to start kernel thread.\n");
> -		return -ENOMEM;
> +		ret = -ENOMEM;

Doesn't kernel_thread return a good error code?

> +		goto out_proc;

Good catch.

>  	}
>  
>  	if (num_online_cpus() > 1 && !smp ) {
>  		printk(KERN_NOTICE
>  		   "apm: disabled - APM is not SMP safe (power off active).\n");
> -		return 0;
> +		ret = -EOPNOTSUPP;
> +		goto out_thread;

This is a success return.  i.e. we want the kernel thread and the proc
file.  So the original was correct.  We have checked above if we are
trying to run SMP without the smp boot option - this check is just to stop
any more initialisation.

>  	}
>  
> -	misc_register(&apm_device);
> +	if (misc_register(&apm_device)) {
> +		printk(KERN_ERR "apm: Unable to register misc device\n");
> +		ret = -ENOMEM;
> +		goto out_thread;
> +	}

We don't care if misc_register fails as the apm module can still do much
good work even if user mode cannot control it.  So the original was
correct.  Maybe a comment here would be good as this patch has been
submitted before.

>  
>  	if (HZ != 100)
>  		idle_period = (idle_period * HZ) / 100;
> @@ -2357,8 +2363,17 @@ #endif
>  		pm_idle  = apm_cpu_idle;
>  		set_pm_idle = 1;
>  	}
> +	
> +	return  0;
>  
> -	return 0;
> +out_thread:
> +	exit_kapmd = 1;
> +	while (kapmd_running)
> +		schedule();

So we don't need the above.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [KJ] [PATCH] apm: clean up module initalization

[Neil Horman]

Copy all that.  New patch attached, with corrected cleanups for dangling proc
entry on kernel_thread failure, and logged warning on failure to register
misc_device.

Regards
Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 apm.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletion(-)


diff --git a/arch/i386/kernel/apm.c b/arch/i386/kernel/apm.c
index 8591f2f..24fd577 100644
--- a/arch/i386/kernel/apm.c
+++ b/arch/i386/kernel/apm.c
@@ -2339,6 +2339,7 @@ #endif
 	ret = kernel_thread(apm, NULL, CLONE_KERNEL | SIGCHLD);
 	if (ret < 0) {
 		printk(KERN_ERR "apm: disabled - Unable to start kernel thread.\n");
+		remove_proc_entry("apm", NULL);
 		return -ENOMEM;
 	}
 
@@ -2348,7 +2349,13 @@ #endif
 		return 0;
 	}
 
-	misc_register(&apm_device);
+	/*
+	 * Note we don't actually care if the misc_device cannot be registered.
+	 * this driver can do its job without it, even if userspace can't
+	 * control it.  just log the error
+	 */
+	if (misc_register(&apm_device))
+		printk(KERN_WARNING "apm: Could not register misc device.\n");
 
 	if (HZ != 100)
 		idle_period = (idle_period * HZ) / 100;

Re: [KJ] [PATCH] apm: clean up module initalization

[Stephen Rothwell]

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

On Tue, 1 Aug 2006 22:21:52 -0400 Neil Horman <nhorman@tuxdriver.com> wrote:
>
> Copy all that.  New patch attached, with corrected cleanups for dangling proc
> entry on kernel_thread failure, and logged warning on failure to register
> misc_device.
> 
> Regards
> Neil
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>

Acked-by: Stephen Rothwell <sfr@canb.auug.org.au>

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]