lkml.org 
[lkml]   [2008]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 01/18] lirc core device driver infrastructure
Date
On Tuesday 09 September 2008 15:01:02 Christoph Hellwig wrote:
> > +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.

removed

> > +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'm pretty sure LIRC will be recognized by its current users, it is
probably enough if it is in the help text though.

> > +#define __KERNEL_SYSCALLS__
>
> No need for this in any semi-recent kernel.

already gone

> > +#include <linux/unistd.h>
> > +#include <linux/kthread.h>
> > +
> > +/* SysFS header */
> > +#include <linux/device.h>
>
> That comment is not quite correct, just remove it.

removed

> > +/* 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.

all inlines (65 in drivers/inpu/lirc) removed

> > +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.

agree, it's done dynamically now

> > +{
> > + 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.

yes, already removed. also removed custom TRUE/FALSE defines

> 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".
>
> > + } 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.

added a waitqueue and wait_event

> > + 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..

ok

> > +int lirc_unregister_plugin(int minor)
>
> Why doesn't this one take a struct lirc_plugin pointer?

I don't know, It doesn't really help though since the struct lirc_plugin
is copied by value to irctl.p in lirc_register_plugin.

> > +{
> > + struct irctl *ir;
> > + DECLARE_COMPLETION(tn);
> > + DECLARE_COMPLETION(tn2);
>
> both completion seems unused.

the one in lirc_register_plugin too, removed

> > + /* end up polling thread */
> > + if (ir->task) {
> > + wake_up_process(ir->task);
> > + kthread_stop(ir->task);
> > + }
>
> kthread_stop already wakes the thread up.

removed

> > +/*
> > + * 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.

already removed.

> > +static int irctl_open(struct inode *inode, struct file *file)
> > +{
> > + struct irctl *ir;
> > + int retval;
> > +
> > + if (MINOR(inode->i_rdev) >= MAX_IRCTL_DEVICES) {
>
> iminor.

replaced in all occurances

> > + /* 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.

done

> > +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.

I don't see why poll has to hold the lock, removed

> > +/* #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

removed

> > +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.

I'll look at it.

> > +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.

removed

Thanks for the review.

Janne


\
 
 \ /
  Last update: 2008-09-12 02:19    [from the cache]
©2003-2011 Jasper Spaans