[PATCH 0/3] e1000: fix, two minor updates

[PATCH 0/3] e1000: fix, two minor updates

[Kok, Auke]

Jeff,

Please pull:

git-pull git://lost.foo-projects.org/~ahkok/git/netdev-2.6 e1000

to receive one fix (patch 1) and 2 updates to the e1000 driver.

Cheers,

Auke

---
Summary:
Jesse Brandeburg <jesse.brandeburg@intel.com>: (1)
     e1000: fix shared interrupt warning message

Auke Kok <auke-jan.h.kok@intel.com>: (2)
     e1000: allow ethtool to see link status when down
     e1000: remove obsolete custom pci_save_state code

---
 drivers/net/e1000/e1000.h         |    1 
 drivers/net/e1000/e1000_ethtool.c |    2 -
 drivers/net/e1000/e1000_main.c    |   70 ++++----------------------------------
 3 files changed, 9 insertions(+), 64 deletions(-)

---
These patches appply against torvalds/linux-2.6 #master as of
commit 0187f221e96e3436d552c0c7143f183eb82fb658 ( Ben Dooks --
"[PATCH] s3c2410fb: fix un-initialised dev field")

[PATCH 1/3] e1000: fix shared interrupt warning message

[Kok, Auke]

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/net/e1000/e1000_main.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 619c892..f293690 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -1417,10 +1417,6 @@ e1000_open(struct net_device *netdev)
 	if ((err = e1000_setup_all_rx_resources(adapter)))
 		goto err_setup_rx;
 
-	err = e1000_request_irq(adapter);
-	if (err)
-		goto err_req_irq;
-
 	e1000_power_up_phy(adapter);
 
 	if ((err = e1000_up(adapter)))
@@ -1431,6 +1427,10 @@ e1000_open(struct net_device *netdev)
 		e1000_update_mng_vlan(adapter);
 	}
 
+	err = e1000_request_irq(adapter);
+	if (err)
+		goto err_req_irq;
+
 	/* If AMT is enabled, let the firmware know that the network
 	 * interface is now open */
 	if (adapter->hw.mac_type == e1000_82573 &&
@@ -1439,10 +1439,10 @@ e1000_open(struct net_device *netdev)
 
 	return E1000_SUCCESS;
 
+err_req_irq:
+	e1000_down(adapter);
 err_up:
 	e1000_power_down_phy(adapter);
-	e1000_free_irq(adapter);
-err_req_irq:
 	e1000_free_all_rx_resources(adapter);
 err_setup_rx:
 	e1000_free_all_tx_resources(adapter);



---
Auke Kok <auke-jan.h.kok@intel.com>

Re: [PATCH 1/3] e1000: fix shared interrupt warning message

[Jeff Garzik]

Kok, Auke wrote:
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/net/e1000/e1000_main.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)

applied 1-3, though patch #1 had a noticable lack of description.  you 
should describe your change, not just the condition that prompted it. 
operation reordering is something that people shouldn't have to /try/ to 
understand.  Things should just be obvious, and if not, document why.

Re: [PATCH 1/3] e1000: fix shared interrupt warning message

[Auke Kok]

Jeff Garzik wrote:
> Kok, Auke wrote:
>> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> ---
>>
>>  drivers/net/e1000/e1000_main.c |   12 ++++++------
>>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> applied 1-3, though patch #1 had a noticable lack of description.  you 
> should describe your change, not just the condition that prompted it. 
> operation reordering is something that people shouldn't have to /try/ to 
> understand.  Things should just be obvious, and if not, document why.

For the record then:

It is suspected that workarounds in the _up() routine of e1000 can cause the 
receive unit to be enabled before we're all done initializing the adapter data. 
An interrupt arriving before we're all done setting up obviously causes 
problems. To fix, we postpone enabling interrupts completely until the software 
intialization is finished.

Thanks,

Auke

Re: [PATCH 1/3] e1000: fix shared interrupt warning message

[Francois Romieu]

Auke Kok <auke-jan.h.kok@intel.com> :
[...]
> It is suspected that workarounds in the _up() routine of e1000 can cause 
        ^^^^^^^^^
> the receive unit to be enabled before we're all done initializing the 
> adapter data. An interrupt arriving before we're all done setting up 
> obviously causes problems. To fix, we postpone enabling interrupts 
> completely until the software intialization is finished.

Call me dense but is there a chance that the same receive unit that should
not be enabled now emits interrupts between e1000_up() and request_irq() ?

-- 
Ueimor

Re: [PATCH 1/3] e1000: fix shared interrupt warning message

[Auke Kok]

Francois Romieu wrote:
> Auke Kok <auke-jan.h.kok@intel.com> :
> [...]
>> It is suspected that workarounds in the _up() routine of e1000 can cause 
>         ^^^^^^^^^
>> the receive unit to be enabled before we're all done initializing the 
>> adapter data. An interrupt arriving before we're all done setting up 
>> obviously causes problems. To fix, we postpone enabling interrupts 
>> completely until the software intialization is finished.
> 
> Call me dense but is there a chance that the same receive unit that should
> not be enabled now emits interrupts between e1000_up() and request_irq() ?

turning on the receive unit starts the rx logic on the mac, but doesn't make the 
mac generate interrupts as far as I know. The fix was verified and even 
suggested earlier by some people as well.

Cheers,

Auke

[PATCH 2/3] e1000: remove obsolete custom pci_save_state code

[Kok, Auke]

Now that 2.6.19 provides a proper implementation that saves MSI, PCI-E
config space, we can have the e1000 driver use those instead of it's
custom implementation.

Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
---

 drivers/net/e1000/e1000.h      |    1 -
 drivers/net/e1000/e1000_main.c |   58 +---------------------------------------
 2 files changed, 2 insertions(+), 57 deletions(-)

diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
index 689f158..dd4b728 100644
--- a/drivers/net/e1000/e1000.h
+++ b/drivers/net/e1000/e1000.h
@@ -337,7 +337,6 @@ struct e1000_adapter {
 	struct e1000_rx_ring test_rx_ring;
 
 
-	uint32_t *config_space;
 	int msg_enable;
 #ifdef CONFIG_PCI_MSI
 	boolean_t have_msi;
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index f293690..a710237 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -5071,58 +5071,6 @@ e1000_set_spd_dplx(struct e1000_adapter *adapter, uint16_t spddplx)
 	return 0;
 }
 
-#ifdef CONFIG_PM
-/* Save/restore 16 or 64 dwords of PCI config space depending on which
- * bus we're on (PCI(X) vs. PCI-E)
- */
-#define PCIE_CONFIG_SPACE_LEN 256
-#define PCI_CONFIG_SPACE_LEN 64
-static int
-e1000_pci_save_state(struct e1000_adapter *adapter)
-{
-	struct pci_dev *dev = adapter->pdev;
-	int size;
-	int i;
-
-	if (adapter->hw.mac_type >= e1000_82571)
-		size = PCIE_CONFIG_SPACE_LEN;
-	else
-		size = PCI_CONFIG_SPACE_LEN;
-
-	WARN_ON(adapter->config_space != NULL);
-
-	adapter->config_space = kmalloc(size, GFP_KERNEL);
-	if (!adapter->config_space) {
-		DPRINTK(PROBE, ERR, "unable to allocate %d bytes\n", size);
-		return -ENOMEM;
-	}
-	for (i = 0; i < (size / 4); i++)
-		pci_read_config_dword(dev, i * 4, &adapter->config_space[i]);
-	return 0;
-}
-
-static void
-e1000_pci_restore_state(struct e1000_adapter *adapter)
-{
-	struct pci_dev *dev = adapter->pdev;
-	int size;
-	int i;
-
-	if (adapter->config_space == NULL)
-		return;
-
-	if (adapter->hw.mac_type >= e1000_82571)
-		size = PCIE_CONFIG_SPACE_LEN;
-	else
-		size = PCI_CONFIG_SPACE_LEN;
-	for (i = 0; i < (size / 4); i++)
-		pci_write_config_dword(dev, i * 4, adapter->config_space[i]);
-	kfree(adapter->config_space);
-	adapter->config_space = NULL;
-	return;
-}
-#endif /* CONFIG_PM */
-
 static int
 e1000_suspend(struct pci_dev *pdev, pm_message_t state)
 {
@@ -5142,9 +5090,7 @@ e1000_suspend(struct pci_dev *pdev, pm_message_t state)
 	}
 
 #ifdef CONFIG_PM
-	/* Implement our own version of pci_save_state(pdev) because pci-
-	 * express adapters have 256-byte config spaces. */
-	retval = e1000_pci_save_state(adapter);
+	retval = pci_save_state(pdev);
 	if (retval)
 		return retval;
 #endif
@@ -5231,7 +5177,7 @@ e1000_resume(struct pci_dev *pdev)
 	uint32_t err;
 
 	pci_set_power_state(pdev, PCI_D0);
-	e1000_pci_restore_state(adapter);
+	pci_restore_state(pdev);
 	if ((err = pci_enable_device(pdev))) {
 		printk(KERN_ERR "e1000: Cannot enable PCI device from suspend\n");
 		return err;



---
Auke Kok <auke-jan.h.kok@intel.com>

[PATCH 3/3] e1000: allow ethtool to see link status when down

[Kok, Auke]

By reading the MAC status register we can detect whether the MAC has
seen the PHY see link. This allows us to show the link properties before
the device is up in ethtool.

Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>
---

 drivers/net/e1000/e1000_ethtool.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/e1000/e1000_ethtool.c b/drivers/net/e1000/e1000_ethtool.c
index 44ebc72..6777887 100644
--- a/drivers/net/e1000/e1000_ethtool.c
+++ b/drivers/net/e1000/e1000_ethtool.c
@@ -166,7 +166,7 @@ e1000_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
 			ecmd->transceiver = XCVR_EXTERNAL;
 	}
 
-	if (netif_carrier_ok(adapter->netdev)) {
+	if (E1000_READ_REG(&adapter->hw, STATUS) & E1000_STATUS_LU) {
 
 		e1000_get_speed_and_duplex(hw, &adapter->link_speed,
 		                                   &adapter->link_duplex);



---
Auke Kok <auke-jan.h.kok@intel.com>