lirc: igorplususb error handling cleanup

lirc: igorplususb error handling cleanup

[Pekka J Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

Use goto instead of a big ugly switch for error handling. Convert some 
kmalloc + memset pairs to kzalloc too.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 drivers/lirc_igorplugusb/lirc_igorplugusb.c |  116 +++++++++++++---------------
 1 file changed, 54 insertions(+), 62 deletions(-)

Index: lirc-0.8.1/drivers/lirc_igorplugusb/lirc_igorplugusb.c
===================================================================
--- lirc-0.8.1.orig/drivers/lirc_igorplugusb/lirc_igorplugusb.c
+++ lirc-0.8.1/drivers/lirc_igorplugusb/lirc_igorplugusb.c
@@ -405,7 +405,7 @@ static int usb_remote_probe(struct usb_i
 	int minor = 0;
 	char buf[63], name[128]="";
 	int mem_failure = 0;
-	int ret;
+	int ret, err;
 
 	dprintk(DRIVER_NAME ": usb probe called.\n");
 
@@ -430,67 +430,44 @@ static int usb_remote_probe(struct usb_i
 		devnum, bytes_in_key, maxp);
 
 
-	/* allocate kernel memory */
-	mem_failure = 0;
-	if (!(ir = kmalloc(sizeof(struct irctl), GFP_KERNEL))) {
-		mem_failure = 1;
-	} else {
-		memset(ir, 0, sizeof(struct irctl));
-
-		if (!(plugin = kmalloc(sizeof(struct lirc_plugin), GFP_KERNEL))) {
-			mem_failure = 2;
-		} else if (!(rbuf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL))) {
-			mem_failure = 3;
-		} else if (lirc_buffer_init(rbuf, bytes_in_key, 
-                		DEVICE_BUFLEN+ADDITIONAL_LIRC_BYTES)) {
-			mem_failure = 4;
-		} else if (!(ir->buf_in = usb_buffer_alloc(dev, 
-				DEVICE_BUFLEN+DEVICE_HEADERLEN, 
-                                GFP_ATOMIC, &ir->dma_in))) {
-			mem_failure = 5;
-		} else {
-
-			memset(plugin, 0, sizeof(struct lirc_plugin));
-
-			strcpy(plugin->name, DRIVER_NAME " ");
-			plugin->minor = -1;
-			plugin->code_length = bytes_in_key*8; /* in bits */
-			plugin->features = LIRC_CAN_REC_MODE2;
-			plugin->data = ir;
-			plugin->rbuf = rbuf;
-			plugin->set_use_inc = &set_use_inc;
-			plugin->set_use_dec = &set_use_dec;
-			plugin->sample_rate = SAMPLE_RATE;    /* per second */
-			plugin->add_to_buf = &usb_remote_poll;
-			plugin->owner = THIS_MODULE;
-
-			init_MUTEX(&ir->lock);
-			init_waitqueue_head(&ir->wait_out);
-
-			if ((minor = lirc_register_plugin(plugin)) < 0) {
-				mem_failure = 9;
-			}
-		}
-	}
-
-	/* free allocated memory in case of failure */
-	switch (mem_failure) {
-	case 9:
-		usb_buffer_free(dev, DEVICE_BUFLEN+DEVICE_HEADERLEN, 
-                	ir->buf_in, ir->dma_in);
-	case 5:
-		lirc_buffer_free(rbuf);
-	case 4:
-		kfree(rbuf);
-	case 3:
-		kfree(plugin);
-	case 2:
-		kfree(ir);
-	case 1:
-		printk(DRIVER_NAME "[%d]: out of memory (code=%d)\n",
-			devnum, mem_failure);
-		return -ENOMEM;
-	}
+	ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
+	if (!ir)
+		goto failed_ctl_alloc;
+
+	plugin = kzalloc(sizeof(struct lirc_plugin), GFP_KERNEL);
+	if (!plugin)
+		goto failed_plugin_alloc;
+	rbuf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
+	if (!rbuf)
+		goto failed_rbuf_alloc;
+
+	err = lirc_buffer_init(rbuf, bytes_in_key, DEVICE_BUFLEN +
+			       ADDITIONAL_LIRC_BYTES);
+	if (err)
+		goto failed_buffer_init;
+
+	ir->buf_in = usb_buffer_alloc(dev, DEVICE_BUFLEN+DEVICE_HEADERLEN, GFP_ATOMIC, &ir->dma_in);
+	if (!ir->buf_in)
+		goto failed_buf_in_alloc;
+
+	strcpy(plugin->name, DRIVER_NAME " ");
+	plugin->minor = -1;
+	plugin->code_length = bytes_in_key*8; /* in bits */
+	plugin->features = LIRC_CAN_REC_MODE2;
+	plugin->data = ir;
+	plugin->rbuf = rbuf;
+	plugin->set_use_inc = &set_use_inc;
+	plugin->set_use_dec = &set_use_dec;
+	plugin->sample_rate = SAMPLE_RATE;    /* per second */
+	plugin->add_to_buf = &usb_remote_poll;
+	plugin->owner = THIS_MODULE;
+
+	init_MUTEX(&ir->lock);
+	init_waitqueue_head(&ir->wait_out);
+
+	minor = lirc_register_plugin(plugin);
+	if (minor < 0)
+		goto failed_register;
 
 	plugin->minor = minor;
 	ir->p = plugin;
@@ -522,6 +499,21 @@ static int usb_remote_probe(struct usb_i
 
 	usb_set_intfdata(intf, ir);
 	return SUCCESS;
+
+  failed_register:
+	usb_buffer_free(dev, DEVICE_BUFLEN+DEVICE_HEADERLEN, ir->buf_in,
+			ir->dma_in);
+  failed_buf_in_alloc:
+	lirc_buffer_free(rbuf);
+  failed_buffer_init:
+	kfree(rbuf);
+  failed_rbuf_alloc:
+	kfree(plugin);
+  failed_plugin_alloc:
+	kfree(ir);
+  failed_ctl_alloc:
+	printk(DRIVER_NAME "[%d]: out of memory (code=%d)\n", devnum, mem_failure);
+	return -ENOMEM;
 }
 
 

Re: lirc: igorplususb error handling cleanup

[Oleg Verych]

> From: Pekka J Enberg
> Newsgroups: gmane.linux.kernel
> Subject: lirc: igorplususb error handling cleanup
> Date: Fri, 2 Feb 2007 11:15:15 +0200 (EET)
> Archived-At: <http://permalink.gmane.org/gmane.linux.kernel/488937>

Hallo.

> Use goto instead of a big ugly switch for error handling. Convert some 
> kmalloc + memset pairs to kzalloc too.

IMHO, goto ugly 2. But this isn't issue here. Let's try to optimize
all this a little bit:

> Index: lirc-0.8.1/drivers/lirc_igorplugusb/lirc_igorplugusb.c
>===================================================================
> --- lirc-0.8.1.orig/drivers/lirc_igorplugusb/lirc_igorplugusb.c
> +++ lirc-0.8.1/drivers/lirc_igorplugusb/lirc_igorplugusb.c
> @@ -405,7 +405,7 @@ static int usb_remote_probe(struct usb_i
>  	int minor = 0;
>  	char buf[63], name[128]="";
>  	int mem_failure = 0;
> -	int ret;
> +	int ret, err;
>  
>  	dprintk(DRIVER_NAME ": usb probe called.\n");
>  
> @@ -430,67 +430,44 @@ static int usb_remote_probe(struct usb_i
>  		devnum, bytes_in_key, maxp);
>  
>  
> -	/* allocate kernel memory */
> -	mem_failure = 0;
> -	if (!(ir = kmalloc(sizeof(struct irctl), GFP_KERNEL))) {
> -		mem_failure = 1;
> -	} else {
> -		memset(ir, 0, sizeof(struct irctl));
> -
> -		if (!(plugin = kmalloc(sizeof(struct lirc_plugin), GFP_KERNEL))) {
> -			mem_failure = 2;
> -		} else if (!(rbuf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL))) {
> -			mem_failure = 3;

If size of all structs, allocated here is 3-4 pages (say, 4096 bytes
one), then, i think something like, allocating all at once may be
utilized:

,-*-
|struct ir_stuff_t {
|       struct irctl *ir;
|       struct lirc_plugin *plugin;
|       struct lirc_buffer *rbuf;
|} ir_stuff;
|
|ir_stuff = kzalloc(...);
|
|if(!ir_stuff)
|       error;
`-*-       

then join buffer init, usb init together and final register, after
it. Thus to have second erroneous path.

So, lucky path may be faster, erroneous, with more *free() stuff slower,
but who cares?

Finally i would put first stage in do { ...if (error) break; ...} while(0),
second in if (error) ... break.

What about that? (ENOPATCH, but i have no lirc ATM ;)

> -		} else if (lirc_buffer_init(rbuf, bytes_in_key, 
> -                		DEVICE_BUFLEN+ADDITIONAL_LIRC_BYTES)) {
> -			mem_failure = 4;
> -		} else if (!(ir->buf_in = usb_buffer_alloc(dev, 
> -				DEVICE_BUFLEN+DEVICE_HEADERLEN, 
> -                                GFP_ATOMIC, &ir->dma_in))) {
> -			mem_failure = 5;
> -		} else {
> -
> -			memset(plugin, 0, sizeof(struct lirc_plugin));
> -
> -			strcpy(plugin->name, DRIVER_NAME " ");
> -			plugin->minor = -1;
> -			plugin->code_length = bytes_in_key*8; /* in bits */
> -			plugin->features = LIRC_CAN_REC_MODE2;
> -			plugin->data = ir;
> -			plugin->rbuf = rbuf;
> -			plugin->set_use_inc = &set_use_inc;
> -			plugin->set_use_dec = &set_use_dec;
> -			plugin->sample_rate = SAMPLE_RATE;    /* per second */
> -			plugin->add_to_buf = &usb_remote_poll;
> -			plugin->owner = THIS_MODULE;
> -
> -			init_MUTEX(&ir->lock);
> -			init_waitqueue_head(&ir->wait_out);
> -
> -			if ((minor = lirc_register_plugin(plugin)) < 0) {
> -				mem_failure = 9;
> -			}
> -		}
> -	}
> -
> -	/* free allocated memory in case of failure */
> -	switch (mem_failure) {
> -	case 9:
> -		usb_buffer_free(dev, DEVICE_BUFLEN+DEVICE_HEADERLEN, 
> -                	ir->buf_in, ir->dma_in);
> -	case 5:
> -		lirc_buffer_free(rbuf);
> -	case 4:
> -		kfree(rbuf);
> -	case 3:
> -		kfree(plugin);
> -	case 2:
> -		kfree(ir);
> -	case 1:
> -		printk(DRIVER_NAME "[%d]: out of memory (code=%d)\n",
> -			devnum, mem_failure);
> -		return -ENOMEM;
> -	}
> +	ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
> +	if (!ir)
> +		goto failed_ctl_alloc;
> +
> +	plugin = kzalloc(sizeof(struct lirc_plugin), GFP_KERNEL);
> +	if (!plugin)
> +		goto failed_plugin_alloc;
> +	rbuf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
> +	if (!rbuf)
> +		goto failed_rbuf_alloc;
> +
> +	err = lirc_buffer_init(rbuf, bytes_in_key, DEVICE_BUFLEN +
> +			       ADDITIONAL_LIRC_BYTES);
> +	if (err)
> +		goto failed_buffer_init;
> +
> +	ir->buf_in = usb_buffer_alloc(dev, DEVICE_BUFLEN+DEVICE_HEADERLEN, GFP_ATOMIC, &ir->dma_in);
> +	if (!ir->buf_in)
> +		goto failed_buf_in_alloc;
> +
> +	strcpy(plugin->name, DRIVER_NAME " ");
> +	plugin->minor = -1;
> +	plugin->code_length = bytes_in_key*8; /* in bits */
> +	plugin->features = LIRC_CAN_REC_MODE2;
> +	plugin->data = ir;
> +	plugin->rbuf = rbuf;
> +	plugin->set_use_inc = &set_use_inc;
> +	plugin->set_use_dec = &set_use_dec;
> +	plugin->sample_rate = SAMPLE_RATE;    /* per second */
> +	plugin->add_to_buf = &usb_remote_poll;
> +	plugin->owner = THIS_MODULE;
> +
> +	init_MUTEX(&ir->lock);
> +	init_waitqueue_head(&ir->wait_out);
> +
> +	minor = lirc_register_plugin(plugin);
> +	if (minor < 0)
> +		goto failed_register;
>  
>  	plugin->minor = minor;
>  	ir->p = plugin;
> @@ -522,6 +499,21 @@ static int usb_remote_probe(struct usb_i
>  
>  	usb_set_intfdata(intf, ir);
>  	return SUCCESS;
> +
> +  failed_register:
> +	usb_buffer_free(dev, DEVICE_BUFLEN+DEVICE_HEADERLEN, ir->buf_in,
> +			ir->dma_in);
> +  failed_buf_in_alloc:
> +	lirc_buffer_free(rbuf);
> +  failed_buffer_init:
> +	kfree(rbuf);
> +  failed_rbuf_alloc:
> +	kfree(plugin);
> +  failed_plugin_alloc:
> +	kfree(ir);
> +  failed_ctl_alloc:
> +	printk(DRIVER_NAME "[%d]: out of memory (code=%d)\n", devnum, mem_failure);
> +	return -ENOMEM;
>  }
>  
>  

p.s. Please honor information in the 'Mail-Followup-To' header. Thanks.
____

Re: lirc: igorplususb error handling cleanup

[Oliver Neukum]

> > -	/* allocate kernel memory */
> > -	mem_failure = 0;
> > -	if (!(ir = kmalloc(sizeof(struct irctl), GFP_KERNEL))) {
> > -		mem_failure = 1;
> > -	} else {
> > -		memset(ir, 0, sizeof(struct irctl));
> > -
> > -		if (!(plugin = kmalloc(sizeof(struct lirc_plugin), GFP_KERNEL))) {
> > -			mem_failure = 2;
> > -		} else if (!(rbuf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL))) {
> > -			mem_failure = 3;
> 
> If size of all structs, allocated here is 3-4 pages (say, 4096 bytes
> one), then, i think something like, allocating all at once may be
> utilized:
> 
> ,-*-
> |struct ir_stuff_t {
> |       struct irctl *ir;
> |       struct lirc_plugin *plugin;
> |       struct lirc_buffer *rbuf;

I guess you mean 
struct lirc_buffer rbuf;
without the "*"

> |} ir_stuff;
> |
> |ir_stuff = kzalloc(...);
> |
> |if(!ir_stuff)
> |       error;
> `-*-       
> 
> then join buffer init, usb init together and final register, after
> it. Thus to have second erroneous path.

If I understand this code correctly the allocated buffers end up as
buffers used in URBs. If that is the case you must allocate each of
them separately with kmalloc() or usb_alloc_buffer() or you violate
DMA constraints on non-coherent architectures (eg. ppc)

	Regards
		Oliver