Re: [RFC 1/3] drivers pps: add PPS generators support

[Rodolfo Giometti <giometti@enneenne.com>]

On 10/10/24 12:04, Greg KH wrote:
> On Thu, Oct 10, 2024 at 11:53:44AM +0200, Rodolfo Giometti wrote:
>> On 10/10/24 09:54, Greg KH wrote:
>>> On Thu, Oct 10, 2024 at 09:32:22AM +0200, Rodolfo Giometti wrote:
>>>> On 10/10/24 09:15, Greg KH wrote:
>>>>> On Wed, Oct 09, 2024 at 10:48:14AM +0200, Rodolfo Giometti wrote:
>>>>>>>> +#ifdef CONFIG_COMPAT
>>>>>>>> +static long pps_gen_cdev_compat_ioctl(struct file *file,
>>>>>>>> +		unsigned int cmd, unsigned long arg)
>>>>>>>> +{
>>>>>>>> +	cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
>>>>>>>> +	return pps_gen_cdev_ioctl(file, cmd, arg);
>>>>>>>> +}
>>>>>>>> +#else
>>>>>>>> +#define pps_gen_cdev_compat_ioctl	NULL
>>>>>>>> +#endif
>>>>>>>> +
>>>>>>>> +static struct pps_gen_device *pps_gen_idr_get(unsigned long id)
>>>>>>>> +{
>>>>>>>> +	struct pps_gen_device *pps_gen;
>>>>>>>> +
>>>>>>>> +	mutex_lock(&pps_gen_idr_lock);
>>>>>>>> +	pps_gen = idr_find(&pps_gen_idr, id);
>>>>>>>> +	if (pps_gen)
>>>>>>>> +		kobject_get(&pps_gen->dev->kobj);
>>>>>>>> +
>>>>>>>> +	mutex_unlock(&pps_gen_idr_lock);
>>>>>>>
>>>>>>> Doesn't an idr have a lock in it?  I can never remember...
>>>>>>
>>>>>> As far as I know we must use a mutex...
>>>>>
>>>>> If you do, someone will come along and remove it, please see:
>>>>> 	https://lore.kernel.org/r/b1fcc6707ec2b6309d50060fa52ccc2c892afde2.1728507153.git.christophe.jaillet@wanadoo.fr
>>>>> as an example (with links that show it is not needed).
>>>>
>>>> Here is an example about ida API, but I'm using idr API.
>>>
>>> Why not use ida then?  :)
>>
>> Because we need an ID associated with a pointer.
> 
> Ah, ok, but why?  Why is the "id" here the mapping to the pointer?  Why
> not use the structures you already have to store this in (i.e. the
> normal driver model stuff?)
> 
> All you should need an idr/ida for is to pick a unique "number" for
> naming your class device.  Everything after that should already be there
> for you to get access to the structures you need to get access to.

OK, let me review the code and then I'm going to propose a new patchset.

Thanks for your suggestions. :)

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming