[linux-dvb] [PATCH] Userspace tuner

Andreas Oberritter obi at linuxtv.org
Tue Aug 14 17:59:40 CEST 2007


Hello Markus,

Markus Rechberger wrote:
> +static int tuner_open(struct inode *inode, struct file *file)
> +{
> +    struct tuner_info *info = kzalloc(sizeof(struct tuner_info), 
> GFP_KERNEL);

you should return -ENOMEM in case kzalloc fails.

> +    init_waitqueue_head(&info->waitqueue);
> +    mutex_init(&info->lock);
> +    INIT_LIST_HEAD(&info->tuner_commands);
> +    file->private_data = info;
> +    return 0;
> +}
> +
> +static int tuner_close(struct inode *inode, struct file *file)
> +{
> +    struct tuner_info *info, *tuner_info;
> +    struct list_head *list;
> +
> +    if (file->private_data == NULL)
> +        return 0;
> +
> +    info = file->private_data;
> +
> +    if(info == NULL) {
> +        printk("BUG: info == NULL\n");
> +        return 0;
> +    }

I think this check is superflous because of the test above. Also I think
that file->private_data can never be NULL if kzalloc in tuner_open
didn't fail.

> +    switch(cmd) {
> +    case TUNER_ADD_DAEMON:
> +    {
> +        if (daemon_client) {
> +            printk("a userspace daemon seems to be attached already\n");

Calls to printk should include a message level.

> +    /* simple i2c wrapper */
> +    case TUNER_SEND_I2C_CMD:
> +    {
> +        struct tuner_i2c_msg *i2ccmd = (struct tuner_i2c_msg *)arg;
> +        struct tuner_i2c_msg icmd;
> +        struct i2c_msg msg = { .flags = 0 };
> +        struct list_head *list;
> +        int retval = 0;
> +        struct tuner_dev *devlist,*dev=NULL;
> +        if(copy_from_user(&icmd, i2ccmd, sizeof(struct tuner_i2c_msg))) {
> +            mutex_unlock(&info->lock);
> +            printk("unable to copy from user!\n");
> +            goto efault;
> +        }
> +        list_for_each(list, &tuner_devlist) {
> +            devlist = list_entry(list, struct tuner_dev, devlist);
> +            if (devlist->clientid == icmd.clientid) {
> +                dev = devlist;
> +                break;
> +            }
> +        }
> +        if(dev == NULL) {
> +            printk("DEV is zero (clientid: %d)!!!\n", icmd.clientid);
> +            mutex_unlock(&info->lock);
> +            goto einval;
> +        }
> +
> +        /* took this restriction from i2c-dev.c */
> +        if (icmd.len>8192) {
> +            printk("len > 8192 error!\n");
> +            mutex_unlock(&info->lock);
> +            goto einval;
> +        }
> +
> +        msg.buf = kzalloc(icmd.len, GFP_KERNEL);
> +        msg.len = icmd.len;
> +        msg.addr = icmd.addr;
> +
> +        if (msg.buf == NULL) {
> +            mutex_unlock(&info->lock);
> +            goto enomem;
> +        }
> +
> +        if(copy_from_user(msg.buf, i2ccmd->buf,i2ccmd->len)) {
> +            kfree(msg.buf);
> +            mutex_unlock(&info->lock);
> +            printk("unable to copy from user!\n");
> +            goto efault;
> +        }
> +
> +
> +
> +        if(!(retval=i2c_transfer(dev->adap,&msg, 1))) {
> +            printk("retval: %d\n",retval);
> +            kfree(msg.buf);
> +            mutex_unlock(&info->lock);
> +            goto einval;
> +        }
> +        kfree(msg.buf);
> +        mutex_unlock(&info->lock);
> +        goto e_ok;
> +    }
> +
> +    case TUNER_READ_I2C_CMD:
> +    {
> +        struct tuner_i2c_msg *i2ccmd = (struct tuner_i2c_msg *)arg;
> +        struct tuner_i2c_msg icmd;
> +        struct i2c_msg msg = { .flags = I2C_M_RD };
> +        struct tuner_dev *devlist,*dev=NULL;
> +        int retval = 0;
> +        struct list_head *list;
> +        if(copy_from_user(&icmd, i2ccmd, sizeof(struct tuner_i2c_msg))) {
> +            mutex_unlock(&info->lock);
> +            goto efault;
> +        }
> +        list_for_each(list, &tuner_devlist) {
> +            devlist = list_entry(list, struct tuner_dev, devlist);
> +            if (devlist->clientid == icmd.clientid) {
> +                dev = devlist;
> +                break;
> +            }
> +        }
> +        if(dev == NULL) {
> +            mutex_unlock(&info->lock);
> +            goto einval;
> +        }
> +        
> +        msg.len = icmd.len;
> +        msg.addr = icmd.addr;
> +
> +        if (msg.len>8192) {
> +            mutex_unlock(&info->lock);
> +            goto einval;
> +        }
> +
> +        msg.buf = kzalloc(icmd.len, GFP_KERNEL);
> +
> +        if (msg.buf == NULL) {
> +            mutex_unlock(&info->lock);
> +            goto enomem;
> +        }
> +
> +        if(!(retval=i2c_transfer(dev->adap,&msg, 1))) {
> +            printk("retval: %d\n",retval);
> +            mutex_unlock(&info->lock);
> +            goto einval;
> +        }
> +
> +        if (copy_to_user(i2ccmd->buf, msg.buf, msg.len)) {
> +            mutex_unlock(&info->lock);
> +            goto efault;
> +        }
> +        kfree(msg.buf);
> +        mutex_unlock(&info->lock);
> +        goto e_ok;
> +    }

Isn't it better to use i2c-dev instead? You would just have to pass the
ID of the i2c bus to userspace. This would allow sending multiple
messages using a single call i2c_transfer (via the I2C_RDWR ioctl). This
is probably needed when tuners are hidden behind an i2c gate which is
closed automatically when a stop condition occurs.

Regards,
Andreas



More information about the linux-dvb mailing list