| Message ID | 20210413134458.17912-6-arnaud.pouliquen@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Restructure the rpmsg char and introduce the rpmsg-raw channel | expand |
Good day Arnaud, On Tue, Apr 13, 2021 at 03:44:56PM +0200, Arnaud Pouliquen wrote: > A rpmsg char device allows to probe the endpoint device on a remote name > service announcement. > > With this patch the /dev/rpmsgX interface is created either by a user > application or by the remote firmware. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > > --- > update from V1: > - add missing unregister_rpmsg_driver call on module exit. > > --- > drivers/rpmsg/rpmsg_char.c | 59 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 58 insertions(+), 1 deletion(-) > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > index a64249d83172..4606787b7011 100644 > --- a/drivers/rpmsg/rpmsg_char.c > +++ b/drivers/rpmsg/rpmsg_char.c > @@ -26,6 +26,8 @@ > #include "rpmsg_char.h" > #include "rpmsg_internal.h" > > +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" > + Why not simply call it rpmsg-char? > static dev_t rpmsg_major; > > static DEFINE_IDA(rpmsg_ept_ida); > @@ -403,13 +405,67 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent > } > EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev); > > +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) > +{ > + struct rpmsg_channel_info chinfo; > + struct rpmsg_eptdev *eptdev; > + > + if (!rpdev->ept) > + return -EINVAL; > + > + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); > + chinfo.src = rpdev->src; > + chinfo.dst = rpdev->dst; > + > + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo, NULL); > + if (IS_ERR(eptdev)) > + return PTR_ERR(eptdev); > + > + /* Set the private field of the default endpoint to retrieve context on callback. */ > + rpdev->ept->priv = eptdev; This is already done in rpmsg_create_ept() when rpmsg_eptdev_open() is called. > + > + return 0; > +} > + > +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) > +{ > + int ret; > + > + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_destroy_eptdev); > + if (ret) > + dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret); > +} > + > +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = { > + { .name = RPMSG_CHAR_DEVNAME }, > + { }, > +}; > + > +static struct rpmsg_driver rpmsg_chrdev_driver = { > + .probe = rpmsg_chrdev_probe, > + .remove = rpmsg_chrdev_remove, > + .id_table = rpmsg_chrdev_id_table, > + .callback = rpmsg_ept_cb, Not sure why we need a callback associated to this driver when rpmsg_eptdev_open() already creates and rpmsg_endpoint. To me the only thing having a callback provides is the association between the rpmsg_device and the rpmsg_endpoint[1] that happens in rpmsg_dev_probe(). The QC folks already do this association in their platform code[2]. Since this is not done in __rpmsg_create_ept() a check for rpdev->ept == NULL could be done in rpmsg_eptdev_open() and do the assignment there. [1]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/rpmsg_core.c#L513 [2]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/qcom_glink_native.c#L1623 > + .drv = { > + .name = "rpmsg_chrdev", > + }, > +}; > + > static int rpmsg_chrdev_init(void) > { > int ret; > > ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_char"); > - if (ret < 0) > + if (ret < 0) { > pr_err("rpmsg: failed to allocate char dev region\n"); > + return ret; > + } > + > + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); > + if (ret < 0) { > + pr_err("rpmsg: failed to register rpmsg raw driver\n"); > + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); > + } > > return ret; > } > @@ -417,6 +473,7 @@ postcore_initcall(rpmsg_chrdev_init); > > static void rpmsg_chrdev_exit(void) > { > + unregister_rpmsg_driver(&rpmsg_chrdev_driver); > unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); > } > module_exit(rpmsg_chrdev_exit); > -- > 2.17.1 >
On 4/21/21 7:40 PM, Mathieu Poirier wrote: > Good day Arnaud, > > On Tue, Apr 13, 2021 at 03:44:56PM +0200, Arnaud Pouliquen wrote: >> A rpmsg char device allows to probe the endpoint device on a remote name >> service announcement. >> >> With this patch the /dev/rpmsgX interface is created either by a user >> application or by the remote firmware. >> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> >> >> --- >> update from V1: >> - add missing unregister_rpmsg_driver call on module exit. >> >> --- >> drivers/rpmsg/rpmsg_char.c | 59 +++++++++++++++++++++++++++++++++++++- >> 1 file changed, 58 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >> index a64249d83172..4606787b7011 100644 >> --- a/drivers/rpmsg/rpmsg_char.c >> +++ b/drivers/rpmsg/rpmsg_char.c >> @@ -26,6 +26,8 @@ >> #include "rpmsg_char.h" >> #include "rpmsg_internal.h" >> >> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" >> + > > Why not simply call it rpmsg-char? I would avoid to link the rpmsg name service to the Linux Kernel device. > >> static dev_t rpmsg_major; >> >> static DEFINE_IDA(rpmsg_ept_ida); >> @@ -403,13 +405,67 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent >> } >> EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev); >> >> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) >> +{ >> + struct rpmsg_channel_info chinfo; >> + struct rpmsg_eptdev *eptdev; >> + >> + if (!rpdev->ept) >> + return -EINVAL; >> + >> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); >> + chinfo.src = rpdev->src; >> + chinfo.dst = rpdev->dst; >> + >> + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo, NULL); >> + if (IS_ERR(eptdev)) >> + return PTR_ERR(eptdev); >> + >> + /* Set the private field of the default endpoint to retrieve context on callback. */ >> + rpdev->ept->priv = eptdev; > > This is already done in rpmsg_create_ept() when rpmsg_eptdev_open() is called. > >> + >> + return 0; >> +} >> + >> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) >> +{ >> + int ret; >> + >> + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_destroy_eptdev); >> + if (ret) >> + dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret); >> +} >> + >> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = { >> + { .name = RPMSG_CHAR_DEVNAME }, >> + { }, >> +}; >> + >> +static struct rpmsg_driver rpmsg_chrdev_driver = { >> + .probe = rpmsg_chrdev_probe, >> + .remove = rpmsg_chrdev_remove, >> + .id_table = rpmsg_chrdev_id_table, >> + .callback = rpmsg_ept_cb, > > Not sure why we need a callback associated to this driver when > rpmsg_eptdev_open() already creates and rpmsg_endpoint. To me the only thing > having a callback provides is the association between the rpmsg_device and the > rpmsg_endpoint[1] that happens in rpmsg_dev_probe(). The QC folks already do > this association in their platform code[2]. Since this is not done in > __rpmsg_create_ept() a check for rpdev->ept == NULL could be done in > rpmsg_eptdev_open() and do the assignment there. > > [1]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/rpmsg_core.c#L513 > [2]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/qcom_glink_native.c#L1623 > That's a good point! When I started the redesign, I faced some issues with the approach you propose. But as I can not remember the reason and because the code has evolved, i need to re-think about this. Thanks, Arnaud >> + .drv = { >> + .name = "rpmsg_chrdev", >> + }, >> +}; >> + >> static int rpmsg_chrdev_init(void) >> { >> int ret; >> >> ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_char"); >> - if (ret < 0) >> + if (ret < 0) { >> pr_err("rpmsg: failed to allocate char dev region\n"); >> + return ret; >> + } >> + >> + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); >> + if (ret < 0) { >> + pr_err("rpmsg: failed to register rpmsg raw driver\n"); >> + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); >> + } >> >> return ret; >> } >> @@ -417,6 +473,7 @@ postcore_initcall(rpmsg_chrdev_init); >> >> static void rpmsg_chrdev_exit(void) >> { >> + unregister_rpmsg_driver(&rpmsg_chrdev_driver); >> unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); >> } >> module_exit(rpmsg_chrdev_exit); >> -- >> 2.17.1 >>
On Thu, Apr 22, 2021 at 09:58:27AM +0200, Arnaud POULIQUEN wrote: > On 4/21/21 7:40 PM, Mathieu Poirier wrote: > > Good day Arnaud, > > > > On Tue, Apr 13, 2021 at 03:44:56PM +0200, Arnaud Pouliquen wrote: > >> A rpmsg char device allows to probe the endpoint device on a remote name > >> service announcement. > >> > >> With this patch the /dev/rpmsgX interface is created either by a user > >> application or by the remote firmware. > >> > >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > >> > >> --- > >> update from V1: > >> - add missing unregister_rpmsg_driver call on module exit. > >> > >> --- > >> drivers/rpmsg/rpmsg_char.c | 59 +++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 58 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > >> index a64249d83172..4606787b7011 100644 > >> --- a/drivers/rpmsg/rpmsg_char.c > >> +++ b/drivers/rpmsg/rpmsg_char.c > >> @@ -26,6 +26,8 @@ > >> #include "rpmsg_char.h" > >> #include "rpmsg_internal.h" > >> > >> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" > >> + > > > > Why not simply call it rpmsg-char? > > I would avoid to link the rpmsg name service to the Linux Kernel device. To me that's exactly what we want to do... Am I missing something? > > > > >> static dev_t rpmsg_major; > >> > >> static DEFINE_IDA(rpmsg_ept_ida); > >> @@ -403,13 +405,67 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent > >> } > >> EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev); > >> > >> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) > >> +{ > >> + struct rpmsg_channel_info chinfo; > >> + struct rpmsg_eptdev *eptdev; > >> + > >> + if (!rpdev->ept) > >> + return -EINVAL; > >> + > >> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); > >> + chinfo.src = rpdev->src; > >> + chinfo.dst = rpdev->dst; > >> + > >> + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo, NULL); > >> + if (IS_ERR(eptdev)) > >> + return PTR_ERR(eptdev); > >> + > >> + /* Set the private field of the default endpoint to retrieve context on callback. */ > >> + rpdev->ept->priv = eptdev; > > > > This is already done in rpmsg_create_ept() when rpmsg_eptdev_open() is called. > > > >> + > >> + return 0; > >> +} > >> + > >> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) > >> +{ > >> + int ret; > >> + > >> + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_destroy_eptdev); > >> + if (ret) > >> + dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret); > >> +} > >> + > >> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = { > >> + { .name = RPMSG_CHAR_DEVNAME }, > >> + { }, > >> +}; > >> + > >> +static struct rpmsg_driver rpmsg_chrdev_driver = { > >> + .probe = rpmsg_chrdev_probe, > >> + .remove = rpmsg_chrdev_remove, > >> + .id_table = rpmsg_chrdev_id_table, > >> + .callback = rpmsg_ept_cb, > > > > Not sure why we need a callback associated to this driver when > > rpmsg_eptdev_open() already creates and rpmsg_endpoint. To me the only thing > > having a callback provides is the association between the rpmsg_device and the > > rpmsg_endpoint[1] that happens in rpmsg_dev_probe(). The QC folks already do > > this association in their platform code[2]. Since this is not done in > > __rpmsg_create_ept() a check for rpdev->ept == NULL could be done in > > rpmsg_eptdev_open() and do the assignment there. > > > > [1]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/rpmsg_core.c#L513 > > [2]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/qcom_glink_native.c#L1623 > > > > That's a good point! When I started the redesign, I faced some issues with the > approach you propose. But as I can not remember the reason and because the code > has evolved, i need to re-think about this. > Glad to see we're on the same page. I stared at this code for a very long time, thinking there was some kind of bigger picture I wasn't getting. > Thanks, > Arnaud > > > >> + .drv = { > >> + .name = "rpmsg_chrdev", > >> + }, > >> +}; > >> + > >> static int rpmsg_chrdev_init(void) > >> { > >> int ret; > >> > >> ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_char"); > >> - if (ret < 0) > >> + if (ret < 0) { > >> pr_err("rpmsg: failed to allocate char dev region\n"); > >> + return ret; > >> + } > >> + > >> + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); > >> + if (ret < 0) { > >> + pr_err("rpmsg: failed to register rpmsg raw driver\n"); > >> + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); > >> + } > >> > >> return ret; > >> } > >> @@ -417,6 +473,7 @@ postcore_initcall(rpmsg_chrdev_init); > >> > >> static void rpmsg_chrdev_exit(void) > >> { > >> + unregister_rpmsg_driver(&rpmsg_chrdev_driver); > >> unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); > >> } > >> module_exit(rpmsg_chrdev_exit); > >> -- > >> 2.17.1 > >>
On 4/22/21 6:36 PM, Mathieu Poirier wrote: > On Thu, Apr 22, 2021 at 09:58:27AM +0200, Arnaud POULIQUEN wrote: >> On 4/21/21 7:40 PM, Mathieu Poirier wrote: >>> Good day Arnaud, >>> >>> On Tue, Apr 13, 2021 at 03:44:56PM +0200, Arnaud Pouliquen wrote: >>>> A rpmsg char device allows to probe the endpoint device on a remote name >>>> service announcement. >>>> >>>> With this patch the /dev/rpmsgX interface is created either by a user >>>> application or by the remote firmware. >>>> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> >>>> >>>> --- >>>> update from V1: >>>> - add missing unregister_rpmsg_driver call on module exit. >>>> >>>> --- >>>> drivers/rpmsg/rpmsg_char.c | 59 +++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 58 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >>>> index a64249d83172..4606787b7011 100644 >>>> --- a/drivers/rpmsg/rpmsg_char.c >>>> +++ b/drivers/rpmsg/rpmsg_char.c >>>> @@ -26,6 +26,8 @@ >>>> #include "rpmsg_char.h" >>>> #include "rpmsg_internal.h" >>>> >>>> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" >>>> + >>> >>> Why not simply call it rpmsg-char? >> >> I would avoid to link the rpmsg name service to the Linux Kernel device. > > To me that's exactly what we want to do... Am I missing something? A discussion started about a service layer in the OpenAMP library. "rpmsg-char" doesn't really make sense in OpenAMP, especially for OpenAMP<->openAMP communication. That's why I think a generic name would be more suitable. Regards, Arnaud > >> >>> >>>> static dev_t rpmsg_major; >>>> >>>> static DEFINE_IDA(rpmsg_ept_ida); >>>> @@ -403,13 +405,67 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent >>>> } >>>> EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev); >>>> >>>> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) >>>> +{ >>>> + struct rpmsg_channel_info chinfo; >>>> + struct rpmsg_eptdev *eptdev; >>>> + >>>> + if (!rpdev->ept) >>>> + return -EINVAL; >>>> + >>>> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); >>>> + chinfo.src = rpdev->src; >>>> + chinfo.dst = rpdev->dst; >>>> + >>>> + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo, NULL); >>>> + if (IS_ERR(eptdev)) >>>> + return PTR_ERR(eptdev); >>>> + >>>> + /* Set the private field of the default endpoint to retrieve context on callback. */ >>>> + rpdev->ept->priv = eptdev; >>> >>> This is already done in rpmsg_create_ept() when rpmsg_eptdev_open() is called. >>> >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_destroy_eptdev); >>>> + if (ret) >>>> + dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret); >>>> +} >>>> + >>>> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = { >>>> + { .name = RPMSG_CHAR_DEVNAME }, >>>> + { }, >>>> +}; >>>> + >>>> +static struct rpmsg_driver rpmsg_chrdev_driver = { >>>> + .probe = rpmsg_chrdev_probe, >>>> + .remove = rpmsg_chrdev_remove, >>>> + .id_table = rpmsg_chrdev_id_table, >>>> + .callback = rpmsg_ept_cb, >>> >>> Not sure why we need a callback associated to this driver when >>> rpmsg_eptdev_open() already creates and rpmsg_endpoint. To me the only thing >>> having a callback provides is the association between the rpmsg_device and the >>> rpmsg_endpoint[1] that happens in rpmsg_dev_probe(). The QC folks already do >>> this association in their platform code[2]. Since this is not done in >>> __rpmsg_create_ept() a check for rpdev->ept == NULL could be done in >>> rpmsg_eptdev_open() and do the assignment there. >>> >>> [1]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/rpmsg_core.c#L513 >>> [2]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/qcom_glink_native.c#L1623 >>> >> >> That's a good point! When I started the redesign, I faced some issues with the >> approach you propose. But as I can not remember the reason and because the code >> has evolved, i need to re-think about this. >> > > Glad to see we're on the same page. I stared at this code for a very long time, > thinking there was some kind of bigger picture I wasn't getting. > > >> Thanks, >> Arnaud >> >> >>>> + .drv = { >>>> + .name = "rpmsg_chrdev", >>>> + }, >>>> +}; >>>> + >>>> static int rpmsg_chrdev_init(void) >>>> { >>>> int ret; >>>> >>>> ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_char"); >>>> - if (ret < 0) >>>> + if (ret < 0) { >>>> pr_err("rpmsg: failed to allocate char dev region\n"); >>>> + return ret; >>>> + } >>>> + >>>> + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); >>>> + if (ret < 0) { >>>> + pr_err("rpmsg: failed to register rpmsg raw driver\n"); >>>> + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); >>>> + } >>>> >>>> return ret; >>>> } >>>> @@ -417,6 +473,7 @@ postcore_initcall(rpmsg_chrdev_init); >>>> >>>> static void rpmsg_chrdev_exit(void) >>>> { >>>> + unregister_rpmsg_driver(&rpmsg_chrdev_driver); >>>> unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); >>>> } >>>> module_exit(rpmsg_chrdev_exit); >>>> -- >>>> 2.17.1 >>>>
Hi, On 4/22/21 6:36 PM, Mathieu Poirier wrote: > On Thu, Apr 22, 2021 at 09:58:27AM +0200, Arnaud POULIQUEN wrote: >> On 4/21/21 7:40 PM, Mathieu Poirier wrote: >>> Good day Arnaud, >>> >>> On Tue, Apr 13, 2021 at 03:44:56PM +0200, Arnaud Pouliquen wrote: >>>> A rpmsg char device allows to probe the endpoint device on a remote name >>>> service announcement. >>>> >>>> With this patch the /dev/rpmsgX interface is created either by a user >>>> application or by the remote firmware. >>>> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> >>>> >>>> --- >>>> update from V1: >>>> - add missing unregister_rpmsg_driver call on module exit. >>>> >>>> --- >>>> drivers/rpmsg/rpmsg_char.c | 59 +++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 58 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >>>> index a64249d83172..4606787b7011 100644 >>>> --- a/drivers/rpmsg/rpmsg_char.c >>>> +++ b/drivers/rpmsg/rpmsg_char.c >>>> @@ -26,6 +26,8 @@ >>>> #include "rpmsg_char.h" >>>> #include "rpmsg_internal.h" >>>> >>>> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" >>>> + >>> >>> Why not simply call it rpmsg-char? >> >> I would avoid to link the rpmsg name service to the Linux Kernel device. > > To me that's exactly what we want to do... Am I missing something? > >> >>> >>>> static dev_t rpmsg_major; >>>> >>>> static DEFINE_IDA(rpmsg_ept_ida); >>>> @@ -403,13 +405,67 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent >>>> } >>>> EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev); >>>> >>>> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) >>>> +{ >>>> + struct rpmsg_channel_info chinfo; >>>> + struct rpmsg_eptdev *eptdev; >>>> + >>>> + if (!rpdev->ept) >>>> + return -EINVAL; >>>> + >>>> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); >>>> + chinfo.src = rpdev->src; >>>> + chinfo.dst = rpdev->dst; >>>> + >>>> + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo, NULL); >>>> + if (IS_ERR(eptdev)) >>>> + return PTR_ERR(eptdev); >>>> + >>>> + /* Set the private field of the default endpoint to retrieve context on callback. */ >>>> + rpdev->ept->priv = eptdev; >>> >>> This is already done in rpmsg_create_ept() when rpmsg_eptdev_open() is called. >>> >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_destroy_eptdev); >>>> + if (ret) >>>> + dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret); >>>> +} >>>> + >>>> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = { >>>> + { .name = RPMSG_CHAR_DEVNAME }, >>>> + { }, >>>> +}; >>>> + >>>> +static struct rpmsg_driver rpmsg_chrdev_driver = { >>>> + .probe = rpmsg_chrdev_probe, >>>> + .remove = rpmsg_chrdev_remove, >>>> + .id_table = rpmsg_chrdev_id_table, >>>> + .callback = rpmsg_ept_cb, >>> >>> Not sure why we need a callback associated to this driver when >>> rpmsg_eptdev_open() already creates and rpmsg_endpoint. To me the only thing >>> having a callback provides is the association between the rpmsg_device and the >>> rpmsg_endpoint[1] that happens in rpmsg_dev_probe(). The QC folks already do >>> this association in their platform code[2]. Since this is not done in >>> __rpmsg_create_ept() a check for rpdev->ept == NULL could be done in >>> rpmsg_eptdev_open() and do the assignment there. >>> >>> [1]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/rpmsg_core.c#L513 >>> [2]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/qcom_glink_native.c#L1623 >>> >> >> That's a good point! When I started the redesign, I faced some issues with the >> approach you propose. But as I can not remember the reason and because the code >> has evolved, i need to re-think about this. >> > > Glad to see we're on the same page. I stared at this code for a very long time, > thinking there was some kind of bigger picture I wasn't getting. I finally found the time to investigate this. If I remember now why I used this approach, I also saw that my patchset does not work with the QCOM platform driver. As a first step of explanation, let's ignore the QC platform. rpdev->ept is null for the rpmsg ctrldev device created by the virtio rpmsg bus. If no default endpoint is created on rpmsg_chrdev_probe, it is not possible to differentiate the two in rpmsg_eptdev_open based on rpdev->ept == NULL. Now let's add the QC implementation As you mentioned, QC sets the rpdev->ept to a default endpoint before registering the rpmsg ctrldev. This shows that it is not reasonable to expect to handle all use cases based on the rpdev->ept value. So, to summarize, I need to rework this, probably by adding a new field in the rpmsg_eptdev structure, to properly handle the endpoint creation in the rpmsg_eptdev_open function. Regards, Arnaud > > >> Thanks, >> Arnaud >> >> >>>> + .drv = { >>>> + .name = "rpmsg_chrdev", >>>> + }, >>>> +}; >>>> + >>>> static int rpmsg_chrdev_init(void) >>>> { >>>> int ret; >>>> >>>> ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_char"); >>>> - if (ret < 0) >>>> + if (ret < 0) { >>>> pr_err("rpmsg: failed to allocate char dev region\n"); >>>> + return ret; >>>> + } >>>> + >>>> + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); >>>> + if (ret < 0) { >>>> + pr_err("rpmsg: failed to register rpmsg raw driver\n"); >>>> + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); >>>> + } >>>> >>>> return ret; >>>> } >>>> @@ -417,6 +473,7 @@ postcore_initcall(rpmsg_chrdev_init); >>>> >>>> static void rpmsg_chrdev_exit(void) >>>> { >>>> + unregister_rpmsg_driver(&rpmsg_chrdev_driver); >>>> unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); >>>> } >>>> module_exit(rpmsg_chrdev_exit); >>>> -- >>>> 2.17.1 >>>>
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c index a64249d83172..4606787b7011 100644 --- a/drivers/rpmsg/rpmsg_char.c +++ b/drivers/rpmsg/rpmsg_char.c @@ -26,6 +26,8 @@ #include "rpmsg_char.h" #include "rpmsg_internal.h" +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" + static dev_t rpmsg_major; static DEFINE_IDA(rpmsg_ept_ida); @@ -403,13 +405,67 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent } EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev); +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) +{ + struct rpmsg_channel_info chinfo; + struct rpmsg_eptdev *eptdev; + + if (!rpdev->ept) + return -EINVAL; + + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); + chinfo.src = rpdev->src; + chinfo.dst = rpdev->dst; + + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo, NULL); + if (IS_ERR(eptdev)) + return PTR_ERR(eptdev); + + /* Set the private field of the default endpoint to retrieve context on callback. */ + rpdev->ept->priv = eptdev; + + return 0; +} + +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) +{ + int ret; + + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_destroy_eptdev); + if (ret) + dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret); +} + +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = { + { .name = RPMSG_CHAR_DEVNAME }, + { }, +}; + +static struct rpmsg_driver rpmsg_chrdev_driver = { + .probe = rpmsg_chrdev_probe, + .remove = rpmsg_chrdev_remove, + .id_table = rpmsg_chrdev_id_table, + .callback = rpmsg_ept_cb, + .drv = { + .name = "rpmsg_chrdev", + }, +}; + static int rpmsg_chrdev_init(void) { int ret; ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_char"); - if (ret < 0) + if (ret < 0) { pr_err("rpmsg: failed to allocate char dev region\n"); + return ret; + } + + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); + if (ret < 0) { + pr_err("rpmsg: failed to register rpmsg raw driver\n"); + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); + } return ret; } @@ -417,6 +473,7 @@ postcore_initcall(rpmsg_chrdev_init); static void rpmsg_chrdev_exit(void) { + unregister_rpmsg_driver(&rpmsg_chrdev_driver); unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); } module_exit(rpmsg_chrdev_exit);
A rpmsg char device allows to probe the endpoint device on a remote name service announcement. With this patch the /dev/rpmsgX interface is created either by a user application or by the remote firmware. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> --- update from V1: - add missing unregister_rpmsg_driver call on module exit. --- drivers/rpmsg/rpmsg_char.c | 59 +++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-)