net: systemport: Add global locking for descriptor lifecycle

The descriptor list is a shared resource across all of the transmit queues, and
the locking mechanism used today only protects concurrency across a given
transmit queue between the transmit and reclaiming. This creates an opportunity
for the SYSTEMPORT hardware to work on corrupted descriptors if we have
multiple producers at once which is the case when using multiple transmit
queues.

This was particularly noticeable when using multiple flows/transmit queues and
it showed up in interesting ways in that UDP packets would get a correct UDP
header checksum being calculated over an incorrect packet length. Similarly TCP
packets would get an equally correct checksum computed by the hardware over an
incorrect packet length.

The SYSTEMPORT hardware maintains an internal descriptor list that it re-arranges
when the driver produces a new descriptor anytime it writes to the
WRITE_PORT_{HI,LO} registers, there is however some delay in the hardware to
re-organize its descriptors and it is possible that concurrent TX queues
eventually break this internal allocation scheme to the point where the
length/status part of the descriptor gets used for an incorrect data buffer.

The fix is to impose a global serialization for all TX queues in the short
section where we are writing to the WRITE_PORT_{HI,LO} registers which solves
the corruption even with multiple concurrent TX queues being used.

Fixes: 2912eb7400ef ("net: systemport: add Broadcom SYSTEMPORT Ethernet MAC driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Link: https://lore.kernel.org/r/20211215202450.4086240-1-f.fainelli@gmail.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index 40933bf5a71007ca2e31c2b4ffbf23e7d4f92ada..60dde29974bfea53f8092fda078f348d36c6a6ef 100644 (file)
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1309,11 +1309,11 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
        struct bcm_sysport_priv *priv = netdev_priv(dev);
        struct device *kdev = &priv->pdev->dev;
        struct bcm_sysport_tx_ring *ring;
+       unsigned long flags, desc_flags;
        struct bcm_sysport_cb *cb;
        struct netdev_queue *txq;
        u32 len_status, addr_lo;
        unsigned int skb_len;
-       unsigned long flags;
        dma_addr_t mapping;
        u16 queue;
        int ret;
@@ -1373,8 +1373,10 @@ static netdev_tx_t bcm_sysport_xmit(struct sk_buff *skb,
        ring->desc_count--;
 
        /* Ports are latched, so write upper address first */
+       spin_lock_irqsave(&priv->desc_lock, desc_flags);
        tdma_writel(priv, len_status, TDMA_WRITE_PORT_HI(ring->index));
        tdma_writel(priv, addr_lo, TDMA_WRITE_PORT_LO(ring->index));
+       spin_unlock_irqrestore(&priv->desc_lock, desc_flags);
 
        /* Check ring space and update SW control flow */
        if (ring->desc_count == 0)
@@ -2013,6 +2015,7 @@ static int bcm_sysport_open(struct net_device *dev)
        }
 
        /* Initialize both hardware and software ring */
+       spin_lock_init(&priv->desc_lock);
        for (i = 0; i < dev->num_tx_queues; i++) {
                ret = bcm_sysport_init_tx_ring(priv, i);
                if (ret) {

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
index 984f76e74b43effde480fd2ce8f4612d75300a6c..16b73bb9acc783b41aefb2e6198f354d9e9cc6bd 100644 (file)
--- a/drivers/net/ethernet/broadcom/bcmsysport.h
+++ b/drivers/net/ethernet/broadcom/bcmsysport.h
@@ -711,6 +711,7 @@ struct bcm_sysport_priv {
        int                     wol_irq;
 
        /* Transmit rings */
+       spinlock_t              desc_lock;
        struct bcm_sysport_tx_ring *tx_rings;
 
        /* Receive queue */