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

["Indan Zupancic" <indan@nul.nu>]

Hello,

On Mon, November 15, 2010 11:28, Jiri Slaby wrote:
> On 11/15/2010 11:08 AM, Baruch Siach wrote:
>> Hi Jiri,
>>
>> 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?

Yes.

Get rid of the array and minor lookup, and instead use container_of()
to get struct altera_as_device directly from file->private_data, which
is a pointer to miscdev, set by misc_open().

Sorry for not noticing this before.

>>
>>>> +	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.
>
>>>> +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.

Ther are only as many minors allocates as needed, not always 16.
Almost always there will be only one FPGA, maybe a couple, but
more is very rare. And when that's the case there are probably
not many other misc devices around either, or they use a more
scalable way to program many FPGAs.

This is niche enough, don't make it more complicated than needed.
Having its own major seems overkill.

>>>> +		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. 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.

Just put the misc_register() call at the end and it should be fine,
once the array is gone.

>>>> +	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
>

Greetings,

Indan