Date:	Fri, 12 Sep 2008 02:16:30 +0200
From:	Janne Grunau <j@...nau.net>
To:	Christoph Hellwig <hch@...radead.org>
Cc:	Jarod Wilson <jwilson@...hat.com>, linux-kernel@...r.kernel.org,
	Jarod Wilson <jarod@...hat.com>,
	Christoph Bartelmus <lirc@...telmus.de>,
	Mario Limonciello <superm1@...ntu.com>,
	Janne Grunau <j@...nau.net>
Subject: Re: [PATCH 01/18] lirc core device driver infrastructure

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists