Date:   Mon, 12 Aug 2019 16:41:01 -0500
From:   Suman Anna <s-anna@...com>
To:     Fabien DESSENNE <fabien.dessenne@...com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>
CC:     Ohad Ben-Cohen <ohad@...ery.com>,
        Loic PALLARDY <loic.pallardy@...com>,
        Arnaud POULIQUEN <arnaud.pouliquen@...com>,
        "linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Tero Kristo <t-kristo@...com>
Subject: Re: [PATCH] rpmsg: add a description field

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@...ery.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@...ery.com>
>> [s-anna@...com: forward port, add sysfs documentation, fixup qcom drivers]
>> Signed-off-by: Suman Anna <s-anna@...com>
>> [t-kristo@...com: reworked to support both rpmsg with/without the desc field]
>> Signed-off-by: Tero Kristo <t-kristo@...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@...aro.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;
> 

Powered by blists - more mailing lists