| Message ID | 20210429135507.8264-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 |
Hi Arnaud, On Thu, Apr 29, 2021 at 03:55:06PM +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 | 53 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 52 insertions(+), 1 deletion(-) > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > index 5c6a7da6e4d7..9166454c1310 100644 > --- a/drivers/rpmsg/rpmsg_char.c > +++ b/drivers/rpmsg/rpmsg_char.c > @@ -18,6 +18,8 @@ > > #include "rpmsg_char.h" > > +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" > + > static dev_t rpmsg_major; > static struct class *rpmsg_class; > > @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent > } > EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create); > > +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) > +{ > + struct rpmsg_channel_info chinfo; > + > + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); > + chinfo.src = rpdev->src; > + chinfo.dst = rpdev->dst; > + > + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true); > +} > + > +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) > +{ > + int ret; > + > + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy); > + 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, > + .drv = { > + .name = "rpmsg_chrdev", > + }, > +}; The sole purpose of doing this is to create instances of rpmsg_chrdevs from the name service - but is it really needed? Up to now and aside from GLINK and SMD, there asn't been other users of it so I'm wondering if it is worth going through all this trouble. As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name service. That way patches 4, 5 and 6 of this set can be dropped. Thanks, Mathieu > + > static int rpmsg_chrdev_init(void) > { > int ret; > @@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void) > 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 = PTR_ERR(rpmsg_class); > + goto free_region; > + } > + > + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); > + if (ret < 0) { > + pr_err("rpmsg: failed to register rpmsg raw driver\n"); > + goto free_class; > } > > return 0; > + > +free_class: > + class_destroy(rpmsg_class); > +free_region: > + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); > + > + return ret; > } > 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); > } > -- > 2.17.1 >
Hi Mathieu, On 5/5/21 6:41 PM, Mathieu Poirier wrote: > Hi Arnaud, > > On Thu, Apr 29, 2021 at 03:55:06PM +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 | 53 +++++++++++++++++++++++++++++++++++++- >> 1 file changed, 52 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >> index 5c6a7da6e4d7..9166454c1310 100644 >> --- a/drivers/rpmsg/rpmsg_char.c >> +++ b/drivers/rpmsg/rpmsg_char.c >> @@ -18,6 +18,8 @@ >> >> #include "rpmsg_char.h" >> >> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" >> + >> static dev_t rpmsg_major; >> static struct class *rpmsg_class; >> >> @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent >> } >> EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create); >> >> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) >> +{ >> + struct rpmsg_channel_info chinfo; >> + >> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); >> + chinfo.src = rpdev->src; >> + chinfo.dst = rpdev->dst; >> + >> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true); >> +} >> + >> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) >> +{ >> + int ret; >> + >> + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy); >> + 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, >> + .drv = { >> + .name = "rpmsg_chrdev", >> + }, >> +}; > > The sole purpose of doing this is to create instances of rpmsg_chrdevs from the > name service - but is it really needed? Up to now and aside from GLINK and SMD, > there asn't been other users of it so I'm wondering if it is worth going through > all this trouble. It is a good point. Just as a reminder, the need of ST and, I assume, some other companies, is to have a basic/generic communication channel to control a remote processor application. Nothing generic exists today for a virtio transport based implementation. Companies have to create their own driver. The purpose of my work is to allow our customer to use RPMsg without developing a specific driver to control remote applications. The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple inter-processor link to send and receive data. The rpmsg_tty is another one. Focusing on the rpmsg_chrdev: We did a part of the work with the first patch set that would be in 5.13. But is it simple to use it for virtio transport based platforms? If we don't implement the NS announcement support in rpmsg_chrdev, using rpmsg_chrdev for a user application seems rather tricky. How to instantiate the communication? The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to determine the services and associated remote address. I don't think the QCOM drivers have the same problem because they seems to initiate the communication and work directly with the RPMsg endpoints ( new channel creation on endpoint creation) while Virtio works with the RPMsg channel. By introducing the ability to instantiate rpmsg_chrdevs through the NS announcement, we make this easy for applications to use. And without rpmsg_chrdevs instantiation, It also means that we can't create an RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control, right? That said, If we consider that the aim was only to extract the rpmsg_ctrl part, I'm not against leaving the rpmsg_char in this state and switching to the rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg channels. We could come back on this if requested by someone else. Thanks, Arnaud > > As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name > service. That way patches 4, 5 and 6 of this set can be dropped. > > Thanks, > Mathieu > >> + >> static int rpmsg_chrdev_init(void) >> { >> int ret; >> @@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void) >> 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 = PTR_ERR(rpmsg_class); >> + goto free_region; >> + } >> + >> + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); >> + if (ret < 0) { >> + pr_err("rpmsg: failed to register rpmsg raw driver\n"); >> + goto free_class; >> } >> >> return 0; >> + >> +free_class: >> + class_destroy(rpmsg_class); >> +free_region: >> + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); >> + >> + return ret; >> } >> 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); >> } >> -- >> 2.17.1 >>
Good day, On Wed, May 05, 2021 at 08:25:24PM +0200, Arnaud POULIQUEN wrote: > Hi Mathieu, > > On 5/5/21 6:41 PM, Mathieu Poirier wrote: > > Hi Arnaud, > > > > On Thu, Apr 29, 2021 at 03:55:06PM +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 | 53 +++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 52 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > >> index 5c6a7da6e4d7..9166454c1310 100644 > >> --- a/drivers/rpmsg/rpmsg_char.c > >> +++ b/drivers/rpmsg/rpmsg_char.c > >> @@ -18,6 +18,8 @@ > >> > >> #include "rpmsg_char.h" > >> > >> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" > >> + > >> static dev_t rpmsg_major; > >> static struct class *rpmsg_class; > >> > >> @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent > >> } > >> EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create); > >> > >> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) > >> +{ > >> + struct rpmsg_channel_info chinfo; > >> + > >> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); > >> + chinfo.src = rpdev->src; > >> + chinfo.dst = rpdev->dst; > >> + > >> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true); > >> +} > >> + > >> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) > >> +{ > >> + int ret; > >> + > >> + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy); > >> + 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, > >> + .drv = { > >> + .name = "rpmsg_chrdev", > >> + }, > >> +}; > > > > The sole purpose of doing this is to create instances of rpmsg_chrdevs from the > > name service - but is it really needed? Up to now and aside from GLINK and SMD, > > there asn't been other users of it so I'm wondering if it is worth going through > > all this trouble. > > It is a good point. > > Just as a reminder, the need of ST and, I assume, some other companies, is to > have a basic/generic communication channel to control a remote processor > application. > > Nothing generic exists today for a virtio transport based implementation. > Companies have to create their own driver. > > The purpose of my work is to allow our customer to use RPMsg without developing > a specific driver to control remote applications. > > The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple > inter-processor link to send and receive data. The rpmsg_tty is another one. > > Focusing on the rpmsg_chrdev: > We did a part of the work with the first patch set that would be in 5.13. > But is it simple to use it for virtio transport based platforms? > If we don't implement the NS announcement support in rpmsg_chrdev, using > rpmsg_chrdev for a user application seems rather tricky. > How to instantiate the communication? Since we already have /dev/rpmsg_ctrlX user space can instantiate an rpmsg_chrdev using that interface, which is how things are done in the GLINK/SMD world. Wouldn't that cover the usecases you had in mind? As you pointed out above rpmsg_chrdev should be light and simple - eliminating patches 4, 5 and 6 would yield that. > The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to > determine the services and associated remote address. > > I don't think the QCOM drivers have the same problem because they seems to > initiate the communication and work directly with the RPMsg endpoints ( new > channel creation on endpoint creation) while Virtio works with the RPMsg channel. > > By introducing the ability to instantiate rpmsg_chrdevs through the NS > announcement, we make this easy for applications to use. > > And without rpmsg_chrdevs instantiation, It also means that we can't create an > RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control, > right? > > That said, If we consider that the aim was only to extract the rpmsg_ctrl part, > I'm not against leaving the rpmsg_char in this state and switching to the > rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg > channels. > > We could come back on this if requested by someone else. > > Thanks, > Arnaud > > > > > As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name > > service. That way patches 4, 5 and 6 of this set can be dropped. > > > > Thanks, > > Mathieu > > > >> + > >> static int rpmsg_chrdev_init(void) > >> { > >> int ret; > >> @@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void) > >> 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 = PTR_ERR(rpmsg_class); > >> + goto free_region; > >> + } > >> + > >> + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); > >> + if (ret < 0) { > >> + pr_err("rpmsg: failed to register rpmsg raw driver\n"); > >> + goto free_class; > >> } > >> > >> return 0; > >> + > >> +free_class: > >> + class_destroy(rpmsg_class); > >> +free_region: > >> + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); > >> + > >> + return ret; > >> } > >> 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); > >> } > >> -- > >> 2.17.1 > >>
Hi Mathieu, Arnaud, On 5/5/21 8:25 PM, Arnaud POULIQUEN wrote: > Hi Mathieu, > > On 5/5/21 6:41 PM, Mathieu Poirier wrote: >> Hi Arnaud, >> >> On Thu, Apr 29, 2021 at 03:55:06PM +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 | 53 +++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 52 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >>> index 5c6a7da6e4d7..9166454c1310 100644 >>> --- a/drivers/rpmsg/rpmsg_char.c >>> +++ b/drivers/rpmsg/rpmsg_char.c >>> @@ -18,6 +18,8 @@ >>> >>> #include "rpmsg_char.h" >>> >>> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" >>> + >>> static dev_t rpmsg_major; >>> static struct class *rpmsg_class; >>> >>> @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent >>> } >>> EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create); >>> >>> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) >>> +{ >>> + struct rpmsg_channel_info chinfo; >>> + >>> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); >>> + chinfo.src = rpdev->src; >>> + chinfo.dst = rpdev->dst; >>> + >>> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true); >>> +} >>> + >>> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) >>> +{ >>> + int ret; >>> + >>> + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy); >>> + 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, >>> + .drv = { >>> + .name = "rpmsg_chrdev", >>> + }, >>> +}; >> >> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the >> name service - but is it really needed? Up to now and aside from GLINK and SMD, >> there asn't been other users of it so I'm wondering if it is worth going through >> all this trouble. > > It is a good point. > > Just as a reminder, the need of ST and, I assume, some other companies, is to > have a basic/generic communication channel to control a remote processor > application. > > Nothing generic exists today for a virtio transport based implementation. > Companies have to create their own driver. > > The purpose of my work is to allow our customer to use RPMsg without developing > a specific driver to control remote applications. > > The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple > inter-processor link to send and receive data. The rpmsg_tty is another one. > > Focusing on the rpmsg_chrdev: > We did a part of the work with the first patch set that would be in 5.13. > But is it simple to use it for virtio transport based platforms? > If we don't implement the NS announcement support in rpmsg_chrdev, using > rpmsg_chrdev for a user application seems rather tricky. > How to instantiate the communication? > The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to > determine the services and associated remote address. > > I don't think the QCOM drivers have the same problem because they seems to > initiate the communication and work directly with the RPMsg endpoints ( new > channel creation on endpoint creation) while Virtio works with the RPMsg channel. > > By introducing the ability to instantiate rpmsg_chrdevs through the NS > announcement, we make this easy for applications to use. > > And without rpmsg_chrdevs instantiation, It also means that we can't create an > RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control, > right? > > That said, If we consider that the aim was only to extract the rpmsg_ctrl part, > I'm not against leaving the rpmsg_char in this state and switching to the > rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg > channels. > > We could come back on this if requested by someone else. I'm personnaly following this thread, our project is to be able to do RPC call from Linux to an RTOS (Zephyr). Our plan is to do that in userspace using the nameservice announcement from virtio/rpmsg. We did an hackish patch to do that internally: https://github.com/iotbzh/meta-rcar-zephyr/blob/master/recipes-kernel/linux/linux-renesas/0001-Add-device-driver-for-rcar-r7- rpmsg.patch That we will be really happy to drop by any cleaner solution. Thanks for your work ! Julien
Hi Mathieu, On 5/6/21 6:11 PM, Mathieu Poirier wrote: > Good day, > > On Wed, May 05, 2021 at 08:25:24PM +0200, Arnaud POULIQUEN wrote: >> Hi Mathieu, >> >> On 5/5/21 6:41 PM, Mathieu Poirier wrote: >>> Hi Arnaud, >>> >>> On Thu, Apr 29, 2021 at 03:55:06PM +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 | 53 +++++++++++++++++++++++++++++++++++++- >>>> 1 file changed, 52 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >>>> index 5c6a7da6e4d7..9166454c1310 100644 >>>> --- a/drivers/rpmsg/rpmsg_char.c >>>> +++ b/drivers/rpmsg/rpmsg_char.c >>>> @@ -18,6 +18,8 @@ >>>> >>>> #include "rpmsg_char.h" >>>> >>>> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" >>>> + >>>> static dev_t rpmsg_major; >>>> static struct class *rpmsg_class; >>>> >>>> @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent >>>> } >>>> EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create); >>>> >>>> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) >>>> +{ >>>> + struct rpmsg_channel_info chinfo; >>>> + >>>> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); >>>> + chinfo.src = rpdev->src; >>>> + chinfo.dst = rpdev->dst; >>>> + >>>> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true); >>>> +} >>>> + >>>> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy); >>>> + 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, >>>> + .drv = { >>>> + .name = "rpmsg_chrdev", >>>> + }, >>>> +}; >>> >>> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the >>> name service - but is it really needed? Up to now and aside from GLINK and SMD, >>> there asn't been other users of it so I'm wondering if it is worth going through >>> all this trouble. >> >> It is a good point. >> >> Just as a reminder, the need of ST and, I assume, some other companies, is to >> have a basic/generic communication channel to control a remote processor >> application. >> >> Nothing generic exists today for a virtio transport based implementation. >> Companies have to create their own driver. >> >> The purpose of my work is to allow our customer to use RPMsg without developing >> a specific driver to control remote applications. >> >> The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple >> inter-processor link to send and receive data. The rpmsg_tty is another one. >> >> Focusing on the rpmsg_chrdev: >> We did a part of the work with the first patch set that would be in 5.13. >> But is it simple to use it for virtio transport based platforms? >> If we don't implement the NS announcement support in rpmsg_chrdev, using >> rpmsg_chrdev for a user application seems rather tricky. >> How to instantiate the communication? > > Since we already have /dev/rpmsg_ctrlX user space can instantiate an > using that interface, which is how things are done in the GLINK/SMD world. > > Wouldn't that cover the usecases you had in mind? I have in mind that to make RPMsg easy to use, we need a generic driver with a basic user interface to send end receive data, that supports the NS announcement: - remote side could instantiate it. - an instantiation of the device by a Linux application generates a NS announcement sent to the remote side (for instance to create a channel for debug trace). On the other side, the initial work requested by Bjorn seems to be reached: de-correlate the control part to be able to reuse it for other rpmsg devices. I just have the feeling that we are stay in the middle of the road without the patches 4,5 and 6 to have a first basic interface relying on RPMsg. > > As you pointed out above rpmsg_chrdev should be light and simple - eliminating > patches 4, 5 and 6 would yield that. > My concern here is more about the complexity of using it by application, for platforms that rely on virtio rpmsg transport. For instance applications need to know the notion of local and remote RPMsg addressing. Based on your feeling, here is my proposition for next steps: 1- resend a version a version with only patch 1,2 3 + the patch to clean-up the #include in rpmsg_char 2- switch back to the RPMsg TTY upstream. 3- extend rpmsg_ctrl IOCTLs to allow instantiate RPMSG_TTY from Linux userland. Then, we can come back to patches 4, 5 and 6 depending on the feedback from the users. Does this proposition would be OK for you? Thanks, Arnaud >> The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to >> determine the services and associated remote address. >> >> I don't think the QCOM drivers have the same problem because they seems to >> initiate the communication and work directly with the RPMsg endpoints ( new >> channel creation on endpoint creation) while Virtio works with the RPMsg channel. >> >> By introducing the ability to instantiate rpmsg_chrdevs through the NS >> announcement, we make this easy for applications to use. >> >> And without rpmsg_chrdevs instantiation, It also means that we can't create an >> RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control, >> right? >> >> That said, If we consider that the aim was only to extract the rpmsg_ctrl part, >> I'm not against leaving the rpmsg_char in this state and switching to the >> rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg >> channels. >> >> We could come back on this if requested by someone else. >> >> Thanks, >> Arnaud >> >>> >>> As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name >>> service. That way patches 4, 5 and 6 of this set can be dropped. >>> >>> Thanks, >>> Mathieu >>> >>>> + >>>> static int rpmsg_chrdev_init(void) >>>> { >>>> int ret; >>>> @@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void) >>>> 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 = PTR_ERR(rpmsg_class); >>>> + goto free_region; >>>> + } >>>> + >>>> + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); >>>> + if (ret < 0) { >>>> + pr_err("rpmsg: failed to register rpmsg raw driver\n"); >>>> + goto free_class; >>>> } >>>> >>>> return 0; >>>> + >>>> +free_class: >>>> + class_destroy(rpmsg_class); >>>> +free_region: >>>> + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); >>>> + >>>> + return ret; >>>> } >>>> 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); >>>> } >>>> -- >>>> 2.17.1 >>>>
Good morning, On Fri, May 07, 2021 at 11:30:30AM +0200, Arnaud POULIQUEN wrote: > Hi Mathieu, > > On 5/6/21 6:11 PM, Mathieu Poirier wrote: > > Good day, > > > > On Wed, May 05, 2021 at 08:25:24PM +0200, Arnaud POULIQUEN wrote: > >> Hi Mathieu, > >> > >> On 5/5/21 6:41 PM, Mathieu Poirier wrote: > >>> Hi Arnaud, > >>> > >>> On Thu, Apr 29, 2021 at 03:55:06PM +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 | 53 +++++++++++++++++++++++++++++++++++++- > >>>> 1 file changed, 52 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > >>>> index 5c6a7da6e4d7..9166454c1310 100644 > >>>> --- a/drivers/rpmsg/rpmsg_char.c > >>>> +++ b/drivers/rpmsg/rpmsg_char.c > >>>> @@ -18,6 +18,8 @@ > >>>> > >>>> #include "rpmsg_char.h" > >>>> > >>>> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" > >>>> + > >>>> static dev_t rpmsg_major; > >>>> static struct class *rpmsg_class; > >>>> > >>>> @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent > >>>> } > >>>> EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create); > >>>> > >>>> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) > >>>> +{ > >>>> + struct rpmsg_channel_info chinfo; > >>>> + > >>>> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); > >>>> + chinfo.src = rpdev->src; > >>>> + chinfo.dst = rpdev->dst; > >>>> + > >>>> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true); > >>>> +} > >>>> + > >>>> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) > >>>> +{ > >>>> + int ret; > >>>> + > >>>> + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy); > >>>> + 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, > >>>> + .drv = { > >>>> + .name = "rpmsg_chrdev", > >>>> + }, > >>>> +}; > >>> > >>> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the > >>> name service - but is it really needed? Up to now and aside from GLINK and SMD, > >>> there asn't been other users of it so I'm wondering if it is worth going through > >>> all this trouble. > >> > >> It is a good point. > >> > >> Just as a reminder, the need of ST and, I assume, some other companies, is to > >> have a basic/generic communication channel to control a remote processor > >> application. > >> > >> Nothing generic exists today for a virtio transport based implementation. > >> Companies have to create their own driver. > >> > >> The purpose of my work is to allow our customer to use RPMsg without developing > >> a specific driver to control remote applications. > >> > >> The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple > >> inter-processor link to send and receive data. The rpmsg_tty is another one. > >> > >> Focusing on the rpmsg_chrdev: > >> We did a part of the work with the first patch set that would be in 5.13. > >> But is it simple to use it for virtio transport based platforms? > >> If we don't implement the NS announcement support in rpmsg_chrdev, using > >> rpmsg_chrdev for a user application seems rather tricky. > >> How to instantiate the communication? > > > > Since we already have /dev/rpmsg_ctrlX user space can instantiate an > > using that interface, which is how things are done in the GLINK/SMD world. > > > > Wouldn't that cover the usecases you had in mind? > > I have in mind that to make RPMsg easy to use, we need a generic driver with a > basic user interface to send end receive data, that supports the NS announcement: > - remote side could instantiate it. > - an instantiation of the device by a Linux application generates a NS > announcement sent to the remote side (for instance to create a channel for debug > trace). > The communication using a rpmsg_chrdev should be happening in two different ways, i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you had in a previous patchset). From user space communication using a rpmsg_chrdev should be initiated in two different ways, i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you had in a previous patchset). Regarding RPMSG_CREATE_EPT_IOCTL, patches 1, 2 and 3 take care of the legacy compatibility and I am quite happy with that. In this case the driver works the same way regardless of the transport mechanism - virtio, GLINK or SMD. Then there is instantiation with RPMSG_CREATE_DEV_IOCTL. That creates a new channel (with endpoint) when coming from /dev/rpmsg_ctrlX. When we have that functionality we can make the rpmsg_chrdev available from the name service, making sure the end result is the same regardless of source of the request (remote processor or user space). I was under the impression that functionality would be part of an upcoming patchset. Unless I'm missing parts of the story, proceeding this way should cover all the requirements we talked about. > On the other side, the initial work requested by Bjorn seems to be reached: > de-correlate the control part to be able to reuse it for other rpmsg devices. > > I just have the feeling that we are stay in the middle of the road without the > patches 4,5 and 6 to have a first basic interface relying on RPMsg. > > > > > As you pointed out above rpmsg_chrdev should be light and simple - eliminating > > patches 4, 5 and 6 would yield that. > > > > My concern here is more about the complexity of using it by application, for > platforms that rely on virtio rpmsg transport. For instance applications need to > know the notion of local and remote RPMsg addressing. > > Based on your feeling, here is my proposition for next steps: > 1- resend a version a version with only patch 1,2 3 + the patch to clean-up the > #include in rpmsg_char > 2- switch back to the RPMsg TTY upstream. > 3- extend rpmsg_ctrl IOCTLs to allow instantiate RPMSG_TTY from Linux userland. > Introducing RPMSG_TTY makes sense if a serial controller is only accessible from the remote processor. On the flip side it is an overkill if we just want a raw message passing mechanism. For that the rpmsg_chrdev driver, with the above extention, should be used. > > Then, we can come back to patches 4, 5 and 6 depending on the feedback from the > users. > > Does this proposition would be OK for you? > > Thanks, > Arnaud > > > >> The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to > >> determine the services and associated remote address. > >> > >> I don't think the QCOM drivers have the same problem because they seems to > >> initiate the communication and work directly with the RPMsg endpoints ( new > >> channel creation on endpoint creation) while Virtio works with the RPMsg channel. > >> > >> By introducing the ability to instantiate rpmsg_chrdevs through the NS > >> announcement, we make this easy for applications to use. > >> > >> And without rpmsg_chrdevs instantiation, It also means that we can't create an > >> RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control, > >> right? > >> > >> That said, If we consider that the aim was only to extract the rpmsg_ctrl part, > >> I'm not against leaving the rpmsg_char in this state and switching to the > >> rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg > >> channels. > >> > >> We could come back on this if requested by someone else. > >> > >> Thanks, > >> Arnaud > >> > >>> > >>> As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name > >>> service. That way patches 4, 5 and 6 of this set can be dropped. > >>> > >>> Thanks, > >>> Mathieu > >>> > >>>> + > >>>> static int rpmsg_chrdev_init(void) > >>>> { > >>>> int ret; > >>>> @@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void) > >>>> 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 = PTR_ERR(rpmsg_class); > >>>> + goto free_region; > >>>> + } > >>>> + > >>>> + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); > >>>> + if (ret < 0) { > >>>> + pr_err("rpmsg: failed to register rpmsg raw driver\n"); > >>>> + goto free_class; > >>>> } > >>>> > >>>> return 0; > >>>> + > >>>> +free_class: > >>>> + class_destroy(rpmsg_class); > >>>> +free_region: > >>>> + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); > >>>> + > >>>> + return ret; > >>>> } > >>>> 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); > >>>> } > >>>> -- > >>>> 2.17.1 > >>>>
Good morning Julien, On Fri, May 07, 2021 at 10:17:12AM +0200, Julien Massot wrote: > Hi Mathieu, Arnaud, > > On 5/5/21 8:25 PM, Arnaud POULIQUEN wrote: > > Hi Mathieu, > > > > On 5/5/21 6:41 PM, Mathieu Poirier wrote: > > > Hi Arnaud, > > > > > > On Thu, Apr 29, 2021 at 03:55:06PM +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 | 53 +++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 52 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > > > > index 5c6a7da6e4d7..9166454c1310 100644 > > > > --- a/drivers/rpmsg/rpmsg_char.c > > > > +++ b/drivers/rpmsg/rpmsg_char.c > > > > @@ -18,6 +18,8 @@ > > > > #include "rpmsg_char.h" > > > > +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" > > > > + > > > > static dev_t rpmsg_major; > > > > static struct class *rpmsg_class; > > > > @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent > > > > } > > > > EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create); > > > > +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) > > > > +{ > > > > + struct rpmsg_channel_info chinfo; > > > > + > > > > + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); > > > > + chinfo.src = rpdev->src; > > > > + chinfo.dst = rpdev->dst; > > > > + > > > > + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true); > > > > +} > > > > + > > > > +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy); > > > > + 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, > > > > + .drv = { > > > > + .name = "rpmsg_chrdev", > > > > + }, > > > > +}; > > > > > > The sole purpose of doing this is to create instances of rpmsg_chrdevs from the > > > name service - but is it really needed? Up to now and aside from GLINK and SMD, > > > there asn't been other users of it so I'm wondering if it is worth going through > > > all this trouble. > > > > It is a good point. > > > > Just as a reminder, the need of ST and, I assume, some other companies, is to > > have a basic/generic communication channel to control a remote processor > > application. > > > > Nothing generic exists today for a virtio transport based implementation. > > Companies have to create their own driver. > > > > The purpose of my work is to allow our customer to use RPMsg without developing > > a specific driver to control remote applications. > > > > The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple > > inter-processor link to send and receive data. The rpmsg_tty is another one. > > > > Focusing on the rpmsg_chrdev: > > We did a part of the work with the first patch set that would be in 5.13. > > But is it simple to use it for virtio transport based platforms? > > If we don't implement the NS announcement support in rpmsg_chrdev, using > > rpmsg_chrdev for a user application seems rather tricky. > > How to instantiate the communication? > > The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to > > determine the services and associated remote address. > > > > I don't think the QCOM drivers have the same problem because they seems to > > initiate the communication and work directly with the RPMsg endpoints ( new > > channel creation on endpoint creation) while Virtio works with the RPMsg channel. > > > > By introducing the ability to instantiate rpmsg_chrdevs through the NS > > announcement, we make this easy for applications to use. > > > > And without rpmsg_chrdevs instantiation, It also means that we can't create an > > RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control, > > right? > > > > That said, If we consider that the aim was only to extract the rpmsg_ctrl part, > > I'm not against leaving the rpmsg_char in this state and switching to the > > rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg > > channels. > > > > We could come back on this if requested by someone else. > > I'm personnaly following this thread, our project is to be able to do RPC call > from Linux to an RTOS (Zephyr). Our plan is to do that in userspace using the nameservice > announcement from virtio/rpmsg. Good to know. I highly encourage you to review patches and provide comments - that will be very helpful to us. Thanks, Mathieu > > We did an hackish patch to do that internally: > https://github.com/iotbzh/meta-rcar-zephyr/blob/master/recipes-kernel/linux/linux-renesas/0001-Add-device-driver-for-rcar-r7- > rpmsg.patch > > That we will be really happy to drop by any cleaner solution. > > Thanks for your work ! > Julien
Hello Mathieu, On 5/7/21 6:31 PM, Mathieu Poirier wrote: > Good morning, > > On Fri, May 07, 2021 at 11:30:30AM +0200, Arnaud POULIQUEN wrote: >> Hi Mathieu, >> >> On 5/6/21 6:11 PM, Mathieu Poirier wrote: >>> Good day, >>> >>> On Wed, May 05, 2021 at 08:25:24PM +0200, Arnaud POULIQUEN wrote: >>>> Hi Mathieu, >>>> >>>> On 5/5/21 6:41 PM, Mathieu Poirier wrote: >>>>> Hi Arnaud, >>>>> >>>>> On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote: [snip...] >>>>>> +}; >>>>> >>>>> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the >>>>> name service - but is it really needed? Up to now and aside from GLINK and SMD, >>>>> there asn't been other users of it so I'm wondering if it is worth going through >>>>> all this trouble. >>>> >>>> It is a good point. >>>> >>>> Just as a reminder, the need of ST and, I assume, some other companies, is to >>>> have a basic/generic communication channel to control a remote processor >>>> application. >>>> >>>> Nothing generic exists today for a virtio transport based implementation. >>>> Companies have to create their own driver. >>>> >>>> The purpose of my work is to allow our customer to use RPMsg without developing >>>> a specific driver to control remote applications. >>>> >>>> The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple >>>> inter-processor link to send and receive data. The rpmsg_tty is another one. >>>> >>>> Focusing on the rpmsg_chrdev: >>>> We did a part of the work with the first patch set that would be in 5.13. >>>> But is it simple to use it for virtio transport based platforms? >>>> If we don't implement the NS announcement support in rpmsg_chrdev, using >>>> rpmsg_chrdev for a user application seems rather tricky. >>>> How to instantiate the communication? >>> >>> Since we already have /dev/rpmsg_ctrlX user space can instantiate an >>> using that interface, which is how things are done in the GLINK/SMD world. >>> >>> Wouldn't that cover the usecases you had in mind? >> >> I have in mind that to make RPMsg easy to use, we need a generic driver with a >> basic user interface to send end receive data, that supports the NS announcement: >> - remote side could instantiate it. >> - an instantiation of the device by a Linux application generates a NS >> announcement sent to the remote side (for instance to create a channel for debug >> trace). >> > > The communication using a rpmsg_chrdev should be happening in two different ways, > i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you had in a previous > patchset). > > From user space communication using a rpmsg_chrdev should be initiated in two > different ways, i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you > had in a previous patchset). > > Regarding RPMSG_CREATE_EPT_IOCTL, patches 1, 2 and 3 take care of the legacy > compatibility and I am quite happy with that. In this case the driver works the > same way regardless of the transport mechanism - virtio, GLINK or SMD. Ok i will send a new revision including only this ones, and continue the updates in a new patchset. > > Then there is instantiation with RPMSG_CREATE_DEV_IOCTL. That creates a new > channel (with endpoint) when coming from /dev/rpmsg_ctrlX. When we have that > functionality we can make the rpmsg_chrdev available from the name service, making > sure the end result is the same regardless of source of the request (remote > processor or user space). I was under the impression that functionality would > be part of an upcoming patchset. > > Unless I'm missing parts of the story, proceeding this way should cover all the > requirements we talked about. From my windows, there are 3 remaining features: - capability to instantiate rpmsg_chrdev from the remote side (NS announcement) - capability to instantiate rpmsg_chrdev from local user application (RPMSG_CREATE_DEV_IOCTL) - capability to send a NS announcement to the remote side on rpmsg_chrdev local instantiation using RPMSG_CREATE_DEV_IOCTL. This one could be more tricky to implement as the endpoint can be created after the channel. To simplify the review while keeping the overall picture in mind (and perhaps prioritize based on other companies' interests), Please, just tell me what would be your preference in term of splitting and next step. > >> On the other side, the initial work requested by Bjorn seems to be reached: >> de-correlate the control part to be able to reuse it for other rpmsg devices. >> >> I just have the feeling that we are stay in the middle of the road without the >> patches 4,5 and 6 to have a first basic interface relying on RPMsg. >> >>> >>> As you pointed out above rpmsg_chrdev should be light and simple - eliminating >>> patches 4, 5 and 6 would yield that. >>> >> >> My concern here is more about the complexity of using it by application, for >> platforms that rely on virtio rpmsg transport. For instance applications need to >> know the notion of local and remote RPMsg addressing. >> >> Based on your feeling, here is my proposition for next steps: >> 1- resend a version a version with only patch 1,2 3 + the patch to clean-up the >> #include in rpmsg_char >> 2- switch back to the RPMsg TTY upstream. >> 3- extend rpmsg_ctrl IOCTLs to allow instantiate RPMSG_TTY from Linux userland. >> > > Introducing RPMSG_TTY makes sense if a serial controller is only accessible from > the remote processor. On the flip side it is an overkill if we just want a raw > message passing mechanism. For that the rpmsg_chrdev driver, with the above > extention, should be used. > Yes the rpmsg_chrdev should be the default one to use for basic communication. The main purpose of the RPMSG_TTY (from ST company POW) is to easy the transition in term of communication between an external and an internal processor based on a serial link. It provides an abstraction layer that the application does not have to manage the transport layer. Both seem to me interesting to implement, but let's continue to focus on rpmsg_chrdev first. Thanks, Arnaud >> >> Then, we can come back to patches 4, 5 and 6 depending on the feedback from the >> users. >> >> Does this proposition would be OK for you? >> >> Thanks, >> Arnaud >> >> >>>> The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to >>>> determine the services and associated remote address. >>>> >>>> I don't think the QCOM drivers have the same problem because they seems to >>>> initiate the communication and work directly with the RPMsg endpoints ( new >>>> channel creation on endpoint creation) while Virtio works with the RPMsg channel. >>>> >>>> By introducing the ability to instantiate rpmsg_chrdevs through the NS >>>> announcement, we make this easy for applications to use. >>>> >>>> And without rpmsg_chrdevs instantiation, It also means that we can't create an >>>> RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control, >>>> right? >>>> >>>> That said, If we consider that the aim was only to extract the rpmsg_ctrl part, >>>> I'm not against leaving the rpmsg_char in this state and switching to the >>>> rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg >>>> channels. >>>> >>>> We could come back on this if requested by someone else. >>>> >>>> Thanks, >>>> Arnaud >>>> >>>>> >>>>> As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name >>>>> service. That way patches 4, 5 and 6 of this set can be dropped. >>>>> >>>>> Thanks, >>>>> Mathieu >>>>>
On Mon, May 17, 2021 at 12:04:29PM +0200, Arnaud POULIQUEN wrote: > Hello Mathieu, > > On 5/7/21 6:31 PM, Mathieu Poirier wrote: > > Good morning, > > > > On Fri, May 07, 2021 at 11:30:30AM +0200, Arnaud POULIQUEN wrote: > >> Hi Mathieu, > >> > >> On 5/6/21 6:11 PM, Mathieu Poirier wrote: > >>> Good day, > >>> > >>> On Wed, May 05, 2021 at 08:25:24PM +0200, Arnaud POULIQUEN wrote: > >>>> Hi Mathieu, > >>>> > >>>> On 5/5/21 6:41 PM, Mathieu Poirier wrote: > >>>>> Hi Arnaud, > >>>>> > >>>>> On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote: > > [snip...] > > >>>>>> +}; > >>>>> > >>>>> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the > >>>>> name service - but is it really needed? Up to now and aside from GLINK and SMD, > >>>>> there asn't been other users of it so I'm wondering if it is worth going through > >>>>> all this trouble. > >>>> > >>>> It is a good point. > >>>> > >>>> Just as a reminder, the need of ST and, I assume, some other companies, is to > >>>> have a basic/generic communication channel to control a remote processor > >>>> application. > >>>> > >>>> Nothing generic exists today for a virtio transport based implementation. > >>>> Companies have to create their own driver. > >>>> > >>>> The purpose of my work is to allow our customer to use RPMsg without developing > >>>> a specific driver to control remote applications. > >>>> > >>>> The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple > >>>> inter-processor link to send and receive data. The rpmsg_tty is another one. > >>>> > >>>> Focusing on the rpmsg_chrdev: > >>>> We did a part of the work with the first patch set that would be in 5.13. > >>>> But is it simple to use it for virtio transport based platforms? > >>>> If we don't implement the NS announcement support in rpmsg_chrdev, using > >>>> rpmsg_chrdev for a user application seems rather tricky. > >>>> How to instantiate the communication? > >>> > >>> Since we already have /dev/rpmsg_ctrlX user space can instantiate an > >>> using that interface, which is how things are done in the GLINK/SMD world. > >>> > >>> Wouldn't that cover the usecases you had in mind? > >> > >> I have in mind that to make RPMsg easy to use, we need a generic driver with a > >> basic user interface to send end receive data, that supports the NS announcement: > >> - remote side could instantiate it. > >> - an instantiation of the device by a Linux application generates a NS > >> announcement sent to the remote side (for instance to create a channel for debug > >> trace). > >> > > > > The communication using a rpmsg_chrdev should be happening in two different ways, > > i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you had in a previous > > patchset). > > > > From user space communication using a rpmsg_chrdev should be initiated in two > > different ways, i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you > > had in a previous patchset). > > > > Regarding RPMSG_CREATE_EPT_IOCTL, patches 1, 2 and 3 take care of the legacy > > compatibility and I am quite happy with that. In this case the driver works the > > same way regardless of the transport mechanism - virtio, GLINK or SMD. > > Ok i will send a new revision including only this ones, and continue the updates > in a new patchset. > > > > > Then there is instantiation with RPMSG_CREATE_DEV_IOCTL. That creates a new > > channel (with endpoint) when coming from /dev/rpmsg_ctrlX. When we have that > > functionality we can make the rpmsg_chrdev available from the name service, making > > sure the end result is the same regardless of source of the request (remote > > processor or user space). I was under the impression that functionality would > > be part of an upcoming patchset. > > > > Unless I'm missing parts of the story, proceeding this way should cover all the > > requirements we talked about. > > From my windows, there are 3 remaining features: > - capability to instantiate rpmsg_chrdev from the remote side (NS announcement) I think this should be #2. > - capability to instantiate rpmsg_chrdev from local user application > (RPMSG_CREATE_DEV_IOCTL) This should be #1. Once this is firmly in place #2 (above) should be relatively easy to implement. #1 and #2 can be in the same patchset, or not, depending on what you prefer. > - capability to send a NS announcement to the remote side on rpmsg_chrdev local > instantiation using RPMSG_CREATE_DEV_IOCTL. This one could be more tricky to > implement as the endpoint can be created after the channel. That should probably come after #1 and #2, and in a separate patchset. > > To simplify the review while keeping the overall picture in mind (and perhaps > prioritize based on other companies' interests), Please, just tell me what would > be your preference in term of splitting and next step. > > > > >> On the other side, the initial work requested by Bjorn seems to be reached: > >> de-correlate the control part to be able to reuse it for other rpmsg devices. > >> > >> I just have the feeling that we are stay in the middle of the road without the > >> patches 4,5 and 6 to have a first basic interface relying on RPMsg. > >> > >>> > >>> As you pointed out above rpmsg_chrdev should be light and simple - eliminating > >>> patches 4, 5 and 6 would yield that. > >>> > >> > >> My concern here is more about the complexity of using it by application, for > >> platforms that rely on virtio rpmsg transport. For instance applications need to > >> know the notion of local and remote RPMsg addressing. > >> > >> Based on your feeling, here is my proposition for next steps: > >> 1- resend a version a version with only patch 1,2 3 + the patch to clean-up the > >> #include in rpmsg_char > >> 2- switch back to the RPMsg TTY upstream. > >> 3- extend rpmsg_ctrl IOCTLs to allow instantiate RPMSG_TTY from Linux userland. > >> > > > > Introducing RPMSG_TTY makes sense if a serial controller is only accessible from > > the remote processor. On the flip side it is an overkill if we just want a raw > > message passing mechanism. For that the rpmsg_chrdev driver, with the above > > extention, should be used. > > > > Yes the rpmsg_chrdev should be the default one to use for basic communication. Perfect, we are on the same page. > The main purpose of the RPMSG_TTY (from ST company POW) is to easy the > transition in term of communication between an external and an internal > processor based on a serial link. It provides an abstraction layer that the > application does not have to manage the transport layer. > Ok > Both seem to me interesting to implement, but let's continue to focus on > rpmsg_chrdev first. > > Thanks, > Arnaud > > >> > >> Then, we can come back to patches 4, 5 and 6 depending on the feedback from the > >> users. > >> > >> Does this proposition would be OK for you? > >> > >> Thanks, > >> Arnaud > >> > >> > >>>> The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to > >>>> determine the services and associated remote address. > >>>> > >>>> I don't think the QCOM drivers have the same problem because they seems to > >>>> initiate the communication and work directly with the RPMsg endpoints ( new > >>>> channel creation on endpoint creation) while Virtio works with the RPMsg channel. > >>>> > >>>> By introducing the ability to instantiate rpmsg_chrdevs through the NS > >>>> announcement, we make this easy for applications to use. > >>>> > >>>> And without rpmsg_chrdevs instantiation, It also means that we can't create an > >>>> RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control, > >>>> right? > >>>> > >>>> That said, If we consider that the aim was only to extract the rpmsg_ctrl part, > >>>> I'm not against leaving the rpmsg_char in this state and switching to the > >>>> rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg > >>>> channels. > >>>> > >>>> We could come back on this if requested by someone else. > >>>> > >>>> Thanks, > >>>> Arnaud > >>>> > >>>>> > >>>>> As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name > >>>>> service. That way patches 4, 5 and 6 of this set can be dropped. > >>>>> > >>>>> Thanks, > >>>>> Mathieu > >>>>> >
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c index 5c6a7da6e4d7..9166454c1310 100644 --- a/drivers/rpmsg/rpmsg_char.c +++ b/drivers/rpmsg/rpmsg_char.c @@ -18,6 +18,8 @@ #include "rpmsg_char.h" +#define RPMSG_CHAR_DEVNAME "rpmsg-raw" + static dev_t rpmsg_major; static struct class *rpmsg_class; @@ -413,6 +415,40 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent } EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create); +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev) +{ + struct rpmsg_channel_info chinfo; + + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME)); + chinfo.src = rpdev->src; + chinfo.dst = rpdev->dst; + + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true); +} + +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev) +{ + int ret; + + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy); + 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, + .drv = { + .name = "rpmsg_chrdev", + }, +}; + static int rpmsg_chrdev_init(void) { int ret; @@ -427,15 +463,30 @@ static int rpmsg_chrdev_init(void) 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 = PTR_ERR(rpmsg_class); + goto free_region; + } + + ret = register_rpmsg_driver(&rpmsg_chrdev_driver); + if (ret < 0) { + pr_err("rpmsg: failed to register rpmsg raw driver\n"); + goto free_class; } return 0; + +free_class: + class_destroy(rpmsg_class); +free_region: + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX); + + return ret; } 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); }
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 | 53 +++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-)