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