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

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

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.

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) {