RE: [PATCH v2] rpmsg: add a description field

[Arnaud POULIQUEN]

Hi Mathieu,

Following our discussion, here is a patches that proposes a solution for non static name services management, to be able to instanciate a rpmsg service.
An alternative is the patch proposed by Xiang: https://patchwork.kernel.org/patch/10791741/
So the capability to support non static names is required at least by TI and Xiaomi.
This is also something that could be considered for my rpmsg tty driver, as it supports multi instances...

IMHO the patch proposed by Xiang seems simpler and more flexible to meet this requirement.

Regards,
Arnaud

> -----Original Message-----
> From: Arnaud Pouliquen <arnaud.pouliquen@xxxxxx>
> Sent: mardi 3 septembre 2019 11:34
> To: Suman Anna <s-anna@xxxxxx>; Bjorn Andersson
> <bjorn.andersson@xxxxxxxxxx>
> Cc: Fabien DESSENNE <fabien.dessenne@xxxxxx>; Ohad Ben-Cohen
> <ohad@xxxxxxxxxx>; Loic PALLARDY <loic.pallardy@xxxxxx>; linux-
> remoteproc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Tero Kristo <t-
> kristo@xxxxxx>; Xiang Xiao <xiaoxiang@xxxxxxxxxx>
> Subject: Re: [PATCH v2] rpmsg: add a description field
> 
> Hi Suman,
> 
> Sorry for the delay in replying...
> 
> On 8/28/19 11:37 PM, Suman Anna wrote:
> > Hi Arnaud,
> >
> > On 8/27/19 8:58 AM, Arnaud Pouliquen wrote:
> >> Hi Suman,
> >>
> >> On 8/16/19 1:14 AM, Suman Anna wrote:
> >>> From: Ohad Ben-Cohen <ohad@xxxxxxxxxx>
> >>>
> >>> Add a new description field to the rpmsg bus infrastructure that can
> >>> be passed onto the rpmsg client drivers for additional information.
> >>> The current rpmsg bus client drivers need to have a fixed id_table
> >>> for proper matching, this new field can allow flexibility for the
> >>> client drivers (eg: like creating unique cdevs).
> >>>
> >>> The description field is published through an enhanced name service
> >>> announcement message structure. The name service message processing
> >>> logic is updated to maintain backward compatibility with the
> >>> previous message structure.
> >>
> >> Could you give some concrete use cases associated with your need?.
> >> I'm not sure I'm interpreting it correctly...
> >
> > I have a generic rpmsg-rpc driver that allows userspace to execute any
> > of a set of remote functions provided by a service. And we can have
> > the same set of functions exported from different rprocs, or different
> > services presenting different sets of functions. And the same service
> > is used by multiple applications through their own fds. I use the desc
> > field to differentiate between the services.
> 
> Thanks, this help me to understand the need. Very interesting feature, do you
> plan to upstream it? :)
> 
> >
> >>
> >> Your patch seems to me a way to create a kind of sub-service. Why not
> >> simply concatenate this in the name services, i.e creating several
> >> name services ("service-0", "service-1"...)?
> >
> > Every-time a new service is added, it requires a driver update adding
> > the new service name to the compatible list, or a corresponding udev
> > rule. It is really upto the individual rpmsg driver as to how it uses
> > the desc field, so it is optional as far as a rpmsg driver is
> > concerned (since this didn't exist previously, and so all existing
> > drivers will continue to work as before).
> >
> 
> >>
> >> Regarding your implementation, the descriptor field seems used to:
> >> - instantiate a rpmsg service
> >> - retrieve the instance from the userland based on the descriptor.
> >>
> >> What not just use this descriptor to provide information to userland
> >> via sysfs, but not use it as a criteria to find the rpmsg device?
> >
> > The rpmsg device logic for actually probing a rpmsg driver still
> > relies on the name field. I added the additional check against desc to
> > just ensure that there are no name clashes on the desc field. Do you
> > have any concerns with this check?
> 
> First be sure that I understand and agree with the need to have a more dynamic
> way of managing the services. My concerns is more about the way to implement
> it. In my view the need corresponds to the introduction of a kind of sub-services to
> instantiate a service, plus a way to identify the sub service. Adding a second field
> is one solution, but seems to me a duplication of the service name. And i have
> also in mind the OpenAMP lib that should support this extra field, with footprint
> impact.
> 
> Xiang Xiao from Xiaomi company proposed an alternative patch a few months
> ago: https://patchwork.kernel.org/patch/10791741/.
> The idea is let the driver to check the device match. In this case it could support
> "dynamic" name service. A service name could be a concatenation of the basic
> service name + a descriptor.
> 
> Could this patch answer to your need? IMHO, I would prefer this solution that
> gives advantage to not extend the message protocol for NS announcement.
> 
> Regards,
> Arnaud
> 
> >
> >>
> >> In this case you can use the remote endpoint address(dst addr) to
> >> instantiate the service. The descriptor field could just be an
> >> information to help application to retrieve the good instance of the
> >> service.
> >>
> >>>
> >>> Based on an initial patch from Ohad Ben-Cohen.
> >>>
> >>> Signed-off-by: Ohad Ben-Cohen <ohad@xxxxxxxxxx>
> >>> [s-anna@xxxxxx: forward port, add sysfs documentation, fixup qcom
> >>> drivers]
> >>> Signed-off-by: Suman Anna <s-anna@xxxxxx>
> >>> [t-kristo@xxxxxx: reworked to support both rpmsg with/without the
> >>> desc field]
> >>> Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
> >>> ---
> >>> v2:
> >>>  Â - Localized the desc match check to virtio-rpmsg-bus
> >>>  Â - Enforced NULL termination of desc similar to name
> >>> v1: https://patchwork.kernel.org/patch/11087717/
> >>>  Â Documentation/ABI/testing/sysfs-bus-rpmsg | 29 ++++++++++
> >>>  Â drivers/rpmsg/qcom_glink_native.cÂÂÂÂÂÂÂÂ |Â 1 +
> >>>  Â drivers/rpmsg/qcom_smd.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 1 +
> >>>  Â drivers/rpmsg/rpmsg_char.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 1 +
> >>>  Â drivers/rpmsg/rpmsg_core.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 2 +
> >>>  Â drivers/rpmsg/virtio_rpmsg_bus.cÂÂÂÂÂÂÂÂÂ | 67
> >>> +++++++++++++++++++++--
> >>>  Â drivers/soc/qcom/wcnss_ctrl.cÂÂÂÂÂÂÂÂÂÂÂÂ |Â 1 +
> >>>  Â include/linux/rpmsg.hÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 4 ++
> >>>  Â 8 files changed, 101 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-rpmsg
> >>> b/Documentation/ABI/testing/sysfs-bus-rpmsg
> >>> index 990fcc420935..7f1b09ecc64d 100644
> >>> --- a/Documentation/ABI/testing/sysfs-bus-rpmsg
> >>> +++ b/Documentation/ABI/testing/sysfs-bus-rpmsg
> >>> @@ -93,3 +93,32 @@ Description:
> >>>  ÂÂÂÂÂÂÂÂÂ This sysfs entry allows the rpmsg driver for a rpmsg
> >>> device
> >>>  ÂÂÂÂÂÂÂÂÂ to be specified which will override standard OF, ID table
> >>>  ÂÂÂÂÂÂÂÂÂ and name matching.
> >>> +
> >>> +What:ÂÂÂÂÂÂÂ /sys/bus/rpmsg/devices/.../desc
> >>> +Date:ÂÂÂÂÂÂÂ August 2019
> >>> +KernelVersion:ÂÂÂ 5.4
> >>> +Contact:ÂÂÂ Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> >>> +Description:
> >>> +ÂÂÂÂÂÂÂ Every rpmsg device is a communication channel with a remote
> >>> +ÂÂÂÂÂÂÂ processor. Channels are identified by a textual name (see
> >>> +ÂÂÂÂÂÂÂ /sys/bus/rpmsg/devices/.../name above) and have a local
> >>> +ÂÂÂÂÂÂÂ ("source") rpmsg address, and remote ("destination") rpmsg
> >>> +ÂÂÂÂÂÂÂ address.
> >>> +
> >>> +ÂÂÂÂÂÂÂ A channel is first created when an entity, whether local
> >>> +ÂÂÂÂÂÂÂ or remote, starts listening on it for messages (and is thus
> >>> +ÂÂÂÂÂÂÂ called an rpmsg server). When that happens, a "name service"
> >>> +ÂÂÂÂÂÂÂ announcement is sent to the other processor, in order to
> >>> +let
> >>> +ÂÂÂÂÂÂÂ it know about the creation of the channel (this way remote
> >>> +ÂÂÂÂÂÂÂ clients know they can start sending messages).
> >>> +
> >>> +ÂÂÂÂÂÂÂ The listening entity (or client) which communicates with a
> >>> +ÂÂÂÂÂÂÂ remote processor is referred as rpmsg driver. The rpmsg
> >>> +device
> >>> +ÂÂÂÂÂÂÂ and rpmsg driver are matched based on rpmsg device name
> >>> +(see
> >>> +ÂÂÂÂÂÂÂ /sys/bus/rpmsg/devices/.../name above) and rpmsg driver ID
> >>> table.
> >>> +
> >>> +ÂÂÂÂÂÂÂ This sysfs entry contains an additional optional
> >>> +description of
> >>> +ÂÂÂÂÂÂÂ the rpmsg device that can be optionally included as part of
> >>> +the
> >>> +ÂÂÂÂÂÂÂ "name service" announcement. This description is then
> >>> +passed on
> >>> +ÂÂÂÂÂÂÂ to the corresponding rpmsg drivers to further distinguish
> >>> multiple
> >>> +ÂÂÂÂÂÂÂ devices associated with the same rpmsg driver.
> >>> diff --git a/drivers/rpmsg/qcom_glink_native.c
> >>> b/drivers/rpmsg/qcom_glink_native.c
> >>> index f46c787733e8..cfdabddc15ac 100644
> >>> --- a/drivers/rpmsg/qcom_glink_native.c
> >>> +++ b/drivers/rpmsg/qcom_glink_native.c
> >>> @@ -1456,6 +1456,7 @@ static void qcom_glink_rx_close(struct
> >>> qcom_glink *glink, unsigned int rcid)
> >>>  ÂÂÂÂÂÂÂÂÂ strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
> >>>  ÂÂÂÂÂÂÂÂÂ chinfo.src = RPMSG_ADDR_ANY;
> >>>  ÂÂÂÂÂÂÂÂÂ chinfo.dst = RPMSG_ADDR_ANY;
> >>> +ÂÂÂÂÂÂÂ chinfo.desc[0] = '\0';
> >>>  Â ÂÂÂÂÂÂÂÂÂ rpmsg_unregister_device(glink->dev, &chinfo);
> >>>  ÂÂÂÂÂ }
> >>> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
> >>> index 4abbeea782fa..7cd6b9c47065 100644
> >>> --- a/drivers/rpmsg/qcom_smd.c
> >>> +++ b/drivers/rpmsg/qcom_smd.c
> >>> @@ -1307,6 +1307,7 @@ static void qcom_channel_state_worker(struct
> >>> work_struct *work)
> >>>  ÂÂÂÂÂÂÂÂÂ strncpy(chinfo.name, channel->name, sizeof(chinfo.name));
> >>>  ÂÂÂÂÂÂÂÂÂ chinfo.src = RPMSG_ADDR_ANY;
> >>>  ÂÂÂÂÂÂÂÂÂ chinfo.dst = RPMSG_ADDR_ANY;
> >>> +ÂÂÂÂÂÂÂ chinfo.desc[0] = '\0';
> >>>  ÂÂÂÂÂÂÂÂÂ rpmsg_unregister_device(&edge->dev, &chinfo);
> >>>  ÂÂÂÂÂÂÂÂÂ channel->registered = false;
> >>>  ÂÂÂÂÂÂÂÂÂ spin_lock_irqsave(&edge->channels_lock, flags); diff
> >>> --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >>> index eea5ebbb5119..4bd91445a2fd 100644
> >>> --- a/drivers/rpmsg/rpmsg_char.c
> >>> +++ b/drivers/rpmsg/rpmsg_char.c
> >>> @@ -442,6 +442,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp,
> >>> unsigned int cmd,
> >>>  ÂÂÂÂÂ chinfo.name[RPMSG_NAME_SIZE-1] = '\0';
> >>>  ÂÂÂÂÂ chinfo.src = eptinfo.src;
> >>>  ÂÂÂÂÂ chinfo.dst = eptinfo.dst;
> >>> +ÂÂÂ chinfo.desc[0] = '\0';
> >>>  Â ÂÂÂÂÂ return rpmsg_eptdev_create(ctrldev, chinfo);
> >>>  Â };
> >>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> >>> index ea88fd4e2a6e..ba0f2c1a7fa4 100644
> >>> --- a/drivers/rpmsg/rpmsg_core.c
> >>> +++ b/drivers/rpmsg/rpmsg_core.c
> >>> @@ -365,6 +365,7 @@ static DEVICE_ATTR_RW(field)
> >>>  Â Â /* for more info, see Documentation/ABI/testing/sysfs-bus-rpmsg
> >>> */
> >>>  Â rpmsg_show_attr(name, id.name, "%s\n");
> >>> +rpmsg_show_attr(desc, desc, "%s\n");
> >>>  Â rpmsg_show_attr(src, src, "0x%x\n");
> >>>  Â rpmsg_show_attr(dst, dst, "0x%x\n");
> >>>  Â rpmsg_show_attr(announce, announce ? "true" : "false", "%s\n");
> >>> @@ -386,6 +387,7 @@ static DEVICE_ATTR_RO(modalias);
> >>>  Â Â static struct attribute *rpmsg_dev_attrs[] = {
> >>>  ÂÂÂÂÂ &dev_attr_name.attr,
> >>> +ÂÂÂ &dev_attr_desc.attr,
> >>>  ÂÂÂÂÂ &dev_attr_modalias.attr,
> >>>  ÂÂÂÂÂ &dev_attr_dst.attr,
> >>>  ÂÂÂÂÂ &dev_attr_src.attr,
> >>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> index 5d3685bd76a2..b42277cd7759 100644
> >>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> @@ -110,6 +110,23 @@ struct rpmsg_ns_msg {
> >>>  ÂÂÂÂÂ u32 flags;
> >>>  Â } __packed;
> >>>  Â +/**
> >>> + * struct rpmsg_ns_msg_ext - dynamic name service announcement
> >>> message v2
> >>> + * @name: name of remote service that is published
> >>> + * @desc: description of remote service
> >>> + * @addr: address of remote service that is published
> >>> + * @flags: indicates whether service is created or destroyed
> >>> + *
> >>> + * Interchangeable nameservice message with rpmsg_ns_msg. This one
> >>> +has
> >>> + * the addition of the desc field for extra flexibility.
> >>> + */
> >>> +struct rpmsg_ns_msg_ext {
> >>> +ÂÂÂ char name[RPMSG_NAME_SIZE];
> >>> +ÂÂÂ char desc[RPMSG_NAME_SIZE];
> >>> +ÂÂÂ u32 addr;
> >>> +ÂÂÂ u32 flags;
> >>> +} __packed;
> >>> +
> >>>  Â /**
> >>>  ÂÂ * enum rpmsg_ns_flags - dynamic name service announcement flags
> >>>  ÂÂ *
> >>> @@ -384,6 +401,24 @@ static void virtio_rpmsg_release_device(struct
> >>> device *dev)
> >>>  ÂÂÂÂÂ kfree(vch);
> >>>  Â }
> >>>  Â +static int virtio_rpmsg_desc_match(struct device *dev, void
> >>> *data)
> >>> +{
> >>> +ÂÂÂ struct rpmsg_channel_info *chinfo = data;
> >>> +ÂÂÂ struct rpmsg_device *rpdev = to_rpmsg_device(dev);
> >>> +
> >>> +ÂÂÂ if (!*chinfo->desc)
> >>> +ÂÂÂÂÂÂÂ return 0;
> >>> +
> >>> +ÂÂÂ if (strncmp(chinfo->name, rpdev->id.name, RPMSG_NAME_SIZE))
> >>> +ÂÂÂÂÂÂÂ return 0;
> >>> +
> >>> +ÂÂÂ if (strncmp(chinfo->desc, rpdev->desc, RPMSG_NAME_SIZE))
> >>> +ÂÂÂÂÂÂÂ return 0;
> >>> +
> >>> +ÂÂÂ /* found a match ! */
> >>> +ÂÂÂ return 1;
> >>> +}
> >>> +
> >>>  Â /*
> >>>  ÂÂ * create an rpmsg channel using its name and address info.
> >>>  ÂÂ * this function will be used to create both static and dynamic
> >>> @@ -407,6 +442,15 @@ static struct rpmsg_device
> >>> *rpmsg_create_channel(struct virtproc_info *vrp,
> >>>  ÂÂÂÂÂÂÂÂÂ return NULL;
> >>>  ÂÂÂÂÂ }
> >>>  Â +ÂÂÂ tmp = device_find_child(dev, chinfo,
> >>> virtio_rpmsg_desc_match);
> >>> +ÂÂÂ if (tmp) {
> >>> +ÂÂÂÂÂÂÂ /* decrement the matched device's refcount back */
> >>> +ÂÂÂÂÂÂÂ put_device(tmp);
> >>> +ÂÂÂÂÂÂÂ dev_err(dev, "channel %s:%x:%x failed, desc '%s' already
> >>> exists\n",
> >>> +ÂÂÂÂÂÂÂÂÂÂÂ chinfo->name, chinfo->src, chinfo->dst, chinfo->desc);
> >>> +ÂÂÂÂÂÂÂ return NULL;
> >>> +ÂÂÂ }
> >>> +
> >>>  ÂÂÂÂÂ vch = kzalloc(sizeof(*vch), GFP_KERNEL);
> >>>  ÂÂÂÂÂ if (!vch)
> >>>  ÂÂÂÂÂÂÂÂÂ return NULL;
> >>> @@ -419,6 +463,7 @@ static struct rpmsg_device
> >>> *rpmsg_create_channel(struct virtproc_info *vrp,
> >>>  ÂÂÂÂÂ rpdev->src = chinfo->src;
> >>>  ÂÂÂÂÂ rpdev->dst = chinfo->dst;
> >>>  ÂÂÂÂÂ rpdev->ops = &virtio_rpmsg_ops;
> >>> +ÂÂÂ strncpy(rpdev->desc, chinfo->desc, RPMSG_NAME_SIZE);
> >>>  Â ÂÂÂÂÂ /*
> >>>  ÂÂÂÂÂÂ * rpmsg server channels has predefined local address (for
> >>> now), @@ -816,18 +861,30 @@ static int rpmsg_ns_cb(struct
> >>> rpmsg_device *rpdev, void *data, int len,
> >>>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void *priv, u32 src)
> >>>  Â {
> >>>  ÂÂÂÂÂ struct rpmsg_ns_msg *msg = data;
> >>> +ÂÂÂ struct rpmsg_ns_msg_ext *msg_ext = data;
> >>>  ÂÂÂÂÂ struct rpmsg_device *newch;
> >>>  ÂÂÂÂÂ struct rpmsg_channel_info chinfo;
> >>>  ÂÂÂÂÂ struct virtproc_info *vrp = priv;
> >>>  ÂÂÂÂÂ struct device *dev = &vrp->vdev->dev;
> >>>  ÂÂÂÂÂ int ret;
> >>> +ÂÂÂ u32 addr;
> >>> +ÂÂÂ u32 flags;
> >>>  Â Â #if defined(CONFIG_DYNAMIC_DEBUG)
> >>>  ÂÂÂÂÂ dynamic_hex_dump("NS announcement: ", DUMP_PREFIX_NONE, 16,
> >>> 1,
> >>>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ data, len, true);
> >>>  Â #endif
> >>>  Â -ÂÂÂ if (len != sizeof(*msg)) {
> >>> +ÂÂÂ if (len == sizeof(*msg)) {
> >>> +ÂÂÂÂÂÂÂ addr = msg->addr;
> >>> +ÂÂÂÂÂÂÂ flags = msg->flags;
> >>> +ÂÂÂÂÂÂÂ chinfo.desc[0] = '\0';
> >>> +ÂÂÂ } else if (len == sizeof(*msg_ext)) {
> >>> +ÂÂÂÂÂÂÂ addr = msg_ext->addr;
> >>> +ÂÂÂÂÂÂÂ flags = msg_ext->flags;
> >>> +ÂÂÂÂÂÂÂ msg_ext->desc[RPMSG_NAME_SIZE - 1] = '\0';
> >>> +ÂÂÂÂÂÂÂ strncpy(chinfo.desc, msg_ext->desc, sizeof(chinfo.desc));
> >>> +ÂÂÂ } else {
> >>>  ÂÂÂÂÂÂÂÂÂ dev_err(dev, "malformed ns msg (%d)\n", len);
> >>>  ÂÂÂÂÂÂÂÂÂ return -EINVAL;
> >>>  ÂÂÂÂÂ }
> >>> @@ -847,14 +904,14 @@ static int rpmsg_ns_cb(struct rpmsg_device
> >>> *rpdev, void *data, int len,
> >>>  ÂÂÂÂÂ msg->name[RPMSG_NAME_SIZE - 1] = '\0';
> >>>  Â ÂÂÂÂÂ dev_info(dev, "%sing channel %s addr 0x%x\n",
> >>> -ÂÂÂÂÂÂÂÂ msg->flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
> >>> -ÂÂÂÂÂÂÂÂ msg->name, msg->addr);
> >>> +ÂÂÂÂÂÂÂÂ flags & RPMSG_NS_DESTROY ? "destroy" : "creat",
> >>> +ÂÂÂÂÂÂÂÂ msg->name, addr);
> >>>  Â ÂÂÂÂÂ strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> >>>  ÂÂÂÂÂ chinfo.src = RPMSG_ADDR_ANY;
> >>> -ÂÂÂ chinfo.dst = msg->addr;
> >>> +ÂÂÂ chinfo.dst = addr;
> >>>  Â -ÂÂÂ if (msg->flags & RPMSG_NS_DESTROY) {
> >>> +ÂÂÂ if (flags & RPMSG_NS_DESTROY) {
> >>>  ÂÂÂÂÂÂÂÂÂ ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
> >>>  ÂÂÂÂÂÂÂÂÂ if (ret)
> >>>  ÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "rpmsg_destroy_channel failed: %d\n",
> >>> ret); diff --git a/drivers/soc/qcom/wcnss_ctrl.c
> >>> b/drivers/soc/qcom/wcnss_ctrl.c index e5c68051fb17..ad9f28dc13f1
> >>> 100644
> >>> --- a/drivers/soc/qcom/wcnss_ctrl.c
> >>> +++ b/drivers/soc/qcom/wcnss_ctrl.c
> >>> @@ -276,6 +276,7 @@ struct rpmsg_endpoint
> >>> *qcom_wcnss_open_channel(void *wcnss, const char *name, rp
> >>>  ÂÂÂÂÂ strscpy(chinfo.name, name, sizeof(chinfo.name));
> >>>  ÂÂÂÂÂ chinfo.src = RPMSG_ADDR_ANY;
> >>>  ÂÂÂÂÂ chinfo.dst = RPMSG_ADDR_ANY;
> >>> +ÂÂÂ chinfo.desc[0] = '\0';
> >>>  Â ÂÂÂÂÂ return rpmsg_create_ept(_wcnss->channel->rpdev, cb, priv,
> >>> chinfo);
> >
> >> There is another way to create a service, by registering an RPMsg
> >> drivers (e.g.
> >> https://elixir.bootlin.com/linux/v5.3-rc6/source/samples/rpmsg/rpmsg_
> >> client_sample.c)
> >
> > I am not sure I understand, you obviously need a rpmsg driver for a
> > rpmsg device to probe.
> >
> >>
> >> In this case the descriptor can not be used, right?
> >> To be compliant probably need to extend the rpmsg_device_id struct...
> >
> > As I mentioned above, it is optional for backward compatibility, and
> > the rpmsg client sample doesn't have any use for it atm, even if a
> > firmware publishes a desc field alongside the "rpmsg-client-sample"
> > name. That is why, I haven't extended the rpmsg_device_id.
> >
> > regards
> > Suman
> >
> >>
> >> Regards,
> >> Arnaud
> >>>  Â }
> >>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h index
> >>> 9fe156d1c018..436faf04ba1c 100644
> >>> --- a/include/linux/rpmsg.h
> >>> +++ b/include/linux/rpmsg.h
> >>> @@ -28,11 +28,13 @@ struct rpmsg_endpoint_ops;
> >>>  Â /**
> >>>  ÂÂ * struct rpmsg_channel_info - channel info representation
> >>>  ÂÂ * @name: name of service
> >>> + * @desc: description of service
> >>>  ÂÂ * @src: local address
> >>>  ÂÂ * @dst: destination address
> >>>  ÂÂ */
> >>>  Â struct rpmsg_channel_info {
> >>>  ÂÂÂÂÂ char name[RPMSG_NAME_SIZE];
> >>> +ÂÂÂ char desc[RPMSG_NAME_SIZE];
> >>>  ÂÂÂÂÂ u32 src;
> >>>  ÂÂÂÂÂ u32 dst;
> >>>  Â };
> >>> @@ -42,6 +44,7 @@ struct rpmsg_channel_info {
> >>>  ÂÂ * @dev: the device struct
> >>>  ÂÂ * @id: device id (used to match between rpmsg drivers and
> >>> devices)
> >>>  ÂÂ * @driver_override: driver name to force a match
> >>> + * @desc: description of remote service
> >>>  ÂÂ * @src: local address
> >>>  ÂÂ * @dst: destination address
> >>>  ÂÂ * @ept: the rpmsg endpoint of this channel @@ -51,6 +54,7 @@
> >>> struct rpmsg_device {
> >>>  ÂÂÂÂÂ struct device dev;
> >>>  ÂÂÂÂÂ struct rpmsg_device_id id;
> >>>  ÂÂÂÂÂ char *driver_override;
> >>> +ÂÂÂ char desc[RPMSG_NAME_SIZE];
> >>>  ÂÂÂÂÂ u32 src;
> >>>  ÂÂÂÂÂ u32 dst;
> >>>  ÂÂÂÂÂ struct rpmsg_endpoint *ept;
> >>>
> >