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