Re: [PATCH] net: renesas: ravb: Fix a stuck issue when a lot of frames are received

[Sergei Shtylyov <sergei.shtylyov@xxxxxxxxx>]

Hello!

On 4/21/21 7:52 AM, Yoshihiro Shimoda wrote:

   Posting a review of the already commited (over my head) patch. It would have
been appropriate if the patch looked OK but it's not. :-/

> When a lot of frames were received in the short term, the driver
> caused a stuck of receiving until a new frame was received. For example,
> the following command from other device could cause this issue.
> 
>     $ sudo ping -f -l 1000 -c 1000 <this driver's ipaddress>

   -l is essential here, right?
   Have you tried testing sh_eth sriver like that, BTW?

> The previous code always cleared the interrupt flag of RX but checks
> the interrupt flags in ravb_poll(). So, ravb_poll() could not call
> ravb_rx() in the next time until a new RX frame was received if
> ravb_rx() returned true. To fix the issue, always calls ravb_rx()
> regardless the interrupt flags condition.

   That bacially defeats the purpose of IIUC...

> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 35 ++++++++----------------
>  1 file changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index eb0c03bdb12d..cad57d58d764 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -911,31 +911,20 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	int q = napi - priv->napi;
>  	int mask = BIT(q);
>  	int quota = budget;
> -	u32 ris0, tis;
>  
> -	for (;;) {
> -		tis = ravb_read(ndev, TIS);
> -		ris0 = ravb_read(ndev, RIS0);
> -		if (!((ris0 & mask) || (tis & mask)))
> -			break;
> +	/* Processing RX Descriptor Ring */
> +	/* Clear RX interrupt */

   I think these 2 coments should've been collapsed...
 
> +	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
> +	if (ravb_rx(ndev, &quota, q))
> +		goto out;
>  
> -		/* Processing RX Descriptor Ring */
> -		if (ris0 & mask) {
> -			/* Clear RX interrupt */
> -			ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
> -			if (ravb_rx(ndev, &quota, q))
> -				goto out;

   This jumps over the TX NAPI code, not good... Seems like another bug.

> -		}
> -		/* Processing TX Descriptor Ring */
> -		if (tis & mask) {
> -			spin_lock_irqsave(&priv->lock, flags);
> -			/* Clear TX interrupt */
> -			ravb_write(ndev, ~(mask | TIS_RESERVED), TIS);
> -			ravb_tx_free(ndev, q, true);
> -			netif_wake_subqueue(ndev, q);
> -			spin_unlock_irqrestore(&priv->lock, flags);
> -		}
> -	}
> +	/* Processing RX Descriptor Ring */

   TX!

> +	spin_lock_irqsave(&priv->lock, flags);
> +	/* Clear TX interrupt */
> +	ravb_write(ndev, ~(mask | TIS_RESERVED), TIS);
> +	ravb_tx_free(ndev, q, true);
> +	netif_wake_subqueue(ndev, q);
> +	spin_unlock_irqrestore(&priv->lock, flags);
>  
>  	napi_complete(napi);
>  

MBR, Sergei