On 4/21/06, Andy Gospodarek <[EMAIL PROTECTED]> wrote:
> On 4/21/06, Michael Chan <[EMAIL PROTECTED]> 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.
Thanks,
Shaw
diff -u -uprN -X linux-2.6.16.11/Documentation/dontdiff linux-2.6.16.11/drivers/net/e1000/e1000.h linux-2.6.16.11.e1000_patch/drivers/net/e1000/e1000.h
--- linux-2.6.16.11/drivers/net/e1000/e1000.h 2006-04-24 13:20:24.000000000 -0700
+++ linux-2.6.16.11.e1000_patch/drivers/net/e1000/e1000.h 2006-04-26 16:23:46.475842000 -0700
@@ -358,5 +358,8 @@ struct e1000_adapter {
#ifdef CONFIG_PCI_MSI
boolean_t have_msi;
#endif
+ uint32_t flags;
+#define E1000_CLEANING 0x00000001
+ spinlock_t clean_lock;
};
#endif /* _E1000_H_ */
diff -u -uprN -X linux-2.6.16.11/Documentation/dontdiff linux-2.6.16.11/drivers/net/e1000/e1000_main.c linux-2.6.16.11.e1000_patch/drivers/net/e1000/e1000_main.c
--- linux-2.6.16.11/drivers/net/e1000/e1000_main.c 2006-04-24 13:20:24.000000000 -0700
+++ linux-2.6.16.11.e1000_patch/drivers/net/e1000/e1000_main.c 2006-04-26 16:59:48.742905000 -0700
@@ -525,6 +525,16 @@ e1000_down(struct e1000_adapter *adapter
boolean_t mng_mode_enabled = (adapter->hw.mac_type >= e1000_82571) &&
e1000_check_mng_mode(&adapter->hw);
+ spin_lock_bh(&adapter->clean_lock);
+ adapter->flags |= E1000_CLEANING;
+
+ if (!netif_running(netdev)) {
+ adapter->flags &= ~E1000_CLEANING;
+ spin_unlock_bh(&adapter->clean_lock);
+ return;
+ }
+ spin_unlock_bh(&adapter->clean_lock);
+
e1000_irq_disable(adapter);
#ifdef CONFIG_E1000_MQ
while (atomic_read(&adapter->rx_sched_call_data.count) != 0);
@@ -549,8 +559,12 @@ e1000_down(struct e1000_adapter *adapter
netif_stop_queue(netdev);
e1000_reset(adapter);
+
+ spin_lock_bh(&adapter->clean_lock);
e1000_clean_all_tx_rings(adapter);
e1000_clean_all_rx_rings(adapter);
+ adapter->flags &= ~E1000_CLEANING;
+ spin_unlock_bh(&adapter->clean_lock);
/* Power down the PHY so no link is implied when interface is down *
* The PHY cannot be powered down if any of the following is TRUE *
@@ -1109,6 +1123,8 @@ e1000_sw_init(struct e1000_adapter *adap
atomic_set(&adapter->irq_sem, 1);
spin_lock_init(&adapter->stats_lock);
+ spin_lock_init(&adapter->clean_lock);
+ adapter->flags = 0;
return 0;
}
@@ -1269,10 +1285,18 @@ e1000_close(struct net_device *netdev)
{
struct e1000_adapter *adapter = netdev_priv(netdev);
+ /* Calling flush_scheduled_work() may deadlock because
+ * linkwatch_event() may be on the workqueue and it will
+ * try to get the rtnl_lock which we are holding. */
+ while (adapter->flags & E1000_CLEANING)
+ msleep(1);
+
e1000_down(adapter);
+ spin_lock_bh(&adapter->clean_lock);
e1000_free_all_tx_resources(adapter);
e1000_free_all_rx_resources(adapter);
+ spin_unlock_bh(&adapter->clean_lock);
if ((adapter->hw.mng_cookie.status &
E1000_MNG_DHCP_COOKIE_STATUS_VLAN_SUPPORT)) {
