Re: [PATCH] net: forcedeth use pci_choose_state instead of PCI_D3hot - v2

Previous thread: [PATCH] net: forcedeth use pci_choose_state instead of PCI_D3hot by Yinghai Lu on Saturday, August 16, 2008 - 11:22 pm. (1 message)

Next thread: [PATCH] x86: remove first_free_entry/pin_map_size by Yinghai Lu on Saturday, August 16, 2008 - 11:31 pm. (2 messages)
From: Yinghai Lu
Date: Saturday, August 16, 2008 - 11:25 pm

after

| commit f735a2a1a4f2a0f5cd823ce323e82675990469e2
| Author: Tobias Diedrich <ranma+kernel@tdiedrich.de>
| Date:   Sun May 18 15:02:37 2008 +0200
|
|    [netdrvr] forcedeth: setup wake-on-lan before shutting down
|
|    When hibernating in 'shutdown' mode, after saving the image the suspend hook
|    is not called again.
|    However, if the device is in promiscous mode, wake-on-lan will not work.
|    This adds a shutdown hook to setup wake-on-lan before the final shutdown.
|
|    Signed-off-by: Tobias Diedrich <ranma+kernel@tdiedrich.de>
|    Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

my servers with nvidia mcp55 nic doesn't work with msi in second kernel by kexec

after remove pci_set_power_state(, PCI_D3hot) that nic/msi will work again.

check with e1000 is using pci_choose_state in _shutdown.

So change that pci_choose_state(pdev, ...), and it works.

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>

---
 drivers/net/forcedeth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/drivers/net/forcedeth.c
===================================================================
--- linux-2.6.orig/drivers/net/forcedeth.c
+++ linux-2.6/drivers/net/forcedeth.c
@@ -5988,7 +5988,7 @@ static void nv_shutdown(struct pci_dev *
 	pci_enable_wake(pdev, PCI_D3hot, np->wolenabled);
 	pci_enable_wake(pdev, PCI_D3cold, np->wolenabled);
 	pci_disable_device(pdev);
-	pci_set_power_state(pdev, PCI_D3hot);
+	pci_set_power_state(pdev, pci_choose_state(pdev, PMSG_SUSPEND));
 }
 #else
 #define nv_suspend NULL
--

From: Rafael J. Wysocki
Date: Sunday, August 17, 2008 - 6:02 am

Well, this doesn't look like a good solution to me, because you're putting
PMSG_SUSPEND in there, which is not correct for shutdown.  The right thing to
do would be to avoid changing the device power state if nv_shutdown() is
used for kexec or to rework the initialization of the adapter to handle the
case when it's initially in D3.

Does it help if you just remove the pci_set_power_state(pdev, PCI_D3hot)

Thanks,
Rafael
--

From: Rafael J. Wysocki
Date: Sunday, August 17, 2008 - 9:55 am

Ah, sorry.  I see it does.

Actually, I think you can use pci_prepare_to_sleep() instead of
pci_enable_wake() / pci_set_power_state() combo.  It wasn't designed for this
purpose, but should work nevertheless.

Can you please check if the appended patch works instead of your one?

Rafael

---
Fix the problem that boxes with NVidia MCP55 don't work with MSI
in a kexeced kernel.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/net/forcedeth.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Index: linux-2.6/drivers/net/forcedeth.c
===================================================================
--- linux-2.6.orig/drivers/net/forcedeth.c
+++ linux-2.6/drivers/net/forcedeth.c
@@ -5975,10 +5975,8 @@ static void nv_shutdown(struct pci_dev *
 	if (netif_running(dev))
 		nv_close(dev);
 
-	pci_enable_wake(pdev, PCI_D3hot, np->wolenabled);
-	pci_enable_wake(pdev, PCI_D3cold, np->wolenabled);
 	pci_disable_device(pdev);
-	pci_set_power_state(pdev, PCI_D3hot);
+	pci_prepare_to_sleep(pdev);
 }
 #else
 #define nv_suspend NULL

--

From: Yinghai Lu
Date: Sunday, August 17, 2008 - 12:16 pm

your patch doesn't work...  it seems that silicon has problem with D3Hot

calling  init_nic+0x0/0x1b
forcedeth: Reverse Engineered nForce ethernet driver. Version 0.61.
..
forcedeth 0000:00:09.0: BAR 0: got res [0xdefb9000-0xdefb9fff] bus
[0xdefb9000-0xdefb9fff] flags 0x20200
forcedeth 0000:00:09.0: BAR 0: error updating (0xdefb9000 != 0x000000)
forcedeth 0000:00:09.0: BAR 0: moved to bus [0xdefb9000-0xdefb9fff]
flags 0x20200
forcedeth 0000:00:09.0: BAR 1: got res [0x8080-0x8087] bus
[0x8080-0x8087] flags 0x20101
forcedeth 0000:00:09.0: BAR 1: moved to bus [0x8080-0x8087] flags 0x20101
forcedeth 0000:00:09.0: BAR 2: got res [0xdefbe000-0xdefbe0ff] bus
[0xdefbe000-0xdefbe0ff] flags 0x20200
forcedeth 0000:00:09.0: BAR 2: moved to bus [0xdefbe000-0xdefbe0ff]
flags 0x20200
forcedeth 0000:00:09.0: BAR 3: got res [0xdefb8c00-0xdefb8c0f] bus
[0xdefb8c00-0xdefb8c0f] flags 0x20200
forcedeth 0000:00:09.0: BAR 3: moved to bus [0xdefb8c00-0xdefb8c0f]
flags 0x20200
forcedeth 0000:00:09.0: enabling device (0000 -> 0003)
forcedeth 0000:00:09.0: PCI INT A -> Link[LMAD] -> GSI 20 (level, low) -> IRQ 20
forcedeth 0000:00:09.0: enabling bus mastering
forcedeth 0000:00:09.0: setting latency timer to 64
forcedeth 0000:00:09.0: Invalid Mac address detected: ff:ff:ff:ff:ff:ff
forcedeth 0000:00:09.0: Please complain to your hardware vendor.
Switching to a random MAC.
forcedeth 0000:00:09.0: open: Could not find a valid PHY.
forcedeth 0000:00:09.0: PCI INT A disabled
forcedeth: probe of 0000:00:09.0 failed with error -12

forcedeth 0000:80:08.0: BAR 0: got res [0xddefb000-0xddefbfff] bus
[0xddefb000-0xddefbfff] flags 0x20200
forcedeth 0000:80:08.0: BAR 0: moved to bus [0xddefb000-0xddefbfff]
flags 0x20200
forcedeth 0000:80:08.0: BAR 1: got res [0x4080-0x4087] bus
[0x4080-0x4087] flags 0x20101
forcedeth 0000:80:08.0: BAR 1: moved to bus [0x4080-0x4087] flags 0x20101
forcedeth 0000:80:08.0: BAR 2: got res [0xddefac00-0xddefacff] bus
[0xddefac00-0xddefacff] flags 0x20200
forcedeth 0000:80:08.0: ...
From: Rafael J. Wysocki
Date: Sunday, August 17, 2008 - 12:29 pm

Okay, so perhaps it's better to do something like this:

---
 drivers/net/forcedeth.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/net/forcedeth.c
===================================================================
--- linux-2.6.orig/drivers/net/forcedeth.c
+++ linux-2.6/drivers/net/forcedeth.c
@@ -5975,10 +5975,12 @@ static void nv_shutdown(struct pci_dev *
 	if (netif_running(dev))
 		nv_close(dev);
 
-	pci_enable_wake(pdev, PCI_D3hot, np->wolenabled);
-	pci_enable_wake(pdev, PCI_D3cold, np->wolenabled);
 	pci_disable_device(pdev);
-	pci_set_power_state(pdev, PCI_D3hot);
+	if (system_state == SYS_POWER_OFF) {
+		if (pci_enable_wake(pdev, PCI_D3cold, np->wolenabled))
+			pci_enable_wake(pdev, PCI_D3hot, np->wolenabled);
+		pci_set_power_state(pdev, PCI_D3hot);
+	}
 }
 #else
 #define nv_suspend NULL
--

From: Rafael J. Wysocki
Date: Sunday, August 17, 2008 - 12:34 pm

---
 drivers/net/forcedeth.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/net/forcedeth.c
===================================================================
--- linux-2.6.orig/drivers/net/forcedeth.c
+++ linux-2.6/drivers/net/forcedeth.c
@@ -5975,10 +5975,12 @@ static void nv_shutdown(struct pci_dev *
 	if (netif_running(dev))
 		nv_close(dev);
 
-	pci_enable_wake(pdev, PCI_D3hot, np->wolenabled);
-	pci_enable_wake(pdev, PCI_D3cold, np->wolenabled);
 	pci_disable_device(pdev);
-	pci_set_power_state(pdev, PCI_D3hot);
+	if (system_state == SYSTEM_POWER_OFF) {
+		if (pci_enable_wake(pdev, PCI_D3cold, np->wolenabled))
+			pci_enable_wake(pdev, PCI_D3hot, np->wolenabled);
+		pci_set_power_state(pdev, PCI_D3hot);
+	}
 }
 #else
 #define nv_suspend NULL
--

From: Yinghai Lu
Date: Sunday, August 17, 2008 - 1:58 pm

why not like e1000 to
pci_set_power_state(pdev, pci_choose_state(pdev, PMSG_SUSPEND));

pci_choose_state would check if platform support those state...

YH
--

From: Rafael J. Wysocki
Date: Sunday, August 17, 2008 - 2:47 pm

... but ->shutdown() only involves the platform in the
'system_state == SYSTEM_POWER_OFF' case!

In fact, using pci_choose_state() in ->shutdown() is a very convoluted way of
checking the 'system_state' value, and not a 100% reliable one. :-)

Namely, the fact that pci_choose_state() returns you D0 instead of D3hot for
'system_state != SYSTEM_POWER_OFF' is a pure coincidence (accidentally, there
is an ACPI handle for the device, but there need not be one) and you should
not rely on that in general.  Generally speaking, pci_choose_state() is not to
be used outside of the drivers' ->suspend() routines.

[Yes, this means e1000 will have to be fixed, as I said before.]

Apart from this, pci_choose_state() is broken and will shortly be deprecated.
Moreover, in future all direct references to PMSG_SUSPEND from the drivers
will be removed.

Thanks,
Rafael
--

From: Rafael J. Wysocki
Date: Monday, August 18, 2008 - 3:22 am

Does the last patch work for you BTW?

Rafael
--

From: Yinghai Lu
Date: Monday, August 18, 2008 - 2:50 pm

it works.

YH
--

From: Rafael J. Wysocki
Date: Monday, August 18, 2008 - 3:08 pm

OK, thanks for testing.

I think we can use it as a quick fix for 2.6.27.  Do you agree?

Still, it would be helpful to verify if this is the same MSI issue reported by
Simon.

Thanks,
Rafael
--

From: Yinghai Lu
Date: Monday, August 18, 2008 - 3:36 pm

yes

YH
--

From: Rafael J. Wysocki
Date: Tuesday, August 19, 2008 - 11:45 am

From: Rafael J. Wysocki <rjw@sisk.pl>

forcedeth: Fix kexec regression

Fix regression tracked as
http://bugzilla.kernel.org/show_bug.cgi?id=11361 and
caused by commit f735a2a1a4f2a0f5cd823ce323e82675990469e2
("[netdrvr] forcedeth: setup wake-on-lan before shutting down")
that makes network adapters integrated into the NVidia
MCP55 chipsets fail to work in kexeced kernels.  The problem appears
to be that if the adapter is put into D3_hot during ->shutdown(),
it cannot be brought back into D0 after kexec (ref.
http://marc.info/?l=linux-kernel&m=121900062814967&w=4).  Therefore,
only put forcedeth into D3 during ->shutdown() if the system is to be
powered off.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Tested-by: Yinghai Lu <yhlu.kernel@gmail.com>
---
 drivers/net/forcedeth.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/net/forcedeth.c
===================================================================
--- linux-2.6.orig/drivers/net/forcedeth.c
+++ linux-2.6/drivers/net/forcedeth.c
@@ -5975,10 +5975,12 @@ static void nv_shutdown(struct pci_dev *
 	if (netif_running(dev))
 		nv_close(dev);
 
-	pci_enable_wake(pdev, PCI_D3hot, np->wolenabled);
-	pci_enable_wake(pdev, PCI_D3cold, np->wolenabled);
 	pci_disable_device(pdev);
-	pci_set_power_state(pdev, PCI_D3hot);
+	if (system_state == SYSTEM_POWER_OFF) {
+		if (pci_enable_wake(pdev, PCI_D3cold, np->wolenabled))
+			pci_enable_wake(pdev, PCI_D3hot, np->wolenabled);
+		pci_set_power_state(pdev, PCI_D3hot);
+	}
 }
 #else
 #define nv_suspend NULL
--

From: Andrew Morton
Date: Tuesday, August 19, 2008 - 1:37 pm

On Tue, 19 Aug 2008 20:45:53 +0200

hm, I wonder if this has any relation to
forcedeth-add-pci_enable_device-to-nv_resume.patch:


From: Simon Arlott <simon@fire.lp0.eu>

My NIC stops working after resuming from standby, it's not receiving any
interrupts.

Commit 25d90810ff49d2a63475776f24c74c6bb49b045f ([netdrvr] forcedeth:
reorder suspend/resume code) introduces pci_disable_device to nv_suspend,
but there's no corresponding pci_enable_device in nv_resume - so I added
one (copied from e1000).  This results in interrupts being re-enabled
after suspend.

However, the NIC (10de:0373) still doesn't work after resume.

Cc: Tobias Diedrich <ranma+kernel@tdiedrich.de>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Ayaz Abdulla <aabdulla@nvidia.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/net/forcedeth.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff -puN drivers/net/forcedeth.c~forcedeth-add-pci_enable_device-to-nv_resume drivers/net/forcedeth.c
--- a/drivers/net/forcedeth.c~forcedeth-add-pci_enable_device-to-nv_resume
+++ a/drivers/net/forcedeth.c
@@ -5960,6 +5960,13 @@ static int nv_resume(struct pci_dev *pde
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
+	rc = pci_enable_device(pdev);
+	if (rc) {
+		printk(KERN_ERR "forcedeth: Cannot enable PCI device from suspend\n");
+		return rc;
+	}
+	pci_set_master(pdev);
+
 	/* ack any pending wake events, disable PME */
 	pci_enable_wake(pdev, PCI_D0, 0);
 
_


--

From: Rafael J. Wysocki
Date: Tuesday, August 19, 2008 - 1:49 pm

There may be a connection, but we need a confirmation that kexec works on the
affected boxes with MSI disabled.  However, the $subject patch makes sense
even if that's the case IMO.

--

From: Eric W. Biederman
Date: Wednesday, August 20, 2008 - 12:01 am

Any chance we can fix this by teaching the forcedeth driver to
bring a card out of PCI_D3hot?

The kexec on panic guys are going to need that and it is just generally
more robust all of the way around than a special case in shutdown
based on system_state.

Eric
--

From: Rafael J. Wysocki
Date: Wednesday, August 20, 2008 - 6:12 am

Maybe, but that's not what I'm able to do at the moment. :-)


Actually, system_state == SYSTEM_POWER_OFF is a special case already, since
it is the only case in which ACPI gets involved.

Thanks,
Rafael
--

From: Yinghai Lu
Date: Monday, August 18, 2008 - 3:37 pm

yes

YH
--

From: Simon Arlott
Date: Monday, August 18, 2008 - 3:42 pm

I tried to test that patch but even without it standby/resume has stopped working:
http://img89.imageshack.us/img89/1861/1005552wb5.jpg (standby, tainted)
http://img89.imageshack.us/img89/5409/1005561ys2.jpg (resume)
http://img503.imageshack.us/img503/8720/1005571bc0.jpg (resume)
http://img135.imageshack.us/img135/5048/1005572be3.jpg (standby)

-- 
Simon Arlott
--

From: Rafael J. Wysocki
Date: Tuesday, August 19, 2008 - 10:58 am

Rafael
--

From: Simon Arlott
Date: Tuesday, August 19, 2008 - 11:33 am

linus-2.6 a7f5aaf36ded825477c4d7167cc6eb1bcdc63191

I've reverted to 2b12a4c524812fb3f6ee590a02e65b95c8c32229 where standby/resume still 
works and applied the above patch plus the pci_enable_device patch. It still doesn't 
work after resume if MSI is enabled.

-- 
Simon Arlott
--

From: Rafael J. Wysocki
Date: Tuesday, August 19, 2008 - 2:09 pm

My patch only affects the shutdown code path and is not related to
suspend/standby etc.

Could you identify the commit that broke standby/resume for you between
2b12a4c524812fb3f6ee590a02e65b95c8c32229 and
a7f5aaf36ded825477c4d7167cc6eb1bcdc63191 ?  It looks like that should be
bisectable.

Thanks,
Rafael
--

From: Simon Arlott
Date: Saturday, August 30, 2008 - 12:39 pm

It appears to be working again as of ee096f75b69913dbad0e6f7f2572513de5c90002.

-- 
Simon Arlott

--

Previous thread: [PATCH] net: forcedeth use pci_choose_state instead of PCI_D3hot by Yinghai Lu on Saturday, August 16, 2008 - 11:22 pm. (1 message)

Next thread: [PATCH] x86: remove first_free_entry/pin_map_size by Yinghai Lu on Saturday, August 16, 2008 - 11:31 pm. (2 messages)