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-2014 Jasper Spaans. Advertise on this site