Messages in this thread | | | Date | Wed, 28 Jul 2004 08:57:37 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] Altix system controller communication driver |
| |
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?
- 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/
| |