Linux-remoteproc Archive on lore.kernel.org
 help / color / Atom feed
From: Arnaud POULIQUEN <arnaud.pouliquen@st.com>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Ohad Ben-Cohen <ohad@wizery.com>,
	"linux-remoteproc@vger.kernel.org"
	<linux-remoteproc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-stm32@st-md-mailman.stormreply.com" 
	<linux-stm32@st-md-mailman.stormreply.com>
Subject: Re: [PATCH 8/9] rpmsg: virtio: use rpmsg_ns driver to manage ns announcement
Date: Wed, 26 Aug 2020 09:42:46 +0200
Message-ID: <c1b81b38-c155-3183-ed67-822c4f87ec71@st.com> (raw)
In-Reply-To: <20200825165433.GA4141387@xps15>

Hi mathieu,

I Sent my V2 few seconds before receiving your comment :)
Please find my answer below

On 8/25/20 6:54 PM, Mathieu Poirier wrote:
> Hi Arnaud,
> 
> On Fri, Jul 31, 2020 at 01:47:31PM +0200, Arnaud Pouliquen wrote:
>> Use the new rpmsg_ns API to send the name service announcements if
>> the VIRTIO_RPMSG_F_NS is set, else just not implement the ops.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  drivers/rpmsg/virtio_rpmsg_bus.c | 94 +++++---------------------------
>>  1 file changed, 13 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index f771fdae150e..3c771a6392be 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -91,35 +91,6 @@ struct rpmsg_hdr {
>>  	u8 data[];
>>  } __packed;
>>  
>> -/**
>> - * struct rpmsg_ns_msg - dynamic name service announcement message
>> - * @name: name of remote service that is published
>> - * @addr: address of remote service that is published
>> - * @flags: indicates whether service is created or destroyed
>> - *
>> - * This message is sent across to publish a new service, or announce
>> - * about its removal. When we receive these messages, an appropriate
>> - * rpmsg channel (i.e device) is created/destroyed. In turn, the ->probe()
>> - * or ->remove() handler of the appropriate rpmsg driver will be invoked
>> - * (if/as-soon-as one is registered).
>> - */
>> -struct rpmsg_ns_msg {
>> -	char name[RPMSG_NAME_SIZE];
>> -	__virtio32 addr;
>> -	__virtio32 flags;
>> -} __packed;
>> -
>> -/**
>> - * enum rpmsg_ns_flags - dynamic name service announcement flags
>> - *
>> - * @RPMSG_NS_CREATE: a new remote service was just created
>> - * @RPMSG_NS_DESTROY: a known remote service was just destroyed
>> - */
>> -enum rpmsg_ns_flags {
>> -	RPMSG_NS_CREATE		= 0,
>> -	RPMSG_NS_DESTROY	= 1,
>> -};
>> -
>>  /**
>>   * @vrp: the remote processor this channel belongs to
>>   */
>> @@ -324,60 +295,18 @@ static void virtio_rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
>>  	__rpmsg_destroy_ept(vch->vrp, ept);
>>  }
>>  
>> -static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
>> -{
>> -	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>> -	struct virtproc_info *vrp = vch->vrp;
>> -	struct device *dev = &rpdev->dev;
>> -	int err = 0;
>> -
>> -	/* need to tell remote processor's name service about this channel ? */
>> -	if (rpdev->announce && rpdev->ept &&
>> -	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
>> -		struct rpmsg_ns_msg nsm;
>> -
>> -		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
>> -		nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
>> -		nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_CREATE);
>> -
>> -		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
>> -		if (err)
>> -			dev_err(dev, "failed to announce service %d\n", err);
>> -	}
>> -
>> -	return err;
>> -}
>> -
>> -static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
>> -{
>> -	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>> -	struct virtproc_info *vrp = vch->vrp;
>> -	struct device *dev = &rpdev->dev;
>> -	int err = 0;
>> -
>> -	/* tell remote processor's name service we're removing this channel */
>> -	if (rpdev->announce && rpdev->ept &&
>> -	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
>> -		struct rpmsg_ns_msg nsm;
>> -
>> -		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
>> -		nsm.addr = cpu_to_virtio32(vrp->vdev, rpdev->ept->addr);
>> -		nsm.flags = cpu_to_virtio32(vrp->vdev, RPMSG_NS_DESTROY);
>> -
>> -		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
>> -		if (err)
>> -			dev_err(dev, "failed to announce service %d\n", err);
>> -	}
>> -
>> -	return err;
>> -}
>> -
>>  static const struct rpmsg_device_ops virtio_rpmsg_ops = {
>>  	.create_channel = virtio_rpmsg_create_channel,
>>  	.release_channel = virtio_rpmsg_release_channel,
>>  	.create_ept = virtio_rpmsg_create_ept,
>> -	.announce_create = virtio_rpmsg_announce_create,
>> -	.announce_destroy = virtio_rpmsg_announce_destroy,
>> +};
>> +
>> +static const struct rpmsg_device_ops virtio_rpmsg_w_nsa_ops = {
>> +	.create_channel = virtio_rpmsg_create_channel,
>> +	.release_channel = virtio_rpmsg_release_channel,
>> +	.create_ept = virtio_rpmsg_create_ept,
>> +	.announce_create = rpmsg_ns_announce_create,
>> +	.announce_destroy = rpmsg_ns_announce_destroy,
>>  };
>>  
>>  static void virtio_rpmsg_release_device(struct device *dev)
>> @@ -423,7 +352,10 @@ __rpmsg_create_channel(struct virtproc_info *vrp,
>>  	rpdev = &vch->rpdev;
>>  	rpdev->src = chinfo->src;
>>  	rpdev->dst = chinfo->dst;
>> -	rpdev->ops = &virtio_rpmsg_ops;
>> +	if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS))
>> +		rpdev->ops = &virtio_rpmsg_w_nsa_ops;
>> +	else
>> +		rpdev->ops = &virtio_rpmsg_ops;
> 
> Yesterday I struggled with this part and I still do this morning.  Function
> __rpmsg_create_channel() can only be called if VIRTIO_RPMSG_F_NS is set so there
> is no need to check it again.

That's right if rpmsg_create_channel is called by the rpmsg_ns only. But it 
could not be the case in future, for instance with the rpmsg_ctrl series [1]
As the channel creation is decorrelate from the ns annoucement we need to check it.

[1]https://patchwork.kernel.org/patch/11694787/

I would also have expected this patch to be a
> simple replace of the .announce_create/destroy functions.  Adding an ops that
> doesn't have the .announce_create/destroy functions looks like a feature to me,
> and one that I don't quite get.
> 
> Do you think you could expand on the motivation behind this patch?

It was my first implementation. It is more of a phylosophical point than anything else.
With this path i tried to implement the following rule:
  if VIRTIO_RPMSG_F_NS is not set
      no ns announcement support
  else 
      delegate to the ns announcement RPMsg service

Rather, legacy implementation is about to implement the announce ops even if not supported.

If this implementation is confusing i can go back to my first implementation which was
only an update the virtio_rpmsg_announce_xx functions:

@@ -322,15 +304,8 @@ static int virtio_rpmsg_announce_create(struct rpmsg_device *rpdev)
 	int err = 0;
 
 	/* need to tell remote processor's name service about this channel ? */
-	if (rpdev->announce && rpdev->ept &&
-	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
-		struct rpmsg_ns_msg nsm;
-
-		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
-		nsm.addr = rpdev->ept->addr;
-		nsm.flags = RPMSG_NS_CREATE;
-
-		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
+	if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
+		err = rpmsg_ctrl_channel_announce(rpdev, RPMSG_NS_CREATE);
 		if (err)
 			dev_err(dev, "failed to announce service %d\n", err);
 	}
@@ -346,15 +321,8 @@ static int virtio_rpmsg_announce_destroy(struct rpmsg_device *rpdev)
 	int err = 0;
 
 	/* tell remote processor's name service we're removing this channel */
-	if (rpdev->announce && rpdev->ept &&
-	    virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
-		struct rpmsg_ns_msg nsm;
-
-		strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
-		nsm.addr = rpdev->ept->addr;
-		nsm.flags = RPMSG_NS_DESTROY;
-
-		err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
+	if (virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {
+		err = rpmsg_ctrl_channel_announce(rpdev, RPMSG_NS_DESTROY);
 		if (err)
 			dev_err(dev, "failed to announce service %d\n", err);
 	}

Regards,
Arnaud

> 
> Thanks,
> Mathieu 
> 
>>  
>>  	/*
>>  	 * rpmsg server channels has predefined local address (for now),
>> @@ -933,7 +865,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>  
>>  		/* Assign public information to the rpmsg_device */
>>  		rpdev_ns = &vch->rpdev;
>> -		rpdev_ns->ops = &virtio_rpmsg_ops;
>> +		rpdev_ns->ops = &virtio_rpmsg_w_nsa_ops;
>>  
>>  		rpdev_ns->dev.parent = &vrp->vdev->dev;
>>  		rpdev_ns->dev.release = virtio_rpmsg_release_device;
>> -- 
>> 2.17.1
>>

  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-31 11:47 [PATCH 0/9] introduce name service announcement rpmsg driver Arnaud Pouliquen
2020-07-31 11:47 ` [PATCH 1/9] rpmsg: virtio: rename rpmsg_create_channel Arnaud Pouliquen
2020-07-31 11:47 ` [PATCH 2/9] rpmsg: core: add channel creation internal API Arnaud Pouliquen
2020-08-24 22:44   ` Mathieu Poirier
2020-07-31 11:47 ` [PATCH 3/9] rpmsg: virtio: add rpmsg channel device ops Arnaud Pouliquen
2020-07-31 11:47 ` [PATCH 4/9] rpmsg: define the name service announcement as reserved address Arnaud Pouliquen
2020-07-31 11:47 ` [PATCH 5/9] rpmsg: introduce reserved rpmsg driver for ns announcement Arnaud Pouliquen
2020-08-05  9:05   ` kernel test robot
2020-08-24 22:47   ` Mathieu Poirier
2020-08-25 11:57     ` Arnaud POULIQUEN
2020-08-25 14:36       ` Mathieu Poirier
2020-07-31 11:47 ` [PATCH 6/9] rpmsg: virtio: use rpmsg ns device for the " Arnaud Pouliquen
2020-08-24 22:48   ` Mathieu Poirier
2020-08-25 12:02     ` Arnaud POULIQUEN
2020-07-31 11:47 ` [PATCH 7/9] rpmsg: ns: add name service announcement service Arnaud Pouliquen
2020-07-31 11:47 ` [PATCH 8/9] rpmsg: virtio: use rpmsg_ns driver to manage ns announcement Arnaud Pouliquen
2020-08-25 16:54   ` Mathieu Poirier
2020-08-26  7:42     ` Arnaud POULIQUEN [this message]
2020-08-26 22:10       ` Mathieu Poirier
2020-08-27  7:03         ` Arnaud POULIQUEN
2020-07-31 11:47 ` [PATCH 9/9] rpmsg: ns: name service announcement endianness Arnaud Pouliquen
2020-08-24 23:04   ` Mathieu Poirier
2020-08-20 22:32 ` [PATCH 0/9] introduce name service announcement rpmsg driver Mathieu Poirier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c1b81b38-c155-3183-ed67-822c4f87ec71@st.com \
    --to=arnaud.pouliquen@st.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=mathieu.poirier@linaro.org \
    --cc=ohad@wizery.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-remoteproc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-remoteproc/0 linux-remoteproc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-remoteproc linux-remoteproc/ https://lore.kernel.org/linux-remoteproc \
		linux-remoteproc@vger.kernel.org
	public-inbox-index linux-remoteproc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-remoteproc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git