Re: [PATCH 1/1] rpmsg: virtio_rpmsg_bus - prevent possible race condition

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Mon, Sep 04, 2023 at 03:52:56PM +0200, Arnaud POULIQUEN wrote:
> Hello Tim,
> 
> On 9/4/23 10:36, Tim Blechmann wrote:
> > when we cannot get a tx buffer (`get_a_tx_buf`) `rpmsg_upref_sleepers`
> > enables tx-complete interrupt.
> > however if the interrupt is executed after `get_a_tx_buf` and before
> > `rpmsg_upref_sleepers` we may mis the tx-complete interrupt and sleep
> > for the full 15 seconds.
> 
> 
> Is there any reason why your co-processor is unable to release the TX RPMSG
> buffers for 15 seconds? If not, you should first determine the reason why it is
> stalled.

Arnaud's concern is valid.  If the remote processor can't consume a buffer
within 15 seconds, something is probably wrong.

That said, I believe your assesment of the situation is correct.  *If* the TX
callback is disabled and there is no buffer available, there is a window of
opportunity between calls to get_a_tx_buf() and rpmsg_upref_sleepers() for an
interrupt to arrive in function rpmsg_send_offchannel_raw().  

From here three things need to happen:

1) You send another version of this patch with a changelong that uses proper
english, i.e capital letters when they are needed and no spelling mistake.

2) Arnaud confirms our suspicions.

3) This patch gets applied when rc1 comes out so that it has 6 or 7 weeks to
soak.  No error are locks are reported due to this patch during that time. 

> 
> Regards,
> Arnaud
> 
> > 
> > in this case, so we re-try once before we really start to sleep
> > 
> > Signed-off-by: Tim Blechmann <tim@klingt.org>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 905ac7910c98..2a9d42225e60 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -587,21 +587,27 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
> >  
> >  	/* no free buffer ? wait for one (but bail after 15 seconds) */
> >  	while (!msg) {
> >  		/* enable "tx-complete" interrupts, if not already enabled */
> >  		rpmsg_upref_sleepers(vrp);
> >  
> > -		/*
> > -		 * sleep until a free buffer is available or 15 secs elapse.
> > -		 * the timeout period is not configurable because there's
> > -		 * little point in asking drivers to specify that.
> > -		 * if later this happens to be required, it'd be easy to add.
> > -		 */
> > -		err = wait_event_interruptible_timeout(vrp->sendq,
> > -					(msg = get_a_tx_buf(vrp)),
> > -					msecs_to_jiffies(15000));
> > +		/* make sure to retry to grab tx buffer before we start waiting */
> > +		msg = get_a_tx_buf(vrp);
> > +		if (msg) {
> > +			err = 0;
> > +		} else {
> > +			/*
> > +			 * sleep until a free buffer is available or 15 secs elapse.
> > +			 * the timeout period is not configurable because there's
> > +			 * little point in asking drivers to specify that.
> > +			 * if later this happens to be required, it'd be easy to add.
> > +			 */
> > +			err = wait_event_interruptible_timeout(vrp->sendq,
> > +						(msg = get_a_tx_buf(vrp)),
> > +						msecs_to_jiffies(15000));
> > +		}
> >  
> >  		/* disable "tx-complete" interrupts if we're the last sleeper */
> >  		rpmsg_downref_sleepers(vrp);
> >  
> >  		/* timeout ? */
> >  		if (!err) {