| Message ID | d3c2562c-d0bf-89f3-d0a4-6a51c30a590b@birger-koblitz.de |
|---|---|
| State | Accepted |
| Delegated to: | Daniel Golle |
| Headers | show |
| Series | realtek: fix locking bug in rtl838x_hw_receive() | expand |
Hi Birger, On Fri, 2022-02-18 at 12:01 +0100, Birger Koblitz wrote: > A Locking bug in the packet receive path was introduced with PR > #4973. The following patch prevents the driver from locking > after a few minutes with an endless flow of > > [ 1434.185085] rtl838x-eth 1b00a300.ethernet eth0: Ring contention: r: 0, last > a28000f4, cur a28000f8 > [ 1434.208971] rtl838x-eth 1b00a300.ethernet eth0: Ring contention: r: 0, last > a28000f4, cur a28000fc > [ 1434.794800] rtl838x-eth 1b00a300.ethernet eth0: Ring contention: r: 0, last > a28000f4, cur a28000fc > [ 1435.049187] rtl838x-eth 1b00a300.ethernet eth0: Ring contention: r: 0, last > a28000f4, cur a28000fc > > Signed-off-by: Bjørn Mork <bjorn@mork.no> > Signed-off-by: Birger Koblitz <mail@birger-koblitz.de> If Bjørn signed off on this patch first, he should be the patch author too. (i.e. start this patch with a From: line). This patch sounds like it should have a Fixes: tag as well; if you can find out which patch introduced it. Best, Sander > --- > .../realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/target/linux/realtek/files- > 5.10/drivers/net/ethernet/rtl838x_eth.c b/target/linux/realtek/files- > 5.10/drivers/net/ethernet/rtl838x_eth.c > index ab4a03550bd8..cf6aabc6142f 100644 > --- a/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c > +++ b/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c > @@ -1254,8 +1254,9 @@ static int rtl838x_hw_receive(struct net_device *dev, > int r, int budget) > bool dsa = netdev_uses_dsa(dev); > struct dsa_tag tag; > > - last = (u32 *)KSEG1ADDR(sw_r32(priv->r->dma_if_rx_cur + r * 4)); > pr_debug("---------------------------------------------------------- > RX - %d\n", r); > + spin_lock_irqsave(&priv->lock, flags); > + last = (u32 *)KSEG1ADDR(sw_r32(priv->r->dma_if_rx_cur + r * 4)); > > do { > if ((ring->rx_r[r][ring->c_rx[r]] & 0x1)) { > @@ -1329,7 +1330,6 @@ static int rtl838x_hw_receive(struct net_device *dev, > int r, int budget) > } > > /* Reset header structure */ > - spin_lock_irqsave(&priv->lock, flags); > memset(h, 0, sizeof(struct p_hdr)); > h->buf = data; > h->size = RING_BUFFER; > @@ -1338,12 +1338,13 @@ static int rtl838x_hw_receive(struct net_device *dev, > int r, int budget) > | (ring->c_rx[r] == (priv->rxringlen - 1) ? WRAP : > 0x1); > ring->c_rx[r] = (ring->c_rx[r] + 1) % priv->rxringlen; > last = (u32 *)KSEG1ADDR(sw_r32(priv->r->dma_if_rx_cur + r * > 4)); > - spin_unlock_irqrestore(&priv->lock, flags); > } while (&ring->rx_r[r][ring->c_rx[r]] != last && work_done < budget); > > // Update counters > priv->r->update_cntr(r, 0); > > + spin_unlock_irqrestore(&priv->lock, flags); > + > return work_done; > } >
Sander Vanheule <sander@svanheule.net> writes: > If Bjørn signed off on this patch first, he should be the patch author too. > (i.e. start this patch with a From: line). This patch sounds like it should have > a Fixes: tag as well; if you can find out which patch introduced it. Sorry about the missing Fixes. I meant to add it but was in a hurry and forgot. As for the author part - in principle I'd agree. But this particular change was actually dictated in detail by Birger. So it's not wrong to make him author. Besides, this is really a partial revert which should be merged into the original patch before it goes anywhere. Making it a completely invisible no-op. Bjørn
diff --git a/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c b/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c index ab4a03550bd8..cf6aabc6142f 100644 --- a/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c +++ b/target/linux/realtek/files-5.10/drivers/net/ethernet/rtl838x_eth.c @@ -1254,8 +1254,9 @@ static int rtl838x_hw_receive(struct net_device *dev, int r, int budget) bool dsa = netdev_uses_dsa(dev); struct dsa_tag tag; - last = (u32 *)KSEG1ADDR(sw_r32(priv->r->dma_if_rx_cur + r * 4)); pr_debug("---------------------------------------------------------- RX - %d\n", r); + spin_lock_irqsave(&priv->lock, flags); + last = (u32 *)KSEG1ADDR(sw_r32(priv->r->dma_if_rx_cur + r * 4)); do { if ((ring->rx_r[r][ring->c_rx[r]] & 0x1)) { @@ -1329,7 +1330,6 @@ static int rtl838x_hw_receive(struct net_device *dev, int r, int budget) } /* Reset header structure */ - spin_lock_irqsave(&priv->lock, flags); memset(h, 0, sizeof(struct p_hdr)); h->buf = data; h->size = RING_BUFFER; @@ -1338,12 +1338,13 @@ static int rtl838x_hw_receive(struct net_device *dev, int r, int budget) | (ring->c_rx[r] == (priv->rxringlen - 1) ? WRAP : 0x1); ring->c_rx[r] = (ring->c_rx[r] + 1) % priv->rxringlen; last = (u32 *)KSEG1ADDR(sw_r32(priv->r->dma_if_rx_cur + r * 4)); - spin_unlock_irqrestore(&priv->lock, flags); } while (&ring->rx_r[r][ring->c_rx[r]] != last && work_done < budget); // Update counters priv->r->update_cntr(r, 0); + spin_unlock_irqrestore(&priv->lock, flags); + return work_done; }