On Tuesday 09 September 2008 15:01:02 Christoph Hellwig wrote:
quoted text
> > +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

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

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

already gone

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

removed

quoted text
> > +/*  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

quoted text
> > +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

quoted text
> > +{
> > +	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

quoted text
> 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

quoted text
> > +	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

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

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

the one in lirc_register_plugin too, removed

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

removed

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

quoted text
> > +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

quoted text
> > +	/* 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

quoted text
> > +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

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

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

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