Re: e1000_down and tx_timeout worker race cleaning the transmit buffers

[Auke Kok <sofar@foo-projects.org>]

Shaw wrote:
> On 4/21/06, Andy Gospodarek <andy@greyhouse.net> wrote:
>> On 4/21/06, Michael Chan <mchan@broadcom.com> wrote:
>>> On Fri, 2006-04-21 at 16:01 -0400, Andy Gospodarek wrote:
>>>
>>>> I just hate to see extra resources used to solve problems that good
>>>> coding can solve (not that my suggestion is necessarily a 'good' one),
>>>> so I was trying to think of a way to resolve this without explicitly
>>>> adding another workqueue.
>>> If you don't want to add another workqueue, then look at tg3, bnx2, and
>>> one of the smc drivers on how to effectively wait for the driver's
>>> workqueue task to finish without deadlocking with linkwatch_event.
>>>
>> I agree 100%.  I just hope others can manage to figure that out too.
> 
> Ok, here's another attempt.  The goal here is to serialize attempts to
> clean the tx and rx buffers, and ensure that e1000_close is called
> after the tx_timeout_task has completed running and/or that the task
> is safe to run after e1000_close hasrun.
> 
> I'm concerned about the addition of the netif_running check to
> e1000_down.  While something like this is needed, I'm not familiar
> enough w/ the code to know if this is okay.
> All explanations and comments are greatly appreciated.

I apologise for not getting back on this earlier but Jesse Brandeburg and I 
have been digging into this for two days and making some big progress. One of 
the main fixes will be that we're taking out a watchdog reset task completely 
and doing down/up cycles instead, which removes a large portion of the race 
conditions at this stage completely (the tx_timeout triggers a watchdog reset 
which can happen during an e1000_down causing a double free interrupt, or a 
double allocation).

We're making good progress with this and are now working on removing the last 
race between the ioctl path and the ifdn/ifup stuff, where the last remaining 
race location is in the ethtool test which does all sorts of funny lowlevel 
driver stuff that can seriously OOPS if you're running ethtool tests while 
ifup/downing your interface.

While I appreciate patches ;^) I think we're on a better path by making these 
cleanups, and actually reducing the code in large places. I hope to be able to 
push something out for RFC soon. Added benefit will be that we're dropping a 
whole bunch of irq operations where we didn't need to (soft resets).

Cheers,

Auke