Messages in this thread | | | Date | Thu, 6 Sep 2012 17:49:11 +0200 | Subject | Re: [PATCH 04/10] net/macb: Fix a race in macb_start_xmit() | From | Havard Skinnemoen <> |
| |
On Wed, Sep 5, 2012 at 11:30 PM, David Miller <davem@davemloft.net> wrote: > From: Nicolas Ferre <nicolas.ferre@atmel.com> > Date: Wed, 5 Sep 2012 10:19:11 +0200 > >> From: Havard Skinnemoen <havard@skinnemoen.net> >> >> 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 <havard@skinnemoen.net> >> [nicolas.ferre@atmel.com: split patch in topics] >> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com> > > 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
| |