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