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@xxxxxxxxxxxxxx>
> ---
> 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
