Re: [PATCH v6 02/10] rpmsg: create the rpmsg class in core instead of in rpmsg char

[Arnaud POULIQUEN]

On 11/2/21 5:38 PM, Bjorn Andersson wrote:
> On Tue 02 Nov 08:23 PDT 2021, Arnaud POULIQUEN wrote:
> 
>>
>>
>> On 11/1/21 5:57 PM, Bjorn Andersson wrote:
>>> On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:
>>>
>>>> Migrate the creation of the rpmsg class from the rpmsg_char
>>>> to the core that the class is usable by all rpmsg services.
>>>>
>>>> Suggested-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
>>>> ---
>>>>  drivers/rpmsg/rpmsg_char.c | 14 ++------------
>>>>  drivers/rpmsg/rpmsg_core.c | 26 ++++++++++++++++++++++++--
>>>>  include/linux/rpmsg.h      | 10 ++++++++++
>>>>  3 files changed, 36 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>>> index 941c5c54dd72..327ed739a3a7 100644
>>>> --- a/drivers/rpmsg/rpmsg_char.c
>>>> +++ b/drivers/rpmsg/rpmsg_char.c
>>>> @@ -28,7 +28,6 @@
>>>>  #define RPMSG_DEV_MAX	(MINORMASK + 1)
>>>>  
>>>>  static dev_t rpmsg_major;
>>>> -static struct class *rpmsg_class;
>>>>  
>>>>  static DEFINE_IDA(rpmsg_ctrl_ida);
>>>>  static DEFINE_IDA(rpmsg_ept_ida);
>>>> @@ -362,7 +361,7 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
>>>>  	init_waitqueue_head(&eptdev->readq);
>>>>  
>>>>  	device_initialize(dev);
>>>> -	dev->class = rpmsg_class;
>>>> +	dev->class = rpmsg_get_class();
>>>>  	dev->parent = parent;
>>>>  	dev->groups = rpmsg_eptdev_groups;
>>>>  	dev_set_drvdata(dev, eptdev);
>>>> @@ -482,7 +481,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>>>  	dev = &ctrldev->dev;
>>>>  	device_initialize(dev);
>>>>  	dev->parent = &rpdev->dev;
>>>> -	dev->class = rpmsg_class;
>>>> +	dev->class = rpmsg_get_class();
>>>>  
>>>>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>>>>  	ctrldev->cdev.owner = THIS_MODULE;
>>>> @@ -558,17 +557,9 @@ static int rpmsg_chrdev_init(void)
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> -	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
>>>> -	if (IS_ERR(rpmsg_class)) {
>>>> -		pr_err("failed to create rpmsg class\n");
>>>> -		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>>>> -		return PTR_ERR(rpmsg_class);
>>>> -	}
>>>> -
>>>>  	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
>>>>  	if (ret < 0) {
>>>>  		pr_err("rpmsgchr: failed to register rpmsg driver\n");
>>>> -		class_destroy(rpmsg_class);
>>>>  		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>>>>  	}
>>>>  
>>>> @@ -579,7 +570,6 @@ postcore_initcall(rpmsg_chrdev_init);
>>>>  static void rpmsg_chrdev_exit(void)
>>>>  {
>>>>  	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
>>>> -	class_destroy(rpmsg_class);
>>>>  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>>>>  }
>>>>  module_exit(rpmsg_chrdev_exit);
>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>>>> index 9151836190ce..53162038254d 100644
>>>> --- a/drivers/rpmsg/rpmsg_core.c
>>>> +++ b/drivers/rpmsg/rpmsg_core.c
>>>> @@ -20,6 +20,8 @@
>>>>  
>>>>  #include "rpmsg_internal.h"
>>>>  
>>>> +static struct class *rpmsg_class;
>>>> +
>>>>  /**
>>>>   * rpmsg_create_channel() - create a new rpmsg channel
>>>>   * using its name and address info.
>>>> @@ -296,6 +298,19 @@ __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>>>>  }
>>>>  EXPORT_SYMBOL(rpmsg_poll);
>>>>  
>>>> +/**
>>>> + * rpmsg_get_class() - get reference to the sysfs rpmsg class
>>>> + *
>>>> + * This function return the pointer to the "rpmsg" class created by the rpmsg core.
>>>> + *
>>>> + * Returns the struct class pointer
>>>> + */
>>>> +struct class *rpmsg_get_class(void)
>>>
>>> What value does this helper function add? Can't we just expose
>>> rpmsg_class directly?
>>
>> look to me cleaner to not expose directly the rpmsg_class in rpmsg.h as this
>> variable is read only for rpmsg services.
>>
> 
> The pointer is read only, but the object isn't. So I think it's cleaner
> to just share the pointer in the first place.
> 
> But that said, looking at this a little bit more, I don't think there's
> any guarantee that class_create() has been executed before
> rpmsg_ctrl_probe() is being invoked.

class_create is called in in the rpmsg_init (rpmsg_core.c). as RPMSG_CTRL and
RPMSG_CHAR depend on RPMSG config this use case should not occurs, or did I miss
something?

> 
>>>
>>>> +{
>>>> +	return rpmsg_class;
>>>> +}
>>>> +EXPORT_SYMBOL(rpmsg_get_class);
> [..]
>>>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>>>
>>> Isn't this just going to be used by rpmsg_char and rpmsg_ctrl? Do we
>>> really need to expose it in the client-facing API?
>>
>> I based this dev on hypothesis that the class could be used by some other rpmsg
>> clients. But it is not mandatory. It can be extended later, on need.
>>
> 
> That's a good hypothesis, it might be useful in other places as well.
> But I think it's best to keep it local for now and make an explicit
> decision about opening up when that need comes.
> 
>> What would you propose as an alternative to this API?
>>
>> I can see 2 alternatives:
>> - Define the rpmsg_class in rpmsg_internal.h
>>   In current patchset rpmsg_char.c does not include the rpmsg_internal.h.
>>   I'm not sure if this include makes sense for an rpmsg service driver.
>>
> 
> rpmsg_ctrl and rpmsg_char are more tightly coupled to rpmsg than typical
> rpmsg drivers, so I think it's better to include rpmsg_internal.h than to
> open up the API to the clients.

So i will declare the variable in rpmsg_internal.h and remove the
rpmsg_get_class to use directly the variable.

Thanks,
Arnaud

> 
> Thanks,
> Bjorn
> 
>> - Use "extern struct class *rpmsg_class; " in rpmsg_char and rpmsg_ctrl modules
>>
>> Regards,
>> Arnaud
>>
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> index a8dcf8a9ae88..6fe51549d931 100644
>>>> --- a/include/linux/rpmsg.h
>>>> +++ b/include/linux/rpmsg.h
>>>> @@ -186,6 +186,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
>>>>  __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>>>>  			poll_table *wait);
>>>>  
>>>> +struct class *rpmsg_get_class(void);
>>>> +
>>>>  #else
>>>>  
>>>>  static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
>>>> @@ -296,6 +298,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static inline struct class *rpmsg_get_class(void)
>>>> +{
>>>> +	/* This shouldn't be possible */
>>>> +	WARN_ON(1);
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>>  #endif /* IS_ENABLED(CONFIG_RPMSG) */
>>>>  
>>>>  /* use a macro to avoid include chaining to get THIS_MODULE */
>>>> -- 
>>>> 2.17.1
>>>>