[PATCH] scanlog_init cleanup, minor fixes

[PATCH] scanlog_init cleanup, minor fixes

[Nathan Lynch]

scanlog_init() could use some love.

* properly return -ENODEV if this system doesn't support scan-log-dump
* don't printk if scan-log-dump not present; only older systems have it
* convert from create_proc_entry() to preferred proc_create()
* allocate zeroed data buffer
* fix potential memory leak of ent->data on failed create_proc_entry()
* simplify control flow

Signed-off-by: Nathan Lynch <ntl@pobox.com>
---

(not urgent, please consider for 2.6.26)

 arch/powerpc/platforms/pseries/scanlog.c |   37 ++++++++++++++---------------
 1 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/scanlog.c b/arch/powerpc/platforms/pseries/scanlog.c
index 8e1ef16..e5b0ea8 100644
--- a/arch/powerpc/platforms/pseries/scanlog.c
+++ b/arch/powerpc/platforms/pseries/scanlog.c
@@ -195,31 +195,30 @@ const struct file_operations scanlog_fops = {
 static int __init scanlog_init(void)
 {
 	struct proc_dir_entry *ent;
+	void *data;
+	int err = -ENOMEM;
 
 	ibm_scan_log_dump = rtas_token("ibm,scan-log-dump");
-	if (ibm_scan_log_dump == RTAS_UNKNOWN_SERVICE) {
-		printk(KERN_ERR "scan-log-dump not implemented on this system\n");
-		return -EIO;
-	}
+	if (ibm_scan_log_dump == RTAS_UNKNOWN_SERVICE)
+		return -ENODEV;
 
-        ent = create_proc_entry("ppc64/rtas/scan-log-dump",  S_IRUSR, NULL);
-	if (ent) {
-		ent->proc_fops = &scanlog_fops;
-		/* Ideally we could allocate a buffer < 4G */
-		ent->data = kmalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
-		if (!ent->data) {
-			printk(KERN_ERR "Failed to allocate a buffer\n");
-			remove_proc_entry("scan-log-dump", ent->parent);
-			return -ENOMEM;
-		}
-		((unsigned int *)ent->data)[0] = 0;
-	} else {
-		printk(KERN_ERR "Failed to create ppc64/scan-log-dump proc entry\n");
-		return -EIO;
-	}
+	/* Ideally we could allocate a buffer < 4G */
+	data = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
+	if (!data)
+		goto err;
+
+	ent = proc_create("ppc64/rtas/scan-log-dump", S_IRUSR, NULL,
+			  &scanlog_fops);
+	if (!ent)
+		goto err;
+
+	ent->data = data;
 	proc_ppc64_scan_log_dump = ent;
 
 	return 0;
+err:
+	kfree(data);
+	return err;
 }
 
 static void __exit scanlog_cleanup(void)
-- 
1.5.4.rc5

Re: [PATCH] scanlog_init cleanup, minor fixes

[Michael Ellerman]

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

On Sun, 2008-03-23 at 17:51 -0500, Nathan Lynch wrote:
> scanlog_init() could use some love.

> diff --git a/arch/powerpc/platforms/pseries/scanlog.c b/arch/powerpc/platforms/pseries/scanlog.c
> index 8e1ef16..e5b0ea8 100644
> --- a/arch/powerpc/platforms/pseries/scanlog.c
> +++ b/arch/powerpc/platforms/pseries/scanlog.c
> @@ -195,31 +195,30 @@ const struct file_operations scanlog_fops = {
>  static int __init scanlog_init(void)
>  {
>  	struct proc_dir_entry *ent;
> +	void *data;
> +	int err = -ENOMEM;
>  
>  	ibm_scan_log_dump = rtas_token("ibm,scan-log-dump");
> -	if (ibm_scan_log_dump == RTAS_UNKNOWN_SERVICE) {
> -		printk(KERN_ERR "scan-log-dump not implemented on this system\n");
> -		return -EIO;
> -	}
> +	if (ibm_scan_log_dump == RTAS_UNKNOWN_SERVICE)
> +		return -ENODEV;
>  
> -        ent = create_proc_entry("ppc64/rtas/scan-log-dump",  S_IRUSR, NULL);
> -	if (ent) {
> -		ent->proc_fops = &scanlog_fops;
> -		/* Ideally we could allocate a buffer < 4G */
> -		ent->data = kmalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
> -		if (!ent->data) {
> -			printk(KERN_ERR "Failed to allocate a buffer\n");
> -			remove_proc_entry("scan-log-dump", ent->parent);
> -			return -ENOMEM;
> -		}
> -		((unsigned int *)ent->data)[0] = 0;
> -	} else {
> -		printk(KERN_ERR "Failed to create ppc64/scan-log-dump proc entry\n");
> -		return -EIO;
> -	}
> +	/* Ideally we could allocate a buffer < 4G */
> +	data = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
> +	if (!data)
> +		goto err;

Not your bug, but what happens if data is > 4G? Kaboom?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] scanlog_init cleanup, minor fixes

[Nathan Lynch]

Michael Ellerman wrote:
> > +	/* Ideally we could allocate a buffer < 4G */
> > +	data = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
> > +	if (!data)
> > +		goto err;
> 
> Not your bug, but what happens if data is > 4G? Kaboom?

An old RPA doc (scan-log-dump isn't specified in PAPR) says the buffer
should be "contiguous real storage", so... yeah, probably.  That's why
I preserved the comment.  Will fix if I get access to a machine to
test this code more thoroughly (plenty of other issues in this file,
too).

Re: [PATCH] scanlog_init cleanup, minor fixes

[Michael Ellerman]

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

On Wed, 2008-03-26 at 18:09 -0500, Nathan Lynch wrote:
> Michael Ellerman wrote:
> > > +	/* Ideally we could allocate a buffer < 4G */
> > > +	data = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
> > > +	if (!data)
> > > +		goto err;
> > 
> > Not your bug, but what happens if data is > 4G? Kaboom?
> 
> An old RPA doc (scan-log-dump isn't specified in PAPR) says the buffer
> should be "contiguous real storage", so... yeah, probably.  That's why
> I preserved the comment.  Will fix if I get access to a machine to
> test this code more thoroughly (plenty of other issues in this file,
> too).

Cool, no point changing it if we can't test it.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]