Good afternoon, Please see my comments inlined in the original thread. On Thu, Jan 30, 2020 at 07:25:54PM +0100, Arnaud POULIQUEN wrote: > 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. I don't think we can move forward without an answer to Arnaud's question, i.e if the concatenation of the basic name service and a descriptor would work. If it is the case I think we can do something even more simple than what Xiang is proposing by tweaking rpmsg_id_match() a little. Something like: static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, const struct rpmsg_device_id *id) { - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; + size_t len = min(strlen(id->name), (size_t)RPMSG_NAME_SIZE); + + /* + * Allow for wildcard matches. For example if rpmsg_driver::id_table + * is: + * + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { + * { .name = "rpmsg-client-sample" }, + * { }, + * } + * + * Then it is possible to support "rpmsg-client-sample*", i.e: + * rpmsg-client-sample_instance0 + * rpmsg-client-sample_instance1 + * ... + * rpmsg-client-sample_instanceX + */ + return strncmp(id->name, rpdev->id.name, len) == 0; } More trickery will be required if we want to support the same service name from more than one remoteproc but the base idea remains. > > > > 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; > > >>> > > >
