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

[Jiri Slaby]

On 11/15/2010 02:40 PM, Baruch Siach wrote:
> 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.

No, it is an array of pointers.

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

Ok, citing in-place:
<cite1>
Please don't create your own class just for a single driver.  Just use
the misc class interface instead, as all you really want/need here is a
character device node, right?
</cite1>

The "miscdevice" is intended for people who want only a single device
per system (singleton) and it is designed that way. There can be only up
to 64 dynamic minors. Here you allow up to 16 devices out of box...

<cite2>
And as discussed at the Plumbers conference this past week, we don't
want to add any new 'struct class' implementations to the kernel from
now on, as it's the overall wrong thing to do.
</cite2>

Well, how do people notify udev about their character devices then? Any
pointers to the discussion?

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

How do you ensure gpios and drvdata to be in the memory before minor?
(Compilers and processors may reorder.)

regards,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/