[PATCH 0/5] net: macb: Fixes

[PATCH 0/5] net: macb: Fixes

[Soren Brinkmann]

Hi Nicolas,

I think I found the cause of the issue I told you about. Looks like
driver and HW are racing (a few more details in the commit message).
On my way finding that, I found a few more minor issues which are fixed
in the first patches.
The last one is "fixing" the actual race. I don't know if I overlooked
anything here, but this seems to work fine on Zynq.

	Thanks,
	Sören


Soren Brinkmann (5):
  net: macb: Pass same size to DMA_UNMAP as used for DMA_MAP
  net: macb: Clear interrupt flags
  net: macb: Re-enable RX interrupt only when RX is done
  net: macb: Remove 'unlikely' optimization
  net: macb: Fix race between HW and driver

 drivers/net/ethernet/cadence/macb.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

-- 
1.9.2.1.g06c4abd

[PATCH 1/5] net: macb: Pass same size to DMA_UNMAP as used for DMA_MAP

[Soren Brinkmann]

Just as commit "net: macb: DMA-unmap full rx-buffer"
(48330e08fa168395b9fd9f369f06cca1df204361), pass the size that
was used for mapping the memory also to the unmap routine to
avoid warnings from the DMA_API.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---

 drivers/net/ethernet/cadence/macb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index ca97005e24b4..18fdcd9d51b3 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -1113,7 +1113,7 @@ static void gem_free_rx_buffers(struct macb *bp)
 
 		desc = &bp->rx_ring[i];
 		addr = MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr));
-		dma_unmap_single(&bp->pdev->dev, addr, skb->len,
+		dma_unmap_single(&bp->pdev->dev, addr, bp->rx_buffer_size,
 				 DMA_FROM_DEVICE);
 		dev_kfree_skb_any(skb);
 		skb = NULL;
-- 
1.9.2.1.g06c4abd

[PATCH 2/5] net: macb: Clear interrupt flags

[Soren Brinkmann]

A few interrupt flags were not cleared in the ISR, resulting in a sytem
trapped in the ISR in cases one of those interrupts occurred. Clear all
flags to avoid such situations.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---

 drivers/net/ethernet/cadence/macb.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 18fdcd9d51b3..e38fe39d9589 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -951,6 +951,10 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 		if (unlikely(status & (MACB_TX_ERR_FLAGS))) {
 			macb_writel(bp, IDR, MACB_TX_INT_FLAGS);
 			schedule_work(&bp->tx_error_task);
+
+			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+				macb_writel(bp, ISR, MACB_TX_ERR_FLAGS);
+
 			break;
 		}
 
@@ -968,6 +972,9 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 				bp->hw_stats.gem.rx_overruns++;
 			else
 				bp->hw_stats.macb.rx_overruns++;
+
+			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+				macb_writel(bp, ISR, MACB_BIT(ISR_ROVR));
 		}
 
 		if (status & MACB_BIT(HRESP)) {
@@ -977,6 +984,9 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 			 * (work queue?)
 			 */
 			netdev_err(dev, "DMA bus error: HRESP not OK\n");
+
+			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+				macb_writel(bp, ISR, MACB_BIT(HRESP));
 		}
 
 		status = macb_readl(bp, ISR);
-- 
1.9.2.1.g06c4abd

[PATCH 3/5] net: macb: Re-enable RX interrupt only when RX is done

[Soren Brinkmann]

When data is received during the driver processing received data the
NAPI is re-scheduled. In that case the RX interrupt should not be
re-enabled.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---

 drivers/net/ethernet/cadence/macb.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index e38fe39d9589..3f4b8ee0b0e7 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -891,16 +891,15 @@ static int macb_poll(struct napi_struct *napi, int budget)
 	if (work_done < budget) {
 		napi_complete(napi);
 
-		/*
-		 * We've done what we can to clean the buffers. Make sure we
-		 * get notified when new packets arrive.
-		 */
-		macb_writel(bp, IER, MACB_RX_INT_FLAGS);
-
 		/* Packets received while interrupts were disabled */
 		status = macb_readl(bp, RSR);
-		if (unlikely(status))
+		if (unlikely(status)) {
+			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+				macb_writel(bp, ISR, MACB_BIT(RCOMP));
 			napi_reschedule(napi);
+		} else {
+			macb_writel(bp, IER, MACB_RX_INT_FLAGS);
+		}
 	}
 
 	/* TODO: Handle errors */
-- 
1.9.2.1.g06c4abd

[PATCH 4/5] net: macb: Remove 'unlikely' optimization

[Soren Brinkmann]

Coverage data suggests that the unlikely case of receiving data while
the receive handler is running may not be that unlikely.
Coverage data after running iperf for a while:
    91320:  891:	work_done = bp->macbgem_ops.mog_rx(bp, budget);
    91320:  892:	if (work_done < budget) {
     2362:  893:		napi_complete(napi);
        -:  894:
        -:  895:		/* Packets received while interrupts were disabled */
     4724:  896:		status = macb_readl(bp, RSR);
     2362:  897:		if (unlikely(status)) {
      762:  898:			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
      762:  899:				macb_writel(bp, ISR, MACB_BIT(RCOMP));
        -:  900:			napi_reschedule(napi);
        -:  901:		} else {
     1600:  902:			macb_writel(bp, IER, MACB_RX_INT_FLAGS);
        -:  903:		}
        -:  904:	}

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---

 drivers/net/ethernet/cadence/macb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 3f4b8ee0b0e7..3e13aa31548a 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -893,7 +893,7 @@ static int macb_poll(struct napi_struct *napi, int budget)
 
 		/* Packets received while interrupts were disabled */
 		status = macb_readl(bp, RSR);
-		if (unlikely(status)) {
+		if (status) {
 			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
 				macb_writel(bp, ISR, MACB_BIT(RCOMP));
 			napi_reschedule(napi);
-- 
1.9.2.1.g06c4abd

[PATCH 5/5] net: macb: Fix race between HW and driver

[Soren Brinkmann]

Under "heavy" RX load, the driver cannot handle the descriptors fast
enough. In detail, when a descriptor is consumed, its used flag is
cleared and once the RX budget is consumed all descriptors with a
cleared used flag are prepared to receive more data. Under load though,
the HW may constantly receive more data and use those descriptors with a
cleared used flag before they are actually prepared for next usage.

The head and tail pointers into the RX-ring should always be valid and
we can omit clearing and checking of the used flag.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---

---
 drivers/net/ethernet/cadence/macb.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 3e13aa31548a..e9daa072ebb4 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -599,25 +599,16 @@ static void gem_rx_refill(struct macb *bp)
 {
 	unsigned int		entry;
 	struct sk_buff		*skb;
-	struct macb_dma_desc	*desc;
 	dma_addr_t		paddr;
 
 	while (CIRC_SPACE(bp->rx_prepared_head, bp->rx_tail, RX_RING_SIZE) > 0) {
-		u32 addr, ctrl;
-
 		entry = macb_rx_ring_wrap(bp->rx_prepared_head);
-		desc = &bp->rx_ring[entry];
 
 		/* Make hw descriptor updates visible to CPU */
 		rmb();
 
-		addr = desc->addr;
-		ctrl = desc->ctrl;
 		bp->rx_prepared_head++;
 
-		if ((addr & MACB_BIT(RX_USED)))
-			continue;
-
 		if (bp->rx_skbuff[entry] == NULL) {
 			/* allocate sk_buff for this free entry in ring */
 			skb = netdev_alloc_skb(bp->dev, bp->rx_buffer_size);
@@ -698,7 +689,6 @@ static int gem_rx(struct macb *bp, int budget)
 		if (!(addr & MACB_BIT(RX_USED)))
 			break;
 
-		desc->addr &= ~MACB_BIT(RX_USED);
 		bp->rx_tail++;
 		count++;
 
-- 
1.9.2.1.g06c4abd

Re: [PATCH 5/5] net: macb: Fix race between HW and driver

[David Miller]

From: Soren Brinkmann <soren.brinkmann@xilinx.com>
Date: Sun,  4 May 2014 15:43:02 -0700

> Under "heavy" RX load, the driver cannot handle the descriptors fast
> enough. In detail, when a descriptor is consumed, its used flag is
> cleared and once the RX budget is consumed all descriptors with a
> cleared used flag are prepared to receive more data. Under load though,
> the HW may constantly receive more data and use those descriptors with a
> cleared used flag before they are actually prepared for next usage.
> 
> The head and tail pointers into the RX-ring should always be valid and
> we can omit clearing and checking of the used flag.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

Isn't the RX_USED bit the only thing that controls what RX entries
the chip will try to use?

I can't see how you can just remove the RX_USED bit handling
altogether.

The problem actually seems to be that in the current code we clear the
RX_USED bit before we actually reallocate the buffer and set it up.

It should be a bug to see the RX_USED bit set in gem_rx_refill(), and
the only reason why it can happen is exactly because you're clearing it
too early in gem_rx().

This change doesn't seem to be correct, I'm not applying this series
sorry.

Re: [PATCH 5/5] net: macb: Fix race between HW and driver

[Sören Brinkmann]

On Mon, 2014-05-05 at 04:56PM -0400, David Miller wrote:
> From: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Date: Sun,  4 May 2014 15:43:02 -0700
> 
> > Under "heavy" RX load, the driver cannot handle the descriptors fast
> > enough. In detail, when a descriptor is consumed, its used flag is
> > cleared and once the RX budget is consumed all descriptors with a
> > cleared used flag are prepared to receive more data. Under load though,
> > the HW may constantly receive more data and use those descriptors with a
> > cleared used flag before they are actually prepared for next usage.
> > 
> > The head and tail pointers into the RX-ring should always be valid and
> > we can omit clearing and checking of the used flag.
> > 
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> 
> Isn't the RX_USED bit the only thing that controls what RX entries
> the chip will try to use?
> 
> I can't see how you can just remove the RX_USED bit handling
> altogether.
> 
> The problem actually seems to be that in the current code we clear the
> RX_USED bit before we actually reallocate the buffer and set it up.
> 
> It should be a bug to see the RX_USED bit set in gem_rx_refill(), and
> the only reason why it can happen is exactly because you're clearing it
> too early in gem_rx().

I don't follow. The HW uses the descriptor and the driver handles the
received data. So, in gem_rx_refill we should actually only replace
descriptor which have the RX_USED _set_, not? Currently the test tests
for the opposite, since SW clears RX_USED in gem_rx. This patch just
removes those two parts. The RX_USED is left as is (HW should have set
it). And in gem_rx_refill we simply rely on the head and tail pointers
to refill the used descriptors. I didn't see a reason to do the additional
checking of the RX_USED bit.
After the refill the RX_USED flags are of course cleared for all
refilled descriptors.

	Thanks,
	Sören

Re: [PATCH 5/5] net: macb: Fix race between HW and driver

[David Miller]

From: Sören Brinkmann <soren.brinkmann@xilinx.com>
Date: Mon, 5 May 2014 14:05:19 -0700

> On Mon, 2014-05-05 at 04:56PM -0400, David Miller wrote:
>> From: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> Date: Sun,  4 May 2014 15:43:02 -0700
>> 
>> > Under "heavy" RX load, the driver cannot handle the descriptors fast
>> > enough. In detail, when a descriptor is consumed, its used flag is
>> > cleared and once the RX budget is consumed all descriptors with a
>> > cleared used flag are prepared to receive more data. Under load though,
>> > the HW may constantly receive more data and use those descriptors with a
>> > cleared used flag before they are actually prepared for next usage.
>> > 
>> > The head and tail pointers into the RX-ring should always be valid and
>> > we can omit clearing and checking of the used flag.
>> > 
>> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> 
>> Isn't the RX_USED bit the only thing that controls what RX entries
>> the chip will try to use?
>> 
>> I can't see how you can just remove the RX_USED bit handling
>> altogether.
>> 
>> The problem actually seems to be that in the current code we clear the
>> RX_USED bit before we actually reallocate the buffer and set it up.
>> 
>> It should be a bug to see the RX_USED bit set in gem_rx_refill(), and
>> the only reason why it can happen is exactly because you're clearing it
>> too early in gem_rx().
> 
> I don't follow. The HW uses the descriptor and the driver handles the
> received data. So, in gem_rx_refill we should actually only replace
> descriptor which have the RX_USED _set_, not? Currently the test tests
> for the opposite, since SW clears RX_USED in gem_rx. This patch just
> removes those two parts. The RX_USED is left as is (HW should have set
> it). And in gem_rx_refill we simply rely on the head and tail pointers
> to refill the used descriptors. I didn't see a reason to do the additional
> checking of the RX_USED bit.
> After the refill the RX_USED flags are of course cleared for all
> refilled descriptors.

Aha, that makes sense, I didn't notice that RX_USED is cleared as a side
effect of gem_rx_refill()'s work.

I'll apply this series, thanks for explaining.

Re: [PATCH 5/5] net: macb: Fix race between HW and driver

[Sören Brinkmann]

On Mon, 2014-05-05 at 05:09PM -0400, David Miller wrote:
> From: Sören Brinkmann <soren.brinkmann@xilinx.com>
> Date: Mon, 5 May 2014 14:05:19 -0700
> 
> > On Mon, 2014-05-05 at 04:56PM -0400, David Miller wrote:
> >> From: Soren Brinkmann <soren.brinkmann@xilinx.com>
> >> Date: Sun,  4 May 2014 15:43:02 -0700
> >> 
> >> > Under "heavy" RX load, the driver cannot handle the descriptors fast
> >> > enough. In detail, when a descriptor is consumed, its used flag is
> >> > cleared and once the RX budget is consumed all descriptors with a
> >> > cleared used flag are prepared to receive more data. Under load though,
> >> > the HW may constantly receive more data and use those descriptors with a
> >> > cleared used flag before they are actually prepared for next usage.
> >> > 
> >> > The head and tail pointers into the RX-ring should always be valid and
> >> > we can omit clearing and checking of the used flag.
> >> > 
> >> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> >> 
> >> Isn't the RX_USED bit the only thing that controls what RX entries
> >> the chip will try to use?
> >> 
> >> I can't see how you can just remove the RX_USED bit handling
> >> altogether.
> >> 
> >> The problem actually seems to be that in the current code we clear the
> >> RX_USED bit before we actually reallocate the buffer and set it up.
> >> 
> >> It should be a bug to see the RX_USED bit set in gem_rx_refill(), and
> >> the only reason why it can happen is exactly because you're clearing it
> >> too early in gem_rx().
> > 
> > I don't follow. The HW uses the descriptor and the driver handles the
> > received data. So, in gem_rx_refill we should actually only replace
> > descriptor which have the RX_USED _set_, not? Currently the test tests
> > for the opposite, since SW clears RX_USED in gem_rx. This patch just
> > removes those two parts. The RX_USED is left as is (HW should have set
> > it). And in gem_rx_refill we simply rely on the head and tail pointers
> > to refill the used descriptors. I didn't see a reason to do the additional
> > checking of the RX_USED bit.
> > After the refill the RX_USED flags are of course cleared for all
> > refilled descriptors.
> 
> Aha, that makes sense, I didn't notice that RX_USED is cleared as a side
> effect of gem_rx_refill()'s work.
> 
> I'll apply this series, thanks for explaining.

Thanks, but we probably wanna wait for Nicolas to test on his platforms
using macb?

	Sören

Re: [PATCH 5/5] net: macb: Fix race between HW and driver

[David Miller]

From: Sören Brinkmann <soren.brinkmann@xilinx.com>
Date: Mon, 5 May 2014 14:10:23 -0700

> On Mon, 2014-05-05 at 05:09PM -0400, David Miller wrote:
>> From: Sören Brinkmann <soren.brinkmann@xilinx.com>
>> Date: Mon, 5 May 2014 14:05:19 -0700
>> 
>> > On Mon, 2014-05-05 at 04:56PM -0400, David Miller wrote:
>> >> From: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> >> Date: Sun,  4 May 2014 15:43:02 -0700
>> >> 
>> >> > Under "heavy" RX load, the driver cannot handle the descriptors fast
>> >> > enough. In detail, when a descriptor is consumed, its used flag is
>> >> > cleared and once the RX budget is consumed all descriptors with a
>> >> > cleared used flag are prepared to receive more data. Under load though,
>> >> > the HW may constantly receive more data and use those descriptors with a
>> >> > cleared used flag before they are actually prepared for next usage.
>> >> > 
>> >> > The head and tail pointers into the RX-ring should always be valid and
>> >> > we can omit clearing and checking of the used flag.
>> >> > 
>> >> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> >> 
>> >> Isn't the RX_USED bit the only thing that controls what RX entries
>> >> the chip will try to use?
>> >> 
>> >> I can't see how you can just remove the RX_USED bit handling
>> >> altogether.
>> >> 
>> >> The problem actually seems to be that in the current code we clear the
>> >> RX_USED bit before we actually reallocate the buffer and set it up.
>> >> 
>> >> It should be a bug to see the RX_USED bit set in gem_rx_refill(), and
>> >> the only reason why it can happen is exactly because you're clearing it
>> >> too early in gem_rx().
>> > 
>> > I don't follow. The HW uses the descriptor and the driver handles the
>> > received data. So, in gem_rx_refill we should actually only replace
>> > descriptor which have the RX_USED _set_, not? Currently the test tests
>> > for the opposite, since SW clears RX_USED in gem_rx. This patch just
>> > removes those two parts. The RX_USED is left as is (HW should have set
>> > it). And in gem_rx_refill we simply rely on the head and tail pointers
>> > to refill the used descriptors. I didn't see a reason to do the additional
>> > checking of the RX_USED bit.
>> > After the refill the RX_USED flags are of course cleared for all
>> > refilled descriptors.
>> 
>> Aha, that makes sense, I didn't notice that RX_USED is cleared as a side
>> effect of gem_rx_refill()'s work.
>> 
>> I'll apply this series, thanks for explaining.
> 
> Thanks, but we probably wanna wait for Nicolas to test on his platforms
> using macb?

We can always apply a fixup or revert.

Re: [PATCH 0/5] net: macb: Fixes

[Alexandre Belloni]

Hi,

On 04/05/2014 at 15:42:57 -0700, Soren Brinkmann wrote :
> 
> Hi Nicolas,
> 
> I think I found the cause of the issue I told you about. Looks like
> driver and HW are racing (a few more details in the commit message).
> On my way finding that, I found a few more minor issues which are fixed
> in the first patches.
> The last one is "fixing" the actual race. I don't know if I overlooked
> anything here, but this seems to work fine on Zynq.
> 

Tested on at91sam9g45ek with iperf showing good performances. For the
whole series:
Tested-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com