[04/10] net/macb: Fix a race in macb_start_xmit()

Commit Message

Nicolas Ferre Sept. 5, 2012, 8:19 a.m. UTC

From: Havard Skinnemoen <[e-mail]>

Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
If an underrun just happened (we do this with interrupts disabled, so it might
not have been handled yet), the controller starts transmitting from the first
entry in the ring, which is usually wrong.
Restart the controller after error handling.

Signed-off-by: Havard Skinnemoen <[e-mail]>
[[e-mail]: split patch in topics]
Signed-off-by: Nicolas Ferre <[e-mail]>
---
 drivers/net/ethernet/cadence/macb.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

David Miller Sept. 5, 2012, 9:30 p.m. UTC | #1

From: Nicolas Ferre <[e-mail]>
Date: Wed, 5 Sep 2012 10:19:11 +0200

> From: Havard Skinnemoen <[e-mail]>
> 
> Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
> If an underrun just happened (we do this with interrupts disabled, so it might
> not have been handled yet), the controller starts transmitting from the first
> entry in the ring, which is usually wrong.
> Restart the controller after error handling.
> 
> Signed-off-by: Havard Skinnemoen <[e-mail]>
> [[e-mail]: split patch in topics]
> Signed-off-by: Nicolas Ferre <[e-mail]>

Accumulating special case code and checks into the hot path of TX packet
processing is extremely unwise.

Instead, when you handle the TX error conditions and reset the chip you
should first ensure that there are no flows of control in the transmit
function of your driver by using the appropriate locking et al. facilities.

For example, you can quiesce the transmit path by handling the chip error
interrupt as follows:

1) Disable chip interrupt generation.

2) Schedule a workqueue so you can process the reset outside of hard
   interrupt context.

3) In the workqueue function, disable NAPI and perform a
   netif_tx_disable() to guarentee there are no threads of
   execution trying to queue up packets for TX into the driver.

4) Perform your chip reset and whatever else is necessary.

5) Re-enable NAPI and TX.

Then you don't need any special checks in your xmit method at all.

Nicolas Ferre Sept. 6, 2012, 2:52 p.m. UTC | #2

On 09/05/2012 11:30 PM, David Miller :
> From: Nicolas Ferre <[e-mail]>
> Date: Wed, 5 Sep 2012 10:19:11 +0200
> 
>> From: Havard Skinnemoen <[e-mail]>
>>
>> Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
>> If an underrun just happened (we do this with interrupts disabled, so it might
>> not have been handled yet), the controller starts transmitting from the first
>> entry in the ring, which is usually wrong.
>> Restart the controller after error handling.
>>
>> Signed-off-by: Havard Skinnemoen <[e-mail]>
>> [[e-mail]: split patch in topics]
>> Signed-off-by: Nicolas Ferre <[e-mail]>
> 
> Accumulating special case code and checks into the hot path of TX packet
> processing is extremely unwise.
> 
> Instead, when you handle the TX error conditions and reset the chip you
> should first ensure that there are no flows of control in the transmit
> function of your driver by using the appropriate locking et al. facilities.
> 
> For example, you can quiesce the transmit path by handling the chip error
> interrupt as follows:
> 
> 1) Disable chip interrupt generation.
> 
> 2) Schedule a workqueue so you can process the reset outside of hard
>    interrupt context.
> 
> 3) In the workqueue function, disable NAPI and perform a
>    netif_tx_disable() to guarentee there are no threads of
>    execution trying to queue up packets for TX into the driver.
> 
> 4) Perform your chip reset and whatever else is necessary.
> 
> 5) Re-enable NAPI and TX.
> 
> Then you don't need any special checks in your xmit method at all.

I see... I will rework the series and try to implement this as part of
the "[PATCH 06/10] net/macb: better manage tx errors"

So this patch will disappear in future v2 series and patch 06 will be
seriously modified. In fact I will also try to stack "cosmetic" patches
at the beginning of the series.

Thanks, best regards,

Havard Skinnemoen Sept. 6, 2012, 3:49 p.m. UTC | #3

On Wed, Sep 5, 2012 at 11:30 PM, David Miller <[e-mail]> wrote:
> From: Nicolas Ferre <[e-mail]>
> Date: Wed, 5 Sep 2012 10:19:11 +0200
>
>> From: Havard Skinnemoen <[e-mail]>
>>
>> Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
>> If an underrun just happened (we do this with interrupts disabled, so it might
>> not have been handled yet), the controller starts transmitting from the first
>> entry in the ring, which is usually wrong.
>> Restart the controller after error handling.
>>
>> Signed-off-by: Havard Skinnemoen <[e-mail]>
>> [[e-mail]: split patch in topics]
>> Signed-off-by: Nicolas Ferre <[e-mail]>
>
> Accumulating special case code and checks into the hot path of TX packet
> processing is extremely unwise.
>
> Instead, when you handle the TX error conditions and reset the chip you
> should first ensure that there are no flows of control in the transmit
> function of your driver by using the appropriate locking et al. facilities.

IIRC, the hardware resets the ring pointers when an error happens, and
if we set TSTART right after that happens, the hardware will happily
transmit whatever is sitting in the beginning of the ring. This is
what I was trying to avoid.

The details are a bit hazy as it's been a while since I looked at
this, so it could be that simply letting it happen and using a bigger
hammer during reset processing might work just as well. Just want to
make sure y'all understand that we're talking about a race against
hardware, not against interrupt handlers, threads or anything that can
be solved by locking :)

Havard

Nicolas Ferre Sept. 7, 2012, 4:34 p.m. UTC | #4

On 09/06/2012 05:49 PM, Havard Skinnemoen :
> On Wed, Sep 5, 2012 at 11:30 PM, David Miller <[e-mail]> wrote:
>> From: Nicolas Ferre <[e-mail]>
>> Date: Wed, 5 Sep 2012 10:19:11 +0200
>>
>>> From: Havard Skinnemoen <[e-mail]>
>>>
>>> Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
>>> If an underrun just happened (we do this with interrupts disabled, so it might
>>> not have been handled yet), the controller starts transmitting from the first
>>> entry in the ring, which is usually wrong.
>>> Restart the controller after error handling.
>>>
>>> Signed-off-by: Havard Skinnemoen <[e-mail]>
>>> [[e-mail]: split patch in topics]
>>> Signed-off-by: Nicolas Ferre <[e-mail]>
>>
>> Accumulating special case code and checks into the hot path of TX packet
>> processing is extremely unwise.
>>
>> Instead, when you handle the TX error conditions and reset the chip you
>> should first ensure that there are no flows of control in the transmit
>> function of your driver by using the appropriate locking et al. facilities.
> 
> IIRC, the hardware resets the ring pointers when an error happens, and
> if we set TSTART right after that happens, the hardware will happily
> transmit whatever is sitting in the beginning of the ring. This is
> what I was trying to avoid.
> 
> The details are a bit hazy as it's been a while since I looked at
> this, so it could be that simply letting it happen and using a bigger
> hammer during reset processing might work just as well. Just want to
> make sure y'all understand that we're talking about a race against
> hardware, not against interrupt handlers, threads or anything that can
> be solved by locking :)

Yes, you are right Havard.

I will see if we can let the transmitter go just before being
interrupted by the pending error.

It is true that there are several cases here:
- tx immediately stopped again by the USED bit of a non initialized
descriptor. We thus have to cleanup the error frame but also take care
about the newly queued packet...
- beginning of transmission of a not-related fragment that has just been
queued by the start_xmit; just before catching the pending error IRQ. We
may have to consider the consequences of this!

So, stay tuned ;-)

Best regards,

Patch

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 2228dfc..f4b8adf 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -390,6 +390,13 @@  static void macb_tx(struct macb *bp)
 		dev_kfree_skb_irq(skb);
 	}
 
+	/*
+	 * Someone may have submitted a new frame while this interrupt
+	 * was pending, or we may just have handled an error.
+	 */
+	if (head != tail && !(status & MACB_BIT(TGO)))
+		macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
+
 	bp->tx_tail = tail;
 	if (netif_queue_stopped(bp->dev) &&
 	    TX_BUFFS_AVAIL(bp) > MACB_TX_WAKEUP_THRESH)
@@ -696,7 +703,18 @@  static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	skb_tx_timestamp(skb);
 
-	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
+	/*
+	 * Only start the controller if the queue was empty; otherwise
+	 * we may race against the hardware resetting the ring pointer
+	 * due to a transmit error.
+	 *
+	 * If the controller is idle but the queue isn't empty, there
+	 * must be a pending interrupt that will trigger as soon as we
+	 * re-enable interrupts, and the interrupt handler will make
+	 * sure the controler is started.
+	 */
+	if (NEXT_TX(bp->tx_tail) == bp->tx_head)
+		macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
 
 	if (TX_BUFFS_AVAIL(bp) < 1)
 		netif_stop_queue(dev);