[1/1] rpmsg: virtio_rpmsg_bus: Fix rpmsg_remove() release order
diff mbox series

Message ID 1532552826-30003-1-git-send-email-loic.pallardy@st.com
State New
Headers show
Series
  • [1/1] rpmsg: virtio_rpmsg_bus: Fix rpmsg_remove() release order
Related show

Commit Message

Loic Pallardy July 25, 2018, 9:07 p.m. UTC
In rpmsg_remove() procedure, children are removed then Name
Service announcement endpoint is destroyed and device released.

Issue with current implementation is if a NS announcement
creation request is ongoing (received before virtio reset),
child device could be created after children removing.
In that case rpmsg_remove() is freeing rpmsg_virtio_bus,
leading to an inconsistent state.

This patch is destroying NS announcement service endpoint before
removing children to guarantee that list of child devices is
fixed and all of them will be correctly removed before cleaning
resources.

fixes: bcabbccabffe ("rpmsg: Split off generic tail of create_channel()")

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Suman Anna July 25, 2018, 10:32 p.m. UTC | #1
Hi Loic,

On 07/25/2018 04:07 PM, Loic Pallardy wrote:
> In rpmsg_remove() procedure, children are removed then Name
> Service announcement endpoint is destroyed and device released.
> 
> Issue with current implementation is if a NS announcement
> creation request is ongoing (received before virtio reset),
> child device could be created after children removing.
> In that case rpmsg_remove() is freeing rpmsg_virtio_bus,
> leading to an inconsistent state.
> 
> This patch is destroying NS announcement service endpoint before
> removing children to guarantee that list of child devices is
> fixed and all of them will be correctly removed before cleaning
> resources.

I do not think this is the right fix for the problem you explained.
The removal of rpmsg devices can actually invoke the announce_destroy
in the rpmsg_dev_remove().

regards
Suman

> 
> fixes: bcabbccabffe ("rpmsg: Split off generic tail of create_channel()")
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 664f957..f5d481f 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -1004,13 +1004,13 @@ static void rpmsg_remove(struct virtio_device *vdev)
>  
>  	vdev->config->reset(vdev);
>  
> +	if (vrp->ns_ept)
> +		__rpmsg_destroy_ept(vrp, vrp->ns_ept);
> +
>  	ret = device_for_each_child(&vdev->dev, NULL, rpmsg_remove_device);
>  	if (ret)
>  		dev_warn(&vdev->dev, "can't remove rpmsg device: %d\n", ret);
>  
> -	if (vrp->ns_ept)
> -		__rpmsg_destroy_ept(vrp, vrp->ns_ept);
> -
>  	idr_destroy(&vrp->endpoints);
>  
>  	vdev->config->del_vqs(vrp->vdev);
> 

--
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
Loic Pallardy July 26, 2018, 7:09 a.m. UTC | #2
> -----Original Message-----
> From: Suman Anna <s-anna@ti.com>
> Sent: Thursday, July 26, 2018 12:32 AM
> To: Loic PALLARDY <loic.pallardy@st.com>; bjorn.andersson@linaro.org;
> ohad@wizery.com
> Cc: linux-remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org;
> Arnaud POULIQUEN <arnaud.pouliquen@st.com>;
> benjamin.gaignard@linaro.org
> Subject: Re: [PATCH 1/1] rpmsg: virtio_rpmsg_bus: Fix rpmsg_remove()
> release order
> 
> Hi Loic,
> 
> On 07/25/2018 04:07 PM, Loic Pallardy wrote:
> > In rpmsg_remove() procedure, children are removed then Name
> > Service announcement endpoint is destroyed and device released.
> >
> > Issue with current implementation is if a NS announcement
> > creation request is ongoing (received before virtio reset),
> > child device could be created after children removing.
> > In that case rpmsg_remove() is freeing rpmsg_virtio_bus,
> > leading to an inconsistent state.
> >
> > This patch is destroying NS announcement service endpoint before
> > removing children to guarantee that list of child devices is
> > fixed and all of them will be correctly removed before cleaning
> > resources.
> 
> I do not think this is the right fix for the problem you explained.
> The removal of rpmsg devices can actually invoke the announce_destroy
> in the rpmsg_dev_remove().

Yes it was one of my interrogation correcting it like that. But as virtio link has been put in reset state just before, no message could be sent to coprocessor...
Other possible correction (maybe less elegant), keeping current order, is to introduce a rpmsg bus "status" that will be checked before registering new device.
Anyway, we need to prevent new rpmsg device register after children removal to avoid orphan device.

Regards,
Loic
> 
> regards
> Suman
> 
> >
> > fixes: bcabbccabffe ("rpmsg: Split off generic tail of create_channel()")
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 664f957..f5d481f 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -1004,13 +1004,13 @@ static void rpmsg_remove(struct virtio_device
> *vdev)
> >
> >  	vdev->config->reset(vdev);
> >
> > +	if (vrp->ns_ept)
> > +		__rpmsg_destroy_ept(vrp, vrp->ns_ept);
> > +
> >  	ret = device_for_each_child(&vdev->dev, NULL,
> rpmsg_remove_device);
> >  	if (ret)
> >  		dev_warn(&vdev->dev, "can't remove rpmsg device: %d\n",
> ret);
> >
> > -	if (vrp->ns_ept)
> > -		__rpmsg_destroy_ept(vrp, vrp->ns_ept);
> > -
> >  	idr_destroy(&vrp->endpoints);
> >
> >  	vdev->config->del_vqs(vrp->vdev);
> >

Patch
diff mbox series

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 664f957..f5d481f 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -1004,13 +1004,13 @@  static void rpmsg_remove(struct virtio_device *vdev)
 
 	vdev->config->reset(vdev);
 
+	if (vrp->ns_ept)
+		__rpmsg_destroy_ept(vrp, vrp->ns_ept);
+
 	ret = device_for_each_child(&vdev->dev, NULL, rpmsg_remove_device);
 	if (ret)
 		dev_warn(&vdev->dev, "can't remove rpmsg device: %d\n", ret);
 
-	if (vrp->ns_ept)
-		__rpmsg_destroy_ept(vrp, vrp->ns_ept);
-
 	idr_destroy(&vrp->endpoints);
 
 	vdev->config->del_vqs(vrp->vdev);