net: renesas: rswitch: fix race window between tx start and complete

If hardware is already transmitting, it can start handling the
descriptor being written to immediately after it observes updated DT
field, before the queue is kicked by a write to GWTRC.

If the start_xmit() execution is preempted at unfortunate moment, this
transmission can complete, and interrupt handled, before gq->cur gets
updated. With the current implementation of completion, this will cause
the last entry not completed.

Fix that by changing completion loop to check DT values directly, instead
of depending on gq->cur.

Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"")
Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Link: https://patch.msgid.link/20241208095004.69468-3-nikita.yoush@cogentembedded.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index f4fc2e7212c3c35906b6691c4f0550a8433c6c0e..cdf4d6ccccb05b3bbae870761faf21fce2c81613 100644 (file)
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -862,13 +862,10 @@ static void rswitch_tx_free(struct net_device *ndev)
        struct rswitch_ext_desc *desc;
        struct sk_buff *skb;
 
-       for (; rswitch_get_num_cur_queues(gq) > 0;
-            gq->dirty = rswitch_next_queue_index(gq, false, 1)) {
-               desc = &gq->tx_ring[gq->dirty];
-               if ((desc->desc.die_dt & DT_MASK) != DT_FEMPTY)
-                       break;
-
+       desc = &gq->tx_ring[gq->dirty];
+       while ((desc->desc.die_dt & DT_MASK) == DT_FEMPTY) {
                dma_rmb();
+
                skb = gq->skbs[gq->dirty];
                if (skb) {
                        rdev->ndev->stats.tx_packets++;
@@ -879,7 +876,10 @@ static void rswitch_tx_free(struct net_device *ndev)
                        dev_kfree_skb_any(gq->skbs[gq->dirty]);
                        gq->skbs[gq->dirty] = NULL;
                }
+
                desc->desc.die_dt = DT_EEMPTY;
+               gq->dirty = rswitch_next_queue_index(gq, false, 1);
+               desc = &gq->tx_ring[gq->dirty];
        }
 }
 
@@ -1685,6 +1685,8 @@ static netdev_tx_t rswitch_start_xmit(struct sk_buff *skb, struct net_device *nd
        gq->skbs[(gq->cur + nr_desc - 1) % gq->ring_size] = skb;
        gq->unmap_addrs[(gq->cur + nr_desc - 1) % gq->ring_size] = dma_addr_orig;
 
+       dma_wmb();
+
        /* DT_FSTART should be set at last. So, this is reverse order. */
        for (i = nr_desc; i-- > 0; ) {
                desc = &gq->tx_ring[rswitch_next_queue_index(gq, true, i)];
@@ -1695,8 +1697,6 @@ static netdev_tx_t rswitch_start_xmit(struct sk_buff *skb, struct net_device *nd
                        goto err_unmap;
        }
 
-       wmb();  /* gq->cur must be incremented after die_dt was set */
-
        gq->cur = rswitch_next_queue_index(gq, true, nr_desc);
        rswitch_modify(rdev->addr, GWTRC(gq->index), 0, BIT(gq->index % 32));