lkml.org 
[lkml]   [2008]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 01/18] lirc core device driver infrastructure
    Date
    On Tuesday 09 September 2008 17:33:08 Jonathan Corbet wrote:
    > I think it's most cool that this code is finally making its way
    > toward the mainline. What I have is mostly nits...

    ignoring bits already handled in Christoph Hellwig's review

    > > + dprintk("lirc_dev [%d]: open result = -ENODEV\n",
    > > + MINOR(inode->i_rdev));
    > > + return -ENODEV;
    > > + }
    > > +
    > > + ir = &irctls[MINOR(inode->i_rdev)];
    > > +
    > > + dprintk(LOGHEAD "open called\n", ir->p.name, ir->p.minor);
    > > +
    > > + /* 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);
    > > +
    > > + if (mutex_lock_interruptible(&plugin_lock))
    > > + return -ERESTARTSYS;
    >
    > So the plugin open() function is called outside of any lock. Note
    > that open() no longer has BKL protection as of 2.6.27. This might
    > all be OK, but I hope you're convinced of it.

    plugins fops are called directly now

    > > + if (ir->p.owner != NULL && try_module_get(ir->p.owner)) {
    > > + ++ir->open;
    > > + retval = ir->p.set_use_inc(ir->p.data);
    >
    > Why is there a set_use_inc() function separate from open()?

    since not all plugins set p.fops? some of those functions in the drivers
    are even empty. I'll look at it.

    > > + if (retval != SUCCESS) {
    > > + module_put(ir->p.owner);
    > > + --ir->open;
    > > + }
    > > + } else {
    > > + if (ir->p.owner == NULL)
    > > + dprintk(LOGHEAD "no module owner!!!\n",
    > > + ir->p.name, ir->p.minor);
    > > +
    > > + retval = -ENODEV;
    > > + }
    >
    > If "no owner" is a fatal condition, it seems better to check it when
    > the plugin is registered.

    maybe because it's not needed if the plugin has its own fops.open?

    > (Also, BTW, your variant of dprintk() is
    > confusing to read - I was wondering where all the %'s were. I still
    > wonder, actually. dev_printk() would be better.)

    the LOGHEAD define is confusing, I'll look into make more clear

    > > +static int irctl_close(struct inode *inode, struct file *file)
    > > +{
    > > + struct irctl *ir = &irctls[MINOR(inode->i_rdev)];
    > > +
    > > + dprintk(LOGHEAD "close called\n", ir->p.name, ir->p.minor);
    > > +
    > > + /* if the plugin has a close function use it instead */
    > > + if (ir->p.fops && ir->p.fops->release)
    > > + return ir->p.fops->release(inode, file);
    > > +
    > > + if (mutex_lock_interruptible(&plugin_lock))
    > > + return -ERESTARTSYS;
    >
    > Should this be interruptible? You probably want the close call to
    > get its job done. Maybe mutex_lock_killable() - and still do the
    > cleanup on a signal?

    That's better than the current state.

    > > +static int irctl_ioctl(struct inode *inode, struct file *file,
    > > + unsigned int cmd, unsigned long arg)
    > > +{
    > > + unsigned long mode;
    > > + int result;
    > > + struct irctl *ir = &irctls[MINOR(inode->i_rdev)];
    > > +
    > > + dprintk(LOGHEAD "ioctl called (0x%x)\n",
    > > + ir->p.name, ir->p.minor, cmd);
    > > +
    > > + /* if the plugin has a ioctl function use it instead */
    > > + if (ir->p.fops && ir->p.fops->ioctl)
    > > + return ir->p.fops->ioctl(inode, file, cmd, arg);
    > > +
    > > + if (ir->p.minor == NOPLUG || !ir->attached) {
    > > + dprintk(LOGHEAD "ioctl result = -ENODEV\n",
    > > + ir->p.name, ir->p.minor);
    > > + return -ENODEV;
    > > + }
    > > +
    > > + /* Give the plugin a chance to handle the ioctl */
    > > + if (ir->p.ioctl) {
    > > + result = ir->p.ioctl(inode, file, cmd, arg);
    > > + if (result != -ENOIOCTLCMD)
    > > + return result;
    > > + }
    >
    > Why two ioctl() handlers? It seems better to just have one way for
    > plugins to handle this call.

    after a cursory look this is used if the plugin doesn't reimplement or
    want to overwrite the commands handled in irctl. I'll look if this can
    be handled better.

    > > + default:
    > > + result = -ENOIOCTLCMD;
    >
    > Hmm, I note with interest that unlocked_ioctl() remaps -ENOIOCTLCMD
    > to -EINVAL, while regular, locked ioctl() (which this is) does not.
    > Not sure what to make of that.

    changed this return value to -EINVAL

    > > +static ssize_t irctl_read(struct file *file,
    > > + char *buffer,
    > > + size_t length,
    > > + loff_t *ppos)
    > > +{
    >
    > ...
    >
    > > + schedule();
    > > + set_current_state(TASK_INTERRUPTIBLE);
    > > + if (!ir->attached) {
    > > + ret = -ENODEV;
    > > + break;
    > > + }
    >
    > How can ir->attached go to zero? You checked it earlier and have
    > been holding the mutex ever since.

    I don't think the lock has to be held while sleeping

    > > +static ssize_t irctl_write(struct file *file, const char *buffer,
    > > + size_t length, loff_t *ppos)
    > > +{
    > > + struct irctl *ir =
    > > &irctls[MINOR(file->f_dentry->d_inode->i_rdev)];
    > > + dprintk(LOGHEAD "write called\n", ir->p.name, ir->p.minor);
    > > +
    > > + /* if the plugin has a specific read function use it instead
    > > */
    > > + if (ir->p.fops && ir->p.fops->write)
    > > + return ir->p.fops->write(file, buffer, length, ppos);
    >
    > Looks like you're using the "specific write function" instead :)

    plugins fops will be used directly now, no need for this comment and
    code anymore ;)

    > > +static struct file_operations fops = {
    > > + .read = irctl_read,
    > > + .write = irctl_write,
    > > + .poll = irctl_poll,
    > > + .ioctl = irctl_ioctl,
    > > + .open = irctl_open,
    > > + .release = irctl_close
    > > +};
    >
    > You should probably set .owner too.

    done

    > > +static int lirc_dev_init(void)
    > > +{
    > > + int i;
    > > +
    > > + for (i = 0; i < MAX_IRCTL_DEVICES; ++i)
    > > + init_irctl(&irctls[i]);
    > > +
    > > + if (register_chrdev(IRCTL_DEV_MAJOR, IRCTL_DEV_NAME, &fops))
    > > {
    > > + printk(KERN_ERR "lirc_dev: register_chrdev
    > > failed\n");
    > > + goto out;
    > > + }
    > > +
    > > + lirc_class = class_create(THIS_MODULE, "lirc");
    > > + if (IS_ERR(lirc_class)) {
    > > + printk(KERN_ERR "lirc_dev: class_create failed\n");
    > > + goto out_unregister;
    > > + }
    > > +
    > > + printk(KERN_INFO "lirc_dev: IR Remote Control driver
    > > registered, "
    > > + "major %d \n", IRCTL_DEV_MAJOR);
    > > +
    > > + return SUCCESS;
    > > +
    > > +out_unregister:
    > > + /* unregister_chrdev returns void now */
    > > + unregister_chrdev(IRCTL_DEV_MAJOR, IRCTL_DEV_NAME);
    > > +out:
    > > + return -1;
    > > +}
    >
    > Do you want to fail completely if class_create() fails? What if
    > somebody already opened one of your devices?

    order reversed and using alloc_chrdev_region so there is no device which
    could be opened. Also returning correct error values now.

    Thanks for the review.

    Janne


    \
     
     \ /
      Last update: 2008-09-12 02:19    [from the cache]
    ©2003-2014 Jasper Spaans. Advertise on this site