| Message ID | 20211219024925.18936-1-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [stable,4.19] net: systemport: Add global locking for descriptor lifecycle | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Sat, Dec 18, 2021 at 06:49:25PM -0800, Florian Fainelli wrote: > commit 8b8e6e782456f1ce02a7ae914bbd5b1053f0b034 upstream > > 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: 80105befdb4b ("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> > --- > drivers/net/ethernet/broadcom/bcmsysport.c | 5 +++++ > drivers/net/ethernet/broadcom/bcmsysport.h | 1 + > 2 files changed, 6 insertions(+) All now queued up, thanks. greg k-h
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index 0c69becc3c17..b3fc8745b580 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -120,9 +120,13 @@ static inline void tdma_port_write_desc_addr(struct bcm_sysport_priv *priv, struct dma_desc *desc, unsigned int port) { + unsigned long desc_flags; + /* Ports are latched, so write upper address first */ + spin_lock_irqsave(&priv->desc_lock, desc_flags); tdma_writel(priv, desc->addr_status_len, TDMA_WRITE_PORT_HI(port)); tdma_writel(priv, desc->addr_lo, TDMA_WRITE_PORT_LO(port)); + spin_unlock_irqrestore(&priv->desc_lock, desc_flags); } /* Ethtool operations */ @@ -2003,6 +2007,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 36e0adf5c9b8..f438b818136a 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.h +++ b/drivers/net/ethernet/broadcom/bcmsysport.h @@ -751,6 +751,7 @@ struct bcm_sysport_priv { int wol_irq; /* Transmit rings */ + spinlock_t desc_lock; struct bcm_sysport_tx_ring *tx_rings; /* Receive queue */
commit 8b8e6e782456f1ce02a7ae914bbd5b1053f0b034 upstream 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: 80105befdb4b ("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> --- drivers/net/ethernet/broadcom/bcmsysport.c | 5 +++++ drivers/net/ethernet/broadcom/bcmsysport.h | 1 + 2 files changed, 6 insertions(+)