| Message ID | 20211022125426.2579-3-arnaud.pouliquen@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Restructure the rpmsg_char driver and introduce rpmsg_ctrl driver. | expand |
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? > +{ > + 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? 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 >
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 >>
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 > >>
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@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. 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 >>>>
On Tue 02 Nov 12:11 CDT 2021, Arnaud POULIQUEN wrote: > 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: [..] > >>>> @@ -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? > Specifying "depends on RPMSG" in the Kconfig doesn't directly impact the load order. But in expanding my question I realized that rpmsg_ctrl_probe() will only ever be invoked if rpmsg_ctrldev_init() was able to call register_rpmsg_drive(), which in turn depends on rpmsg_init() having initialized rpmsg_bus of the driver_register() will fail. So we should be good. Thanks, Bjorn
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) +{ + 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 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 */
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(-)