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

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

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.

> 
>> +{
>> +	return rpmsg_class;
>> +}
>> +EXPORT_SYMBOL(rpmsg_get_class);
>> +
>>  /**
>>   * rpmsg_trysend_offchannel() - send a message using explicit src/dst addresses
>>   * @ept: the rpmsg endpoint
>> @@ -629,10 +644,17 @@ static int __init rpmsg_init(void)
>>  {
>>  	int ret;
>>  
>> +	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
>> +	if (IS_ERR(rpmsg_class)) {
>> +		pr_err("failed to create rpmsg class\n");
>> +		return PTR_ERR(rpmsg_class);
>> +	}
>> +
>>  	ret = bus_register(&rpmsg_bus);
>> -	if (ret)
>> +	if (ret) {
>>  		pr_err("failed to register rpmsg bus: %d\n", ret);
>> -
>> +		class_destroy(rpmsg_class);
>> +	}
>>  	return ret;
>>  }
>>  postcore_initcall(rpmsg_init);
>> 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.

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.

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