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