Re: [PATCH 02/21] e1000: rework driver hardware reset locking

2006-06-27 Thread Auke Kok

Jeff Garzik wrote:

Kok, Auke wrote:

@@ -631,6 +627,9 @@ e1000_set_ringparam(struct net_device *n
 tx_ring_size = sizeof(struct e1000_tx_ring) * 
adapter-num_tx_queues;
 rx_ring_size = sizeof(struct e1000_rx_ring) * 
adapter-num_rx_queues;
 
+while (test_and_set_bit(__E1000_RESETTING, adapter-flags))

+msleep(1);


This is a bit worrying, but no outright objection.  We don't want to see 
these accumulate.


Agreed but at least we removed the opportunity to panic the driver as easy as 
with 2 commandline commands (in several ways) - we're still looking into 
improving this - any comments would be appreciated.


Cheers,

Auke
-
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


Re: [PATCH 02/21] e1000: rework driver hardware reset locking

2006-06-26 Thread Jeff Garzik

Kok, Auke wrote:

@@ -631,6 +627,9 @@ e1000_set_ringparam(struct net_device *n
tx_ring_size = sizeof(struct e1000_tx_ring) * adapter-num_tx_queues;
rx_ring_size = sizeof(struct e1000_rx_ring) * adapter-num_rx_queues;
 
+	while (test_and_set_bit(__E1000_RESETTING, adapter-flags))

+   msleep(1);


This is a bit worrying, but no outright objection.  We don't want to see 
these accumulate.


-
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


[PATCH 02/21] e1000: rework driver hardware reset locking

2006-06-21 Thread Kok, Auke

After studying the driver mac reset code it was found that there
were multiple race conditions possible to reset the unit twice or
bring it e1000_up() double. This fixes all occurences where the
driver needs to reset the mac.

We also remove irq requesting/releasing into _open and _close so
that while the device is _up we will never touch the irq's. This fixes
the double free irq bug that people saw.

To make sure that the watchdog task doesn't cause another race we let
it run as a non-scheduled task.

Signed-off-by: Jesse Brandeburg [EMAIL PROTECTED]
Signed-off-by: Auke Kok [EMAIL PROTECTED]
---

 drivers/net/e1000/e1000.h |8 ++
 drivers/net/e1000/e1000_ethtool.c |   46 --
 drivers/net/e1000/e1000_main.c|  126 +
 3 files changed, 105 insertions(+), 75 deletions(-)

diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
index 2bc34fb..2b96ad0 100644
--- a/drivers/net/e1000/e1000.h
+++ b/drivers/net/e1000/e1000.h
@@ -69,7 +69,6 @@
 #ifdef NETIF_F_TSO
 #include net/checksum.h
 #endif
-#include linux/workqueue.h
 #include linux/mii.h
 #include linux/ethtool.h
 #include linux/if_vlan.h
@@ -255,7 +254,6 @@ struct e1000_adapter {
spinlock_t tx_queue_lock;
 #endif
atomic_t irq_sem;
-   struct work_struct watchdog_task;
struct work_struct reset_task;
uint8_t fc_autoneg;
 
@@ -340,8 +338,13 @@ struct e1000_adapter {
 #ifdef NETIF_F_TSO
boolean_t tso_force;
 #endif
+   unsigned long flags;
 };
 
+enum e1000_state_t {
+   __E1000_DRIVER_TESTING,
+   __E1000_RESETTING,
+};
 
 /*  e1000_main.c  */
 extern char e1000_driver_name[];
@@ -349,6 +352,7 @@ extern char e1000_driver_version[];
 int e1000_up(struct e1000_adapter *adapter);
 void e1000_down(struct e1000_adapter *adapter);
 void e1000_reset(struct e1000_adapter *adapter);
+void e1000_reinit_locked(struct e1000_adapter *adapter);
 int e1000_setup_all_tx_resources(struct e1000_adapter *adapter);
 void e1000_free_all_tx_resources(struct e1000_adapter *adapter);
 int e1000_setup_all_rx_resources(struct e1000_adapter *adapter);
diff --git a/drivers/net/e1000/e1000_ethtool.c 
b/drivers/net/e1000/e1000_ethtool.c
index 845d293..cf5c5f4 100644
--- a/drivers/net/e1000/e1000_ethtool.c
+++ b/drivers/net/e1000/e1000_ethtool.c
@@ -203,11 +203,9 @@ e1000_set_settings(struct net_device *ne
 
/* reset the link */
 
-   if (netif_running(adapter-netdev)) {
-   e1000_down(adapter);
-   e1000_reset(adapter);
-   e1000_up(adapter);
-   } else
+   if (netif_running(adapter-netdev))
+   e1000_reinit_locked(adapter);
+   else
e1000_reset(adapter);
 
return 0;
@@ -254,10 +252,9 @@ e1000_set_pauseparam(struct net_device *
hw-original_fc = hw-fc;
 
if (adapter-fc_autoneg == AUTONEG_ENABLE) {
-   if (netif_running(adapter-netdev)) {
-   e1000_down(adapter);
-   e1000_up(adapter);
-   } else
+   if (netif_running(adapter-netdev))
+   e1000_reinit_locked(adapter);
+   else
e1000_reset(adapter);
} else
return ((hw-media_type == e1000_media_type_fiber) ?
@@ -279,10 +276,9 @@ e1000_set_rx_csum(struct net_device *net
struct e1000_adapter *adapter = netdev_priv(netdev);
adapter-rx_csum = data;
 
-   if (netif_running(netdev)) {
-   e1000_down(adapter);
-   e1000_up(adapter);
-   } else
+   if (netif_running(netdev))
+   e1000_reinit_locked(adapter);
+   else
e1000_reset(adapter);
return 0;
 }
@@ -631,6 +627,9 @@ e1000_set_ringparam(struct net_device *n
tx_ring_size = sizeof(struct e1000_tx_ring) * adapter-num_tx_queues;
rx_ring_size = sizeof(struct e1000_rx_ring) * adapter-num_rx_queues;
 
+   while (test_and_set_bit(__E1000_RESETTING, adapter-flags))
+   msleep(1);
+
if (netif_running(adapter-netdev))
e1000_down(adapter);
 
@@ -691,9 +690,11 @@ e1000_set_ringparam(struct net_device *n
adapter-rx_ring = rx_new;
adapter-tx_ring = tx_new;
if ((err = e1000_up(adapter)))
-   return err;
+   goto err_setup;
}
 
+   clear_bit(__E1000_RESETTING, adapter-flags);
+
return 0;
 err_setup_tx:
e1000_free_all_rx_resources(adapter);
@@ -701,6 +702,8 @@ err_setup_rx:
adapter-rx_ring = rx_old;
adapter-tx_ring = tx_old;
e1000_up(adapter);
+err_setup:
+   clear_bit(__E1000_RESETTING, adapter-flags);
return err;
 }
 
@@ -1568,6 +1571,7 @@ e1000_diag_test(struct net_device *netde
struct e1000_adapter *adapter = netdev_priv(netdev);
boolean_t if_running = netif_running(netdev);
 
+