Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1549955596-19784-1-git-send-email-xiaoxiang@xiaomi.com> <5cf04a05-938a-2dd0-d36b-e939f0c935ff@st.com>
In-Reply-To: <5cf04a05-938a-2dd0-d36b-e939f0c935ff@st.com>
From:   xiang xiao <xiaoxiang781216@gmail.com>
Date:   Sat, 23 Feb 2019 03:36:21 +0800
Message-ID: <CAH2Cfb9NQeZFuPexG31FR_jNUvRbh_M=5tAgfsSxsZ-UxF91DQ@mail.gmail.com>
Subject: Re: [PATCH] rpmsg: virtio_rpmsg_bus: acknowledge the received
 creation message
To:     Arnaud Pouliquen <arnaud.pouliquen@st.com>
Cc:     Ohad Ben Cohen <ohad@wizery.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        wendy.liang@xilinx.com, Kumar Gala <kumar.gala@linaro.org>,
        linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
        QianWenfa <qianwenfa@xiaomi.com>,
        Xiang Xiao <xiaoxiang@xiaomi.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks for review.

On Fri, Feb 22, 2019 at 6:44 PM Arnaud Pouliquen
<arnaud.pouliquen@st.com> wrote:
>
> Hello Xiang,
>
>
>
> On 2/12/19 8:13 AM, Xiang Xiao wrote:
> > From: QianWenfa <qianwenfa@xiaomi.com>
> >
> > the two phase handsake make the client could initiate the transfer
> > immediately without the server side send any dummy message first.
> As discussed on OpenAMP mailing list, the first point (from my pov) is
> to figure out if this should be part of the rpmsg protocol or service
> dependent (so managed by the rpmsg client itself)...
>

Here is my thought:
1.The ack message don't have any side effect except kernel send back
the local address through NS channel directly.
2.But without this message, remote side can't send the message first
due to the lack of kernel address information.
The second limitation is very strange for all developer I think.

>
> >
> > Signed-off-by: Wenfa Qian <qianwenfa@xiaomi.com>
> > Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 25 ++++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 664f957..e323c98 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -71,6 +71,7 @@ struct virtproc_info {
> >
> >  /* The feature bitmap for virtio rpmsg */
> >  #define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
> > +#define VIRTIO_RPMSG_F_ACK   1 /* RP supports name service acknowledge */
> >
> >  /**
> >   * struct rpmsg_hdr - common header for all rpmsg messages
> > @@ -115,10 +116,12 @@ struct rpmsg_ns_msg {
> >   *
> >   * @RPMSG_NS_CREATE: a new remote service was just created
> >   * @RPMSG_NS_DESTROY: a known remote service was just destroyed
> > + * @RPMSG_NS_ACK: acknowledge the previous creation message
> >   */
> >  enum rpmsg_ns_flags {
> >       RPMSG_NS_CREATE         = 0,
> >       RPMSG_NS_DESTROY        = 1,
> > +     RPMSG_NS_ACK            = 2,
> >  };
> >
> >  /**
> > @@ -330,13 +333,14 @@ 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 &&
> > +     if (rpdev->ept && (rpdev->announce ||
> > +         virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_ACK)) &&
> >           virtio_has_feature(vrp->vdev, VIRTIO_RPMSG_F_NS)) {Regarding this condition i have a concern. If rpdev->announce is false
> but  VIRTIO_RPMSG_F_ACK feature is set, this should generate an ack message.
> Seems that this condition can occurs depending on the rpmsg_device
> struct provided on registration, when rpmsg_dev_probe is called.

The ack message can't be sent before device probe success, otherwise
the early ack make  the remote side consider kernel driver is ready
and send the message immediately, but nobody can receive the message
actually.

>
> >               struct rpmsg_ns_msg nsm;
> >
> >               strncpy(nsm.name, rpdev->id.name, RPMSG_NAME_SIZE);
> >               nsm.addr = rpdev->ept->addr;
> > -             nsm.flags = RPMSG_NS_CREATE;
> > +             nsm.flags = rpdev->announce ? RPMSG_NS_CREATE : RPMSG_NS_ACK;
> >
> >               err = rpmsg_sendto(rpdev->ept, &nsm, sizeof(nsm), RPMSG_NS_ADDR);
> >               if (err)
> > @@ -820,6 +824,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
> >       struct rpmsg_channel_info chinfo;
> >       struct virtproc_info *vrp = priv;
> >       struct device *dev = &vrp->vdev->dev;
> > +     struct device *tmp;
> >       int ret;
> >
> >  #if defined(CONFIG_DYNAMIC_DEBUG)
> > @@ -847,21 +852,30 @@ 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->flags == RPMSG_NS_ACK ? "ack" :
> > +              msg->flags == RPMSG_NS_DESTROY ? "destroy" : "creat",
> >                msg->name, msg->addr);
> >
> >       strncpy(chinfo.name, msg->name, sizeof(chinfo.name));
> >       chinfo.src = RPMSG_ADDR_ANY;
> >       chinfo.dst = msg->addr;
> >
> > -     if (msg->flags & RPMSG_NS_DESTROY) {
> > +     if (msg->flags == RPMSG_NS_DESTROY) {
> >               ret = rpmsg_unregister_device(&vrp->vdev->dev, &chinfo);
> >               if (ret)
> >                       dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret);
> > -     } else {
> > +     } else if (msg->flags == RPMSG_NS_CREATE) {
> >               newch = rpmsg_create_channel(vrp, &chinfo);
> >               if (!newch)
> >                       dev_err(dev, "rpmsg_create_channel failed\n");
> > +     } else if (msg->flags == RPMSG_NS_ACK) {
> > +             chinfo.dst = RPMSG_ADDR_ANY;
> > +             tmp = rpmsg_find_device(&vrp->vdev->dev, &chinfo);
> nit-picking... replace &vrp->vdev->dev by dev and change tmp by a more
> explicit name.

Ok.

> > +             if (tmp) {
> > +                     newch = to_rpmsg_device(tmp);
> > +                     newch->dst = msg->addr;
> > +             } else
> typo: braces
>

Ok.

> Regards,
> Arnaud
> > +                     dev_err(dev, "rpmsg_find_device failed\n");
> >       }
> >
> >       return 0;
> > @@ -1028,6 +1042,7 @@ static struct virtio_device_id id_table[] = {
> >
> >  static unsigned int features[] = {
> >       VIRTIO_RPMSG_F_NS,
> > +     VIRTIO_RPMSG_F_ACK,
> >  };
> >
> >  static struct virtio_driver virtio_ipc_driver = {
> >