Re: [PATCHv3] drivers/misc: Altera active serial implementation

[Baruch Siach <baruch@tkos.co.il>]

Hi Jiri,

On Mon, Nov 15, 2010 at 11:28:02AM +0100, Jiri Slaby wrote:
> On 11/15/2010 11:08 AM, Baruch Siach wrote:
> > Thanks for reviewing. See my response to your comments below.
> > 
> > On Mon, Nov 15, 2010 at 10:33:37AM +0100, Jiri Slaby wrote:
> >> 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.
> > 
> > I use the minor field to lookup the drvdata pointer in get_as_dev(), which I 
> > use is altera_as_open().  I do this because I have no access to the 'struct 
> > device' pointer from the .open method. Do you know a better way?
> > 
> >>> +	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.
> > 
> > See above.
> 
> The answer to you previous question is here. You can just have a global
> array of struct altera_as_device.

This static array would occupy sizeof(struct altera_as_device) * AS_MAX_DEVS 
== 128 (for 32bit) * 16 == 2KB unconditionally. This seems unreasonable to me.  
IMO struct altera_as_device should be allocated per device at run time.

> >>> +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.
> > 
> > Can you explain why this is wrong? I don't mind doing test_and_set_bit 
> > instead, I'm just curious.
> 
> The mutex has an owner which it expects to unlock it. For example if you
> fork a process which already opened the device, you have a problem. This
> is a technical point. Another point is that it's ugly to leave to
> userspace with any lock.
> 
> >>> +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?
> > 
> > Up to AS_MAX_DEVS, currently 16.
> > 
> >> This doesn't look like the right way.
> > 
> > What is the right way then?
> 
> Ok, so for that count it definitely deserves its own major to not eat up
> misc device space.

A previous version of this driver[1] did just that. It was Greg KH who 
suggested[2] to use the misc class.

[1] http://article.gmane.org/gmane.linux.kernel/1057434
[2] http://article.gmane.org/gmane.linux.kernel/1058627

> >>> +		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?
> > 
> > I was thinking that the device lock which is being held during the .probe run, 
> > prevents the device from being open. After all I can still return -EGOAWAY at 
> > this point. Am I wrong?
> > 
> > If so, I'll reorder this code.
> 
> This cannot be done easily. You need to set drvdata prior to minor and
> after all the assignments here. The former because in get_as_dev you
> test minor and return drvdata.

When drvdata is not set it contains NULL. The .open method then just returns 
-ENODEV. So moving the drvdata assignment to the end of .probe should plug all 
race scenarios without any additional locking, isn't it?

> The latter because you use open_lock &
> gpios after playing with minor & drvdata.
> 
> Technically, it can be done with a use of barriers, but I don't
> recommend it as drivers should not use barriers at all. You should
> introduce some locking here (it introduces barriers on its own).
> 
> So move the assignment to altera_as_devs below the mutex_init & gpios
> setup and lock it appropriately. Then add a lock to get_as_dev.
> 
> >>> +	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

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -