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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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@linaro.org>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  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
> >>