[PATCH 0/3] rpmsg: smd: Redeliver messages after probe

[PATCH 0/3] rpmsg: smd: Redeliver messages after probe

[Bjorn Andersson]

A race condition exists in SMD where incoming messages might be processed
before we've finished executing the rpmsg device's probe function. The driver's
callback function will in this case be unable to handle the incoming message
and might return an error.

Using the announce_create ops we can invoke the handler for incoming messages
once again after probe returns, solving this issue.

With SMD and QRTR this shows a high failure rate, but there are (at least
theoretical) similar issues in glink and virtio-rpmsg, so this needs to be
further investigated.

Bjorn Andersson (3):
  rpmsg: smd: Fix container_of macros
  rpmsg: Only invoke announce_create for rpdev with endpoints
  rpmsg: smd: Use announce_create to process any receive work

 drivers/rpmsg/qcom_smd.c   | 22 ++++++++++++++++++++--
 drivers/rpmsg/rpmsg_core.c |  2 +-
 2 files changed, 21 insertions(+), 3 deletions(-)

-- 
2.16.2

[PATCH 1/3] rpmsg: smd: Fix container_of macros

[Bjorn Andersson]

The container_of macros should not use the same name for the parameter
as the member to use for lookup, as this will result in a compilation
error unless the passed parameter has the same name as the member.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/rpmsg/qcom_smd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index dabd73bd5577..6bb9d21be6bc 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -167,9 +167,9 @@ struct qcom_smd_endpoint {
 	struct qcom_smd_channel *qsch;
 };
 
-#define to_smd_device(_rpdev)	container_of(_rpdev, struct qcom_smd_device, rpdev)
+#define to_smd_device(r)	container_of(r, struct qcom_smd_device, rpdev)
 #define to_smd_edge(d)		container_of(d, struct qcom_smd_edge, dev)
-#define to_smd_endpoint(ept)	container_of(ept, struct qcom_smd_endpoint, ept)
+#define to_smd_endpoint(e)	container_of(e, struct qcom_smd_endpoint, ept)
 
 /**
  * struct qcom_smd_channel - smd channel struct
-- 
2.16.2

[PATCH 2/3] rpmsg: Only invoke announce_create for rpdev with endpoints

[Bjorn Andersson]

For special rpmsg devices without a primary endpoint there is nothing to
announce so don't call the backend announce create function if we didn't
create an endpoint.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/rpmsg/rpmsg_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index dffa3aab7178..e85d2691d2cf 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -442,7 +442,7 @@ static int rpmsg_dev_probe(struct device *dev)
 		goto out;
 	}
 
-	if (rpdev->ops->announce_create)
+	if (ept && rpdev->ops->announce_create)
 		err = rpdev->ops->announce_create(rpdev);
 out:
 	return err;
-- 
2.16.2

[PATCH 3/3] rpmsg: smd: Use announce_create to process any receive work

[Bjorn Andersson]

It is possible that incoming data arrives before the client driver has
reached a point in the probe method where adequate context for handling
the incoming message has been established.

In the event that the client's callback function returns an error the
message will be left on the FIFO and by invoking the receive handler
after the device has been probed the message will be picked off the FIFO
and the callback invoked again.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/rpmsg/qcom_smd.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index 6bb9d21be6bc..4f6b216522a8 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -996,8 +996,26 @@ static struct device_node *qcom_smd_match_channel(struct device_node *edge_node,
 	return NULL;
 }
 
+static int qcom_smd_announce_create(struct rpmsg_device *rpdev)
+{
+	struct qcom_smd_endpoint *qept = to_smd_endpoint(rpdev->ept);
+	struct qcom_smd_channel *channel = qept->qsch;
+	unsigned long flags;
+	bool kick_state;
+
+	spin_lock_irqsave(&channel->recv_lock, flags);
+	kick_state = qcom_smd_channel_intr(channel);
+	spin_unlock_irqrestore(&channel->recv_lock, flags);
+
+	if (kick_state)
+		schedule_work(&channel->edge->state_work);
+
+	return 0;
+}
+
 static const struct rpmsg_device_ops qcom_smd_device_ops = {
 	.create_ept = qcom_smd_create_ept,
+	.announce_create = qcom_smd_announce_create,
 };
 
 static const struct rpmsg_endpoint_ops qcom_smd_endpoint_ops = {
-- 
2.16.2

RE: [PATCH 2/3] rpmsg: Only invoke announce_create for rpdev with endpoints

[Loic PALLARDY]

Hi Bjorn,

> -----Original Message-----
> From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc-
> owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> Sent: Tuesday, March 27, 2018 11:07 PM
> To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> <bjorn.andersson@linaro.org>
> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> arm-msm@vger.kernel.org
> Subject: [PATCH 2/3] rpmsg: Only invoke announce_create for rpdev with
> endpoints
> 
> For special rpmsg devices without a primary endpoint there is nothing to
> announce so don't call the backend announce create function if we didn't
> create an endpoint.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/rpmsg/rpmsg_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index dffa3aab7178..e85d2691d2cf 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -442,7 +442,7 @@ static int rpmsg_dev_probe(struct device *dev)
>  		goto out;
>  	}
> 
> -	if (rpdev->ops->announce_create)
> +	if (ept && rpdev->ops->announce_create)

This check is already part of virtio_rpmsg.c (see line 341)
	/* 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)) {

should it be part of qcom_smd driver too? (but each implementation will duplicate checks)
Or may have a generic check in the core including rpdev->announce as well (and doing virtio_rpmsg.c clean-up)

Change will become:
	if (rpdev->announce && ept && rpdev->ops->announce_create)

Regards,
Loic
>  		err = rpdev->ops->announce_create(rpdev);
>  out:
>  	return err;
> --
> 2.16.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/3] rpmsg: Only invoke announce_create for rpdev with endpoints

[Loic PALLARDY]

Hi Bjorn,

> -----Original Message-----
> From: Loic PALLARDY
> Sent: Tuesday, April 03, 2018 11:13 AM
> To: 'Bjorn Andersson' <bjorn.andersson@linaro.org>; Ohad Ben-Cohen
> <ohad@wizery.com>
> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> arm-msm@vger.kernel.org
> Subject: RE: [PATCH 2/3] rpmsg: Only invoke announce_create for rpdev with
> endpoints
> 
> Hi Bjorn,
> 
> > -----Original Message-----
> > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-
> remoteproc-
> > owner@vger.kernel.org] On Behalf Of Bjorn Andersson
> > Sent: Tuesday, March 27, 2018 11:07 PM
> > To: Ohad Ben-Cohen <ohad@wizery.com>; Bjorn Andersson
> > <bjorn.andersson@linaro.org>
> > Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-
> > arm-msm@vger.kernel.org
> > Subject: [PATCH 2/3] rpmsg: Only invoke announce_create for rpdev with
> > endpoints
> >
> > For special rpmsg devices without a primary endpoint there is nothing to
> > announce so don't call the backend announce create function if we didn't
> > create an endpoint.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/rpmsg/rpmsg_core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> > index dffa3aab7178..e85d2691d2cf 100644
> > --- a/drivers/rpmsg/rpmsg_core.c
> > +++ b/drivers/rpmsg/rpmsg_core.c
> > @@ -442,7 +442,7 @@ static int rpmsg_dev_probe(struct device *dev)
> >  		goto out;
> >  	}
> >
> > -	if (rpdev->ops->announce_create)
> > +	if (ept && rpdev->ops->announce_create)
> 
> This check is already part of virtio_rpmsg.c (see line 341)
> 	/* 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)) {
> 
> should it be part of qcom_smd driver too? (but each implementation will
> duplicate checks)
> Or may have a generic check in the core including rpdev->announce as well
> (and doing virtio_rpmsg.c clean-up)
> 
> Change will become:
> 	if (rpdev->announce && ept && rpdev->ops->announce_create)
> 
> Regards,
> Loic

I'll appreciate if you reply to the review comments before merging patch and sending pull request.

Regards,
Loic

> >  		err = rpdev->ops->announce_create(rpdev);
> >  out:
> >  	return err;
> > --
> > 2.16.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc"
> in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] rpmsg: Only invoke announce_create for rpdev with endpoints

[Bjorn Andersson]

On Tue 03 Apr 02:12 PDT 2018, Loic PALLARDY wrote:
> > -----Original Message-----
> > From: linux-remoteproc-owner@vger.kernel.org [mailto:linux-remoteproc-
> > owner@vger.kernel.org] On Behalf Of Bjorn Andersson
[..]
> > diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> > index dffa3aab7178..e85d2691d2cf 100644
> > --- a/drivers/rpmsg/rpmsg_core.c
> > +++ b/drivers/rpmsg/rpmsg_core.c
> > @@ -442,7 +442,7 @@ static int rpmsg_dev_probe(struct device *dev)
> >  		goto out;
> >  	}
> > 
> > -	if (rpdev->ops->announce_create)
> > +	if (ept && rpdev->ops->announce_create)
> 
> This check is already part of virtio_rpmsg.c (see line 341)
> 	/* 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)) {

The introduction of rpdev->ept in this check was done by Henri, as he
was implementing rpmsg_char support for virtio_rpmsg:

b2599ebffb2d ("rpmsg: virtio_rpmsg_bus: fix announce for devices without endpoint")

It's there because the rpmsg_device will not have a primary endpoint and
as such there's no communication channel to announce. We could add this
check in each implementation of announce, but I think it's a common
issue.

> 
> should it be part of qcom_smd driver too? (but each implementation will duplicate checks)
> Or may have a generic check in the core including rpdev->announce as well (and doing virtio_rpmsg.c clean-up)
> 

I think we want to remove the ept check in virtio_rpmsg, to reduce that
part of the duplication at least.

> Change will become:
> 	if (rpdev->announce && ept && rpdev->ops->announce_create)
> 

That might make sense, let's see what Wendy comes up with related to
rpmsg_char and supporting Linux-side services, as I suspect that this
handling might be affected.

Regards,
Bjorn