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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html