Re: [PATCH] Altix system controller communication driver

[Andrew Morton <akpm@osdl.org>]

Greg Howard <ghoward@sgi.com> wrote:
>
> Hi Andrew,
> 
> The following patch ("altix-system-controller-driver.patch")
> implements a driver that allows user applications to access the system
> controllers on SGI Altix machines.  It applies on top of the
> 2.6.8-rc-mm1 patch.
> 
>...
> +static struct file_operations scdrv_fops = {
> +	owner:THIS_MODULE,
> +	read:scdrv_read,
> +	write:scdrv_write,
> +	poll:scdrv_poll,
> +	open:scdrv_open,
> +	release:scdrv_release,
> +};

As Jes says,

	.owner	= THIS_MODULE,

is preferred here.

> +			scd =
> +			    (struct sysctl_data_s *) kmalloc
> +				(sizeof (struct sysctl_data_s), GFP_KERNEL);

There's no need to cast the return value of kmalloc.

	scd = kmalloc(sizeof(*scd), GFP_KERNEL);

would suffice here.

> +static ssize_t
> +scdrv_read(struct file *file, char *buf, size_t count, loff_t * f_pos)
> +{
> +	int status;
> +	int len;
> +	unsigned long flags;
> +	struct subch_data_s *sd = (struct subch_data_s *) file->private_data;
> +
> +	/* try to get control of the read buffer */
> +	if (down_trylock(&sd->sd_rbs)) {
> +		/* somebody else has it now;
> +		 * if we're non-blocking, then exit...
> +		 */
> +		if (file->f_flags & O_NONBLOCK) {
> +			return -EAGAIN;
> +		}

hm.  O_NONBLOCK means "don't wait for more input to arrive" rather than
"don't block if someone else is holding a lock I want".  But given that the
semaphore is held by !O_NONBLOCK readers, it has to be done this way.

I guess there's no bug here, but it's a bit odd.

> +		copy_to_user(buf, sd->sd_rb, len);

What Jes said: return -EFAULT if copy_to_user() returns non-zero.

> +static unsigned int
> +scdrv_poll(struct file *file, struct poll_table_struct *wait)
> +{
> +	unsigned int mask = 0;
> +	int status = 0;
> +	struct subch_data_s *sd = (struct subch_data_s *) file->private_data;
> +	unsigned long flags;
> +
> +	scdrv_lock_all(sd, &flags);
> +	poll_wait(file, &sd->sd_rq, wait);
> +	poll_wait(file, &sd->sd_wq, wait);

This function will sleep with spinlocks held, won't it?