Re: [PATCH] riport LADAR driver

[Arjan van de Ven <arjan@infradead.org>]

On Thu, 2006-06-22 at 07:41 -0700, mark gross wrote:
> +
> +#undef PDEBUG
> +#ifdef RIPORT_DEBUG
> +#  define PDEBUG(fmt, args...) printk( KERN_DEBUG "riport: " fmt, ## args)
> +#else	/*  */
> +#  define PDEBUG(fmt, args...)
> +#endif	/*  */

what's wrong with prdebug ?
> +   
> +
> +struct devriport *devriport_init(int major, int minor, int io, int irq,
> +				   int dma, int size, int *presult);
> +void devriport_cleanup(struct devriport *this);
> +int devriport_open(struct devriport *this);
> +int devriport_release(struct devriport *this);
> +int devriport_read(struct devriport *this, char *pbuf, int length);
> +unsigned int devriport_poll(struct devriport *this,
> +			     struct poll_table_struct *ptable);
> +void devriport_irq(struct devriport *this, int irq, struct pt_regs *regs);

if you reorder the functions a bit you can get rid of these prototypes
entirely... (and make a bunch static)


> +
> +irqreturn_t devriport_irq_wrap(int irq, void *pv, struct pt_regs *pr) 
> +{
> +	devriport_irq(pv, irq, pr);
> +	return IRQ_HANDLED;
> +} 

that looks odd; even if it's not your IRQ you always call it handled....




> +int devriport_open(struct devriport *this) 
> +{
> +	int result;
> +	
> +	if (this->usage)
> +		return -EBUSY;
> +	
> +	result =
> +	request_irq(this->irq, devriport_irq_wrap, SA_INTERRUPT, "riport",
> +			this);

are you sure you can handle an interrupt at this time already?  Usually
the request_irq() is done *after* initialization of all other data
structures to make sure that if an irq hits all data structures are
initialized...


> +int devriport_read(struct devriport *this, char *pbuf, int length) 
> +{
> +	DECLARE_WAITQUEUE(wait, current);
> +	int retval;
> +	int length0, length1;
> +	int count;
> +	
> +	add_wait_queue(&this->qwait, &wait);
> +	
> +	retval = 0;
> +	current->state = TASK_INTERRUPTIBLE;
> +	
> +	while (this->pin == this->pout) {

this looks buggy, at least you want to set the state inside the while
loop somewhere (and use set_task_state() and friends API)

> +	current->state = TASK_RUNNING;

set_current_state()

> +		/* length1 is the number of bytes from the current read
> +		 * position in the circular buffer to the end of the buffer */
> +		length1 = this->pbuf + this->size - this->pout;
> +		if (length < length1) {
> +			count = copy_to_user(pbuf, this->pout, length);
> +			WARN_ON(count != length);

WARN_ON isn't really handling the error......
> +void devriport_irq(struct devriport *this, int irq, struct pt_regs *regs) 
> +{
> +	if (this->irqinuse) {
> +		spin_lock_irq(&this->lock);
> +		devriport_rx(this);
> +		spin_unlock_irq(&this->lock);

this unconditionally enables interrupts... that's generally bad. Please
consider using irqsave/irqrestore

> +ssize_t drvriport_read(struct file * pfile, char *pbuf, size_t length,
> +			 loff_t * ppos)
> +{
> +	return devriport_read((struct devriport *)pfile->private_data, pbuf,
> +			       length);
> +}

this looks odd; you don't pass pfile to the _read function below... yet
inside it you do use the file argument. Would make a lot more sense to
pass it in....

> +static struct file_operations drvriport_fops = { 
> +	owner: THIS_MODULE, 
> +	read : drvriport_read,
> +	open : drvriport_open, 
> +	release : drvriport_release, 
> +};

can be "const"



-
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/