| Message ID | 20190809214005.32159-1-s-anna@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rpmsg: add a description field | expand |
Hi Suman, See my remarks below BR Fabien On 09/08/2019 11:40 PM, Suman Anna wrote: > From: Ohad Ben-Cohen <ohad@wizery.com> > > 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. > > Based on an initial patch from Ohad Ben-Cohen. > > Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com> > [s-anna@ti.com: forward port, add sysfs documentation, fixup qcom drivers] > Signed-off-by: Suman Anna <s-anna@ti.com> > [t-kristo@ti.com: reworked to support both rpmsg with/without the desc field] > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > Documentation/ABI/testing/sysfs-bus-rpmsg | 29 +++++++++++++++ > drivers/rpmsg/qcom_glink_native.c | 1 + > drivers/rpmsg/qcom_smd.c | 1 + Should not you extend qcom_glink_create_ept() and qcom_smd_create_ept() so they compare both 'name' AND 'desc' (just like this is proposed in rpmsg_device_match()) ? > drivers/rpmsg/rpmsg_char.c | 1 + > drivers/rpmsg/rpmsg_core.c | 6 ++++ > drivers/rpmsg/virtio_rpmsg_bus.c | 44 +++++++++++++++++++---- > drivers/soc/qcom/wcnss_ctrl.c | 1 + > include/linux/rpmsg.h | 4 +++ > 8 files changed, 80 insertions(+), 7 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@linaro.org> > +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); > }; It would be good to add DEVICE_ATTR_RO(desc) / desc_show() > diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c > index ea88fd4e2a6e..334a50425b5c 100644 > --- a/drivers/rpmsg/rpmsg_core.c > +++ b/drivers/rpmsg/rpmsg_core.c > @@ -302,6 +302,10 @@ static int rpmsg_device_match(struct device *dev, void *data) > if (strncmp(chinfo->name, rpdev->id.name, RPMSG_NAME_SIZE)) > return 0; > > + if (chinfo->desc && chinfo->desc != rpdev->desc && chinfo->desc is defined as 'char desc[RPMSG_NAME_SIZE]', so I do not think that you shall check chinfo->desc (can't be NULL) Did you want to check *chinfo->desc ? (desc differs from '\0') I can't understand the "chinfo->desc != rpdec->desc" test. > + strncmp(chinfo->desc, rpdev->desc, RPMSG_NAME_SIZE)) > + return 0; > + > /* found a match ! */ > return 1; > } > @@ -365,6 +369,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 +391,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, What about adding strncpy(desc) in rpmsg_dev_probe() ? > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c > index 376ebbf880d6..49901582ff0e 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]; It may be better to use a dedicated value for desc length (#define RPMSG_DESC_SIZE). I also wonder if 32 chars are enough for a (text) description. > + u32 addr; > + u32 flags; > +} __packed; > + > /** > * enum rpmsg_ns_flags - dynamic name service announcement flags > * > @@ -402,8 +419,9 @@ static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp, > if (tmp) { > /* decrement the matched device's refcount back */ > put_device(tmp); > - dev_err(dev, "channel %s:%x:%x already exist\n", > - chinfo->name, chinfo->src, chinfo->dst); > + dev_err(dev, "channel %s:%s:%x:%x already exist\n", > + chinfo->name, chinfo->desc, > + chinfo->src, chinfo->dst); > return NULL; > } > > @@ -419,6 +437,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 +835,29 @@ 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; > + strncpy(chinfo.desc, msg_ext->desc, sizeof(chinfo.desc)); > + } else if (len != sizeof(*msg)) { shall be 'else', not 'else if' > dev_err(dev, "malformed ns msg (%d)\n", len); > return -EINVAL; > } > @@ -847,14 +877,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); > } > 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;
Hi Fabien, On 8/12/19 8:43 AM, Fabien DESSENNE wrote: > Hi Suman, > > See my remarks below Thank you for all the review comments. > > BR > > Fabien > > > On 09/08/2019 11:40 PM, Suman Anna wrote: >> From: Ohad Ben-Cohen <ohad@wizery.com> >> >> 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. >> >> Based on an initial patch from Ohad Ben-Cohen. >> >> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com> >> [s-anna@ti.com: forward port, add sysfs documentation, fixup qcom drivers] >> Signed-off-by: Suman Anna <s-anna@ti.com> >> [t-kristo@ti.com: reworked to support both rpmsg with/without the desc field] >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> Documentation/ABI/testing/sysfs-bus-rpmsg | 29 +++++++++++++++ >> drivers/rpmsg/qcom_glink_native.c | 1 + >> drivers/rpmsg/qcom_smd.c | 1 + > > > Should not you extend qcom_glink_create_ept() and qcom_smd_create_ept() > so they compare both 'name' AND 'desc' (just like this is proposed in > rpmsg_device_match()) ? I did not understand your comment against the qcom create_ept()s, we do not initialize it for the virtio_rpmsg_create_ept() either. The desc field is only for the rpmsg_device and is filled in before the rpmsg_register_device() function, and is already zero-initialized for the qcom transports in the qcom_glink_rx_open() and qcom_smd_create_device() functions. > > >> drivers/rpmsg/rpmsg_char.c | 1 + >> drivers/rpmsg/rpmsg_core.c | 6 ++++ >> drivers/rpmsg/virtio_rpmsg_bus.c | 44 +++++++++++++++++++---- >> drivers/soc/qcom/wcnss_ctrl.c | 1 + >> include/linux/rpmsg.h | 4 +++ >> 8 files changed, 80 insertions(+), 7 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@linaro.org> >> +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); >> }; > > > It would be good to add DEVICE_ATTR_RO(desc) / desc_show() I can add it, but it serves no purpose today on the rpmsg_char driver. It is not filled in with any value of use today. > > >> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c >> index ea88fd4e2a6e..334a50425b5c 100644 >> --- a/drivers/rpmsg/rpmsg_core.c >> +++ b/drivers/rpmsg/rpmsg_core.c >> @@ -302,6 +302,10 @@ static int rpmsg_device_match(struct device *dev, void *data) >> if (strncmp(chinfo->name, rpdev->id.name, RPMSG_NAME_SIZE)) >> return 0; >> >> + if (chinfo->desc && chinfo->desc != rpdev->desc && > > > chinfo->desc is defined as 'char desc[RPMSG_NAME_SIZE]', so I do not > think that you shall check chinfo->desc (can't be NULL) > > Did you want to check *chinfo->desc ? (desc differs from '\0') > > I can't understand the "chinfo->desc != rpdec->desc" test. Yeah, this can do with some cleanup. > > >> + strncmp(chinfo->desc, rpdev->desc, RPMSG_NAME_SIZE)) >> + return 0; >> + >> /* found a match ! */ >> return 1; >> } >> @@ -365,6 +369,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 +391,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, > > > What about adding strncpy(desc) in rpmsg_dev_probe() ? chinfo variable in rpmsg_dev_probe is zero initialized. Again, we are not using the desc for the endpoints, only the rpmsg_device, and that is already copied before rpmsg_register_device(). > > >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c >> index 376ebbf880d6..49901582ff0e 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]; > > > It may be better to use a dedicated value for desc length (#define > RPMSG_DESC_SIZE). I still expect this to be a single word. If we were to increase this, how many characters do you think it should be? I picked the same size as the name field, 32 characters is decently large enough. > > I also wonder if 32 chars are enough for a (text) description. desc is kinda a misnomer (coming from original patch), it is supposed to serve the equivalent of a dev_name since the virtio rpmsg devices are kinda non-DT legacy-style, name field serves as the compatible. > > >> + u32 addr; >> + u32 flags; >> +} __packed; >> + >> /** >> * enum rpmsg_ns_flags - dynamic name service announcement flags >> * >> @@ -402,8 +419,9 @@ static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp, >> if (tmp) { >> /* decrement the matched device's refcount back */ >> put_device(tmp); >> - dev_err(dev, "channel %s:%x:%x already exist\n", >> - chinfo->name, chinfo->src, chinfo->dst); >> + dev_err(dev, "channel %s:%s:%x:%x already exist\n", >> + chinfo->name, chinfo->desc, >> + chinfo->src, chinfo->dst); >> return NULL; >> } >> >> @@ -419,6 +437,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 +835,29 @@ 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; >> + strncpy(chinfo.desc, msg_ext->desc, sizeof(chinfo.desc)); >> + } else if (len != sizeof(*msg)) { > > > shall be 'else', not 'else if' Thanks for catching, will fix it in the next version. regards Suman > > >> dev_err(dev, "malformed ns msg (%d)\n", len); >> return -EINVAL; >> } >> @@ -847,14 +877,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); >> } >> 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; >
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@linaro.org> +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..334a50425b5c 100644 --- a/drivers/rpmsg/rpmsg_core.c +++ b/drivers/rpmsg/rpmsg_core.c @@ -302,6 +302,10 @@ static int rpmsg_device_match(struct device *dev, void *data) if (strncmp(chinfo->name, rpdev->id.name, RPMSG_NAME_SIZE)) return 0; + if (chinfo->desc && chinfo->desc != rpdev->desc && + strncmp(chinfo->desc, rpdev->desc, RPMSG_NAME_SIZE)) + return 0; + /* found a match ! */ return 1; } @@ -365,6 +369,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 +391,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 376ebbf880d6..49901582ff0e 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 * @@ -402,8 +419,9 @@ static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp, if (tmp) { /* decrement the matched device's refcount back */ put_device(tmp); - dev_err(dev, "channel %s:%x:%x already exist\n", - chinfo->name, chinfo->src, chinfo->dst); + dev_err(dev, "channel %s:%s:%x:%x already exist\n", + chinfo->name, chinfo->desc, + chinfo->src, chinfo->dst); return NULL; } @@ -419,6 +437,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 +835,29 @@ 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; + strncpy(chinfo.desc, msg_ext->desc, sizeof(chinfo.desc)); + } else if (len != sizeof(*msg)) { dev_err(dev, "malformed ns msg (%d)\n", len); return -EINVAL; } @@ -847,14 +877,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); } 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;