> 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.
____
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
