Received-SPF: pass (google.com: domain of linux-kernel+bounces-144135-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Feedback-ID: i80c9496c:Fastmail
Date: Sun, 14 Apr 2024 14:17:19 +0200
From: Niklas =?utf-8?Q?S=C3=B6derlund?= <niklas.soderlund+renesas@ragnatech.se>
To: Paul Barker <paul.barker.ct@bp.renesas.com>
Cc: Sergey Shtylyov <s.shtylyov@omp.ru>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net 2/4] net: ravb: Allow RX loop to move past DMA
 mapping errors
Message-ID: <20240414121719.GB2860391@ragnatech.se>
References: <20240411114434.26186-1-paul.barker.ct@bp.renesas.com>
 <20240411114434.26186-3-paul.barker.ct@bp.renesas.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240411114434.26186-3-paul.barker.ct@bp.renesas.com>

Hi Paul,

Thanks for your patch.

On 2024-04-11 12:44:31 +0100, Paul Barker wrote:
> The RX loops in ravb_rx_gbeth() and ravb_rx_rcar() skip to the next loop
> interation if a zero-length descriptor is seen (indicating a DMA mapping
> error). However, the current rx descriptor index `priv->cur_rx[q]` was
> incremented at the end of the loop and so would not be incremented when
> we skip to the next loop iteration. This would cause the loop to keep
> seeing the same zero-length descriptor instead of moving on to the next
> descriptor.
> 
> As the loop counter `i` still increments, the loop would eventually
> terminate so there is no risk of being stuck here forever - but we
> should still fix this to avoid wasting cycles.
> 
> To fix this, the rx descriptor index is incremented at the top of the
> loop, in the for statement itself. The assignments of `entry` and `desc`
> are brought into the loop to avoid the need for duplication.
> 
> Fixes: d8b48911fd24 ("ravb: fix ring memory allocation")
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 70f2900648d4..028ab5c6aaf7 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -775,12 +775,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>  	int limit;
>  	int i;
>  
> -	entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>  	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>  	stats = &priv->stats[q];
>  
> -	desc = &priv->rx_ring[q].desc[entry];
> -	for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> +	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> +		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> +		desc = &priv->rx_ring[q].desc[entry];
> +		if (desc->die_dt == DT_FEMPTY)
> +			break;
> +
>  		/* Descriptor type must be checked before all other reads */
>  		dma_rmb();
>  		desc_status = desc->msc;
> @@ -848,9 +851,6 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>  				break;
>  			}
>  		}
> -
> -		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
> -		desc = &priv->rx_ring[q].desc[entry];
>  	}
>  
>  	/* Refill the RX ring buffers. */
> @@ -891,7 +891,6 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *info = priv->info;
> -	int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>  	struct net_device_stats *stats = &priv->stats[q];
>  	struct ravb_ex_rx_desc *desc;
>  	struct sk_buff *skb;
> @@ -900,12 +899,17 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  	int rx_packets = 0;
>  	u8  desc_status;
>  	u16 pkt_len;
> +	int entry;
>  	int limit;
>  	int i;
>  
>  	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> -	desc = &priv->rx_ring[q].ex_desc[entry];
> -	for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> +	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> +		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> +		desc = &priv->rx_ring[q].ex_desc[entry];
> +		if (desc->die_dt == DT_FEMPTY)
> +			break;

I really like moving the assignment of entry and desc to the top of the 
loop. But I don't like the loop limits as it's hard, at least for me, to 
immediately see what's going on. How about,

    limit = min(priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q], *quota);

    for (i = 0; i < limit; i++) {
        entry = priv->cur_rx[q] % priv->num_rx_ring[q];
        desc = &priv->rx_ring[q].ex_desc[entry];

        /* There are no more valid descriptors after an empty one. */
        if (desc->die_dt == DT_FEMPTY)
                break;

        ...
    }

> +
>  		/* Descriptor type must be checked before all other reads */
>  		dma_rmb();
>  		desc_status = desc->msc;
> @@ -959,9 +963,6 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  			rx_packets++;
>  			stats->rx_bytes += pkt_len;
>  		}
> -
> -		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
> -		desc = &priv->rx_ring[q].ex_desc[entry];
>  	}
>  
>  	/* Refill the RX ring buffers. */
> -- 
> 2.39.2
> 

-- 
Kind Regards,
Niklas Söderlund