Messages in this thread | | | Date | Tue, 9 Sep 2008 09:01:02 -0400 | From | Christoph Hellwig <> | Subject | Re: [PATCH 01/18] lirc core device driver infrastructure |
| |
> +menuconfig INPUT_LIRC > + bool "Linux Infrared Remote Control IR receiver/transmitter drivers" > + default n
n is the default, no needed add a "default n" line for it.
Anyone configuring the kernel should know he's on Linux, so strike the Linux away :)
> +if INPUT_LIRC > + > +config LIRC_DEV > + tristate "LIRC device loadable module support" > + default n > + help > + LIRC device loadable module support, required for most LIRC drivers
Obviously this can be built in, so it should be named better. Also I don't think LIRC means anything to a user, so use Infrared Remote Control support or something similar instead.
I don't quite understand why this needs a different option from INPUT_LIRC.
> +#define __KERNEL_SYSCALLS__
No need for this in any semi-recent kernel.
> +#include <linux/unistd.h> > +#include <linux/kthread.h> > + > +/* SysFS header */ > +#include <linux/device.h>
That comment is not quite correct, just remove it.
> +/* helper function > + * initializes the irctl structure > + */ > +static inline void init_irctl(struct irctl *ir) > +{ > + memset(&ir->p, 0, sizeof(struct lirc_plugin)); > + mutex_init(&ir->buffer_lock); > + ir->p.minor = NOPLUG; > + > + ir->task = NULL; > + ir->jiffies_to_wait = 0; > + > + ir->open = 0; > + ir->attached = 0; > +}
Please don't mark funtion inline unless there's a very good reason for it.
> +static void cleanup(struct irctl *ir) > +{ > + dprintk(LOGHEAD "cleaning up\n", ir->p.name, ir->p.minor); > + > + device_destroy(lirc_class, MKDEV(IRCTL_DEV_MAJOR, ir->p.minor)); > + > + if (ir->buf != ir->p.rbuf) { > + lirc_buffer_free(ir->buf); > + kfree(ir->buf); > + } > + ir->buf = NULL; > + > + init_irctl(ir); > +}
What's the init doing in a cleanup routine? Oh, you initialize it again becaus of the static array. I think the right approach is to dynamically allocate struct irctl.
> +static inline int add_to_buf(struct irctl *ir)
The inline thing also applies here and in various other placez.
> +{ > + if (lirc_buffer_full(ir->buf)) { > + dprintk(LOGHEAD "buffer overflow\n", > + ir->p.name, ir->p.minor); > + return -EOVERFLOW; > + } > + > + if (ir->p.add_to_buf) { > + int res = -ENODATA; > + int got_data = 0; > + > + /* service the device as long as it is returning > + * data and we have space > + */ > + while (!lirc_buffer_full(ir->buf)) { > + res = ir->p.add_to_buf(ir->p.data, ir->buf); > + if (res == SUCCESS) > + got_data++; > + else > + break; > + } > + > + if (res == -ENODEV) > + kthread_stop(ir->task); > + > + return got_data ? SUCCESS : res; > + } > + > + return SUCCESS;
I guess success is a #define for 0? Just user 0 directly. Also the kthread_stop here looks odd. The normal way to use kthreads is to start them when bringing up an interface of some sorts, and call kthread_stop when the interface is brought down. Doing it in a routine like this screams "unclear lifetime rules".
> + do { > + if (ir->open) { > + if (ir->jiffies_to_wait) { > + set_current_state(TASK_INTERRUPTIBLE); > + schedule_timeout(ir->jiffies_to_wait); > + } else { > + interruptible_sleep_on( > + ir->p.get_queue(ir->p.data)); > + }
No sleep on, please. But this one should be trivial to fix anyway, by just changing this to a
set_current_state(TASK_INTERRUPTIBLE); schedule();
and using wake_up_process in the wakeup path.
> + } else { > + /* if device not opened so we can sleep half a second */ > + set_current_state(TASK_INTERRUPTIBLE); > + schedule_timeout(HZ/2); > + }
Yikes. This should use some form of more fine-grained wakeus.
> +int lirc_register_plugin(struct lirc_plugin *p) > +{
Please add a kerneldoc comments for exported functions like this, instead of the comments in the header.
> + struct irctl *ir; > + int minor; > + int bytes_in_key; > + int err; > + DECLARE_COMPLETION(tn); > + > + if (!p) { > + printk(KERN_ERR "lirc_dev: lirc_register_plugin: " > + "plugin pointer must be not NULL!\n"); > + err = -EBADRQC; > + goto out; > + }
No need for this, a null pointer derference should be a clear enough warning for the writer of the broken pluging..
> +int lirc_unregister_plugin(int minor)
Why doesn't this one take a struct lirc_plugin pointer?
> +{ > + struct irctl *ir; > + DECLARE_COMPLETION(tn); > + DECLARE_COMPLETION(tn2);
both completion seems unused.
> + /* end up polling thread */ > + if (ir->task) { > + wake_up_process(ir->task); > + kthread_stop(ir->task); > + }
kthread_stop already wakes the thread up.
> +/* > + * Recent kernels should handle this autmatically by increasing/decreasing > + * use count when a dependant module is loaded/unloaded. > + */ > + > + return SUCCESS;
The comment above looks superflous.
> +static int irctl_open(struct inode *inode, struct file *file) > +{ > + struct irctl *ir; > + int retval; > + > + if (MINOR(inode->i_rdev) >= MAX_IRCTL_DEVICES) {
iminor.
> + /* if the plugin has an open function use it instead */ > + if (ir->p.fops && ir->p.fops->open) > + return ir->p.fops->open(inode, file);
in which case this 'plugin' should just install it's own fops. Thanks to cdev_add we can install fops at minor number granularity.
> +static unsigned int irctl_poll(struct file *file, poll_table *wait) > +{ > + struct irctl *ir = &irctls[MINOR(file->f_dentry->d_inode->i_rdev)]; > + unsigned int ret; > + > + dprintk(LOGHEAD "poll called\n", ir->p.name, ir->p.minor); > + > + /* if the plugin has a poll function use it instead */ > + if (ir->p.fops && ir->p.fops->poll) > + return ir->p.fops->poll(file, wait); > + > + mutex_lock(&ir->buffer_lock);
->poll ust not sleep.
> +/* > + * > + */
???
> + > +void *lirc_get_pdata(struct file *file) > +{ > + void *data = NULL; > + > + if (file && file->f_dentry && file->f_dentry->d_inode && > + file->f_dentry->d_inode->i_rdev) { > + struct irctl *ir; > + ir = &irctls[MINOR(file->f_dentry->d_inode->i_rdev)]; > + data = ir->p.data; > + } > + > + return data; > +} > +EXPORT_SYMBOL(lirc_get_pdata);
Yikes. All these should never really be null. But once you have a struct file you should just use file->private_data.
> +/* #define LIRC_BUFF_POWER_OF_2 */ > +#ifdef LIRC_BUFF_POWER_OF_2 > +#define mod(n, div) ((n) & ((div) - 1)) > +#else > +#define mod(n, div) ((n) % (div)) > +#endif
??
> +static inline void _lirc_buffer_clear(struct lirc_buffer *buf) > +{ > + buf->head = 0; > + buf->tail = 0; > + buf->fill = 0; > +} > +static inline int lirc_buffer_init(struct lirc_buffer *buf, > + unsigned int chunk_size, > + unsigned int size) > +{ > + /* Adjusting size to the next power of 2 would allow for > + * inconditional LIRC_BUFF_POWER_OF_2 optimization */ > + init_waitqueue_head(&buf->wait_poll); > + spin_lock_init(&buf->lock); > + _lirc_buffer_clear(buf); > + buf->chunk_size = chunk_size; > + buf->size = size; > + buf->data = kmalloc(size*chunk_size, GFP_KERNEL); > + if (buf->data == NULL) > + return -1; > + memset(buf->data, 0, size*chunk_size); > + return 0; > +} > +static inline void lirc_buffer_free(struct lirc_buffer *buf) > +{ > + kfree(buf->data); > + buf->data = NULL; > + buf->head = 0; > + buf->tail = 0; > + buf->fill = 0; > + buf->chunk_size = 0; > + buf->size = 0; > +}
Please move these out of line. And please document all the functions. Or switch to a kfifo or the existing tty buffering helpers.
> +static inline void lirc_buffer_lock(struct lirc_buffer *buf, > + unsigned long *flags) > +{ > + spin_lock_irqsave(&buf->lock, *flags); > +} > +static inline void lirc_buffer_unlock(struct lirc_buffer *buf, > + unsigned long *flags) > +{ > + spin_unlock_irqrestore(&buf->lock, *flags); > +}
Please don't do you own spinlock wrappers.
| |