Messages in this thread | | | Date | Mon, 15 Nov 2010 10:33:37 +0100 | From | Jiri Slaby <> | Subject | Re: [PATCHv3] drivers/misc: Altera active serial implementation |
| |
On 11/15/2010 09:23 AM, Baruch Siach wrote: > --- /dev/null > +++ b/drivers/misc/altera_as.c > @@ -0,0 +1,349 @@ ... > +struct altera_as_device { > + unsigned id; > + struct device *dev; > + struct miscdevice miscdev; > + struct mutex open_lock; > + struct gpio gpios[AS_GPIO_NUM]; > +}; > + > +/* > + * The only functions updating this array are .probe/.remove, which are > + * serialized. Hence, no locking is needed here. > + */ > +static struct { > + int minor;
Why you need minor here? It's in drvdata->miscdev.minor.
> + struct altera_as_device *drvdata; > +} altera_as_devs[AS_MAX_DEVS];
You don't need the struct at all. static struct altera_as_device *drvdata[AS_MAX_DEVS]; should be enough.
> +static int altera_as_open(struct inode *inode, struct file *file) > +{ > + int ret; > + struct altera_as_device *drvdata = get_as_dev(iminor(inode)); > + > + if (drvdata == NULL) > + return -ENODEV; > + file->private_data = drvdata; > + > + ret = mutex_trylock(&drvdata->open_lock); > + if (ret == 0) > + return -EBUSY; > + > + ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios)); > + if (ret < 0) { > + mutex_unlock(&drvdata->open_lock); > + return ret; > + }
So leaving to userspace with mutex held. This is *wrong*. use test_and_set_bit or alike instead.
> + return 0; > +} > + > +static ssize_t altera_as_write(struct file *file, const char __user *buf, > + size_t count, loff_t *ppos) > +{ ... > + while ((count - written) > 0) { > + err = copy_from_user(page_buf, buf+written, AS_PAGE_SIZE); > + if (err < 0) { > + err = -EFAULT; > + goto out; > + } > + > + xmit_cmd(drvdata->gpios, AS_WRITE_ENABLE); > + > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0); > + /* op code */ > + xmit_byte(drvdata->gpios, AS_PAGE_PROGRAM); > + /* address */ > + xmit_byte(drvdata->gpios, (*ppos >> 16) & 0xff); > + xmit_byte(drvdata->gpios, (*ppos >> 8) & 0xff); > + xmit_byte(drvdata->gpios, *ppos & 0xff); > + /* page data (LSB first) */ > + for (i = 0; i < AS_PAGE_SIZE; i++) > + xmit_byte(drvdata->gpios, bitrev8(page_buf[i])); > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1); > + mdelay(7);
So if I write 100 pages, I will _spin_ for 700 ms. You should rather (m)sleep here.
> + > + *ppos += AS_PAGE_SIZE; > + written += AS_PAGE_SIZE; > + } > + > +out: > + kfree(page_buf); > + return err ?: written; > +} ... > +static int __init altera_as_probe(struct platform_device *pdev) > +{ ... > + drvdata->miscdev.minor = MISC_DYNAMIC_MINOR; > + drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d", > + pdata->id); > + if (drvdata->miscdev.name == NULL) > + return -ENOMEM; > + drvdata->miscdev.fops = &altera_as_fops; > + if (misc_register(&drvdata->miscdev) < 0) {
Hmm, how many devices can be in the system? This doesn't look like the right way.
> + kfree(drvdata->miscdev.name); > + return -ENODEV; > + } > + altera_as_devs[pdata->id].minor = drvdata->miscdev.minor; > + altera_as_devs[pdata->id].drvdata = drvdata;
So now the device is usable without the mutex and gpios inited?
> + mutex_init(&drvdata->open_lock); > + > + drvdata->id = pdata->id; > + > + drvdata->gpios[ASIO_DATA].gpio = pdata->data; > + drvdata->gpios[ASIO_DATA].flags = GPIOF_IN; > + drvdata->gpios[ASIO_DATA].label = "as_data"; ... > + drvdata->gpios[ASIO_NCE].gpio = pdata->nce; > + drvdata->gpios[ASIO_NCE].flags = GPIOF_OUT_INIT_HIGH; > + drvdata->gpios[ASIO_NCE].label = "as_nce"; > + > + dev_info(drvdata->dev, "Altera Cyclone Active Serial driver\n"); > + > + return 0; > +}
regards, -- js suse labs
| |