Note to all: please use this address when copying me regarding
upstream matters.  It keeps my mailboxes better sorted...thanks!

Since I'm being called-out by name, I suppose I should add my
$0.02... :-)

On Thu, Aug 25, 2005 at 04:38:35PM -0400, Jeff Garzik wrote:
> Malli Chilakala wrote:
> >Backout watchdog task patch from John Linville
 
> Rejected for two reasons:
> 
> 1) No explanation why this back-out is needed.
 
The patch that created the e1000_watchdog_task inadvertantly created
a synchronization problem.  e1000_down calls del_timer_sync on the
watchdog_timer.  Code later in that path executes under the presumption
that the code which had been part of the watchdog_timer handler (but
is now in e1000_watchdog_task) is not running.  Since that code is now
part of e1000_watchdog_task apparently there is a window in which the
del_timer_sync can return before e1000_watchdog_task actually runs,
and in some cases (SMP?) that can lead to locking/corruption problems.

> 2) You should fix problems rather than running from them.  We want as 
> much slow path code as possible -- such as reset or phy code -- to be in 
> process context.

Although I did consent to backing-out the patch, I am inclined to
agree with Jeff for the long term.

It seems like we tried adding a flush_workqueue after the
del_timer_sync, but that was somehow insufficient?  Ganesh, do you
remember?

Ganesh, John, Malli, etc., I'll be happy to work with you guys on this
one if you'd like to figure-out how to make this work and please Jeff.
We need to do something, as there is a real problem there in the
current upstream code.

John

P.S.  Why would this patch not work?

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -352,6 +352,7 @@ e1000_down(struct e1000_adapter *adapter
 #endif
        del_timer_sync(&adapter->tx_fifo_stall_timer);
        del_timer_sync(&adapter->watchdog_timer);
+       flush_workqueue(&adapter->watchdog_task);
        del_timer_sync(&adapter->phy_info_timer);
 
 #ifdef CONFIG_E1000_NAPI
-- 
John W. Linville
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to