> 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/