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

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

[Yinghai Lu]

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

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

[Rafael J. Wysocki]

On Sunday, 17 of August 2008, Yinghai Lu wrote:
> 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.

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)
altogether?

> 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

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
> On Sunday, 17 of August 2008, Yinghai Lu wrote:
> > 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.

This is wrong.

> > So change that pci_choose_state(pdev, ...), and it works.
> 
> 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)
> altogether?

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

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

[Yinghai Lu]

On Sun, Aug 17, 2008 at 9:55 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
>> On Sunday, 17 of August 2008, Yinghai Lu wrote:
>> > 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.
>
> This is wrong.
>
>> > So change that pci_choose_state(pdev, ...), and it works.
>>
>> 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)
>> altogether?
>
> 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
>
>

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: BAR 2: moved to bus [0xddefac00-0xddefacff]
flags 0x20200
forcedeth 0000:80:08.0: BAR 3: got res [0xddefa800-0xddefa80f] bus
[0xddefa800-0xddefa80f] flags 0x20200
forcedeth 0000:80:08.0: BAR 3: moved to bus [0xddefa800-0xddefa80f]
flags 0x20200
forcedeth 0000:80:08.0: enabling device (0000 -> 0003)
forcedeth 0000:80:08.0: PCI INT A -> Link[IIM0] -> GSI 47 (level, low) -> IRQ 47
forcedeth 0000:80:08.0: enabling bus mastering
forcedeth 0000:80:08.0: setting latency timer to 64
nv_probe: set workarund bit for reversed mac addr
forcedeth 0000:80:08.0: ifname eth1, PHY OUI 0x5043 @ 2, addr 00:14:4f:8d:5d:b0
forcedeth 0000:80:08.0: highdma csum vlan pwrctl mgmt timirq gbit
lnktim msi desc-v3
...
initcall init_nic+0x0/0x1b returned 0 after 5351 msecs

YH

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

[Rafael J. Wysocki]

On Sunday, 17 of August 2008, Yinghai Lu wrote:
> On Sun, Aug 17, 2008 at 9:55 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
> >> On Sunday, 17 of August 2008, Yinghai Lu wrote:
> >> > 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.
> >
> > This is wrong.
> >
> >> > So change that pci_choose_state(pdev, ...), and it works.
> >>
> >> 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)
> >> altogether?
> >
> > 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
> >
> >
> 
> your patch doesn't work...  it seems that silicon has problem with D3Hot

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

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

[Rafael J. Wysocki]

On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
> On Sunday, 17 of August 2008, Yinghai Lu wrote:
> > On Sun, Aug 17, 2008 at 9:55 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
> > >> On Sunday, 17 of August 2008, Yinghai Lu wrote:
> > >> > 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.
> > >
> > > This is wrong.
> > >
> > >> > So change that pci_choose_state(pdev, ...), and it works.
> > >>
> > >> 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)
> > >> altogether?
> > >
> > > 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
> > >
> > >
> > 
> > your patch doesn't work...  it seems that silicon has problem with D3Hot
> 
> 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) {

Sorry, it should be SYSTEM_POWER_OFF here.  Corrected patch is appended.

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

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

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

[Yinghai Lu]

On Sun, Aug 17, 2008 at 12:34 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
>> On Sunday, 17 of August 2008, Yinghai Lu wrote:
>> > On Sun, Aug 17, 2008 at 9:55 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > > On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
>> > >> On Sunday, 17 of August 2008, Yinghai Lu wrote:
>> > >> > 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.
>> > >
>> > > This is wrong.
>> > >
>> > >> > So change that pci_choose_state(pdev, ...), and it works.
>> > >>
>> > >> 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)
>> > >> altogether?
>> > >
>> > > 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
>> > >
>> > >
>> >
>> > your patch doesn't work...  it seems that silicon has problem with D3Hot
>>
>> 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) {
>
> Sorry, it should be SYSTEM_POWER_OFF here.  Corrected patch is appended.
>
>> +             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
>> --
>
> ---
>  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
>

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

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

[Rafael J. Wysocki]

On Sunday, 17 of August 2008, Yinghai Lu wrote:
> On Sun, Aug 17, 2008 at 12:34 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
> >> On Sunday, 17 of August 2008, Yinghai Lu wrote:
> >> > On Sun, Aug 17, 2008 at 9:55 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > > On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
> >> > >> On Sunday, 17 of August 2008, Yinghai Lu wrote:
> >> > >> > 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.
> >> > >
> >> > > This is wrong.
> >> > >
> >> > >> > So change that pci_choose_state(pdev, ...), and it works.
> >> > >>
> >> > >> 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)
> >> > >> altogether?
> >> > >
> >> > > 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
> >> > >
> >> > >
> >> >
> >> > your patch doesn't work...  it seems that silicon has problem with D3Hot
> >>
> >> 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) {
> >
> > Sorry, it should be SYSTEM_POWER_OFF here.  Corrected patch is appended.
> >
> >> +             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
> >> --
> >
> > ---
> >  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
> >
> 
> 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...

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

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

[Rafael J. Wysocki]

On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
> On Sunday, 17 of August 2008, Yinghai Lu wrote:
> > On Sun, Aug 17, 2008 at 12:34 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
> > >> On Sunday, 17 of August 2008, Yinghai Lu wrote:
> > >> > On Sun, Aug 17, 2008 at 9:55 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > >> > > On Sunday, 17 of August 2008, Rafael J. Wysocki wrote:
> > >> > >> On Sunday, 17 of August 2008, Yinghai Lu wrote:
> > >> > >> > 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.
> > >> > >
> > >> > > This is wrong.
> > >> > >
> > >> > >> > So change that pci_choose_state(pdev, ...), and it works.
> > >> > >>
> > >> > >> 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)
> > >> > >> altogether?
> > >> > >
> > >> > > 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
> > >> > >
> > >> > >
> > >> >
> > >> > your patch doesn't work...  it seems that silicon has problem with D3Hot
> > >>
> > >> 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) {
> > >
> > > Sorry, it should be SYSTEM_POWER_OFF here.  Corrected patch is appended.
> > >
> > >> +             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
> > >> --
> > >
> > > ---
> > >  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
> > >
> > 
> > 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...
> 
> ... 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.

Does the last patch work for you BTW?

Rafael

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

[Yinghai Lu]

On Mon, Aug 18, 2008 at 3:22 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:

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

> Does the last patch work for you BTW?
>

it works.

YH

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

[Rafael J. Wysocki]

On Monday, 18 of August 2008, Yinghai Lu wrote:
> On Mon, Aug 18, 2008 at 3:22 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> >> > >  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
> >> > >
> >> >
> 
> > Does the last patch work for you BTW?
> >
> 
> it works.

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

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

[Yinghai Lu]

On Mon, Aug 18, 2008 at 3:08 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, 18 of August 2008, Yinghai Lu wrote:
>> On Mon, Aug 18, 2008 at 3:22 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>
>> >> > >  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
>> >> > >
>> >> >
>>
>> > Does the last patch work for you BTW?
>> >
>>
>> it works.
>
> OK, thanks for testing.
>
> I think we can use it as a quick fix for 2.6.27.  Do you agree?
>

yes

YH

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

[Yinghai Lu]

On Mon, Aug 18, 2008 at 3:08 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, 18 of August 2008, Yinghai Lu wrote:
>> On Mon, Aug 18, 2008 at 3:22 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>
>> >> > >  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
>> >> > >
>> >> >
>>
>> > Does the last patch work for you BTW?
>> >
>>
>> it works.
>
> OK, thanks for testing.
>
> I think we can use it as a quick fix for 2.6.27.  Do you agree?
>

yes

YH

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

[Simon Arlott]

On 18/08/08 23:08, Rafael J. Wysocki wrote:
> On Monday, 18 of August 2008, Yinghai Lu wrote:
>> On Mon, Aug 18, 2008 at 3:22 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> 
>> >> > >  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
>> >> > >
>> >> >
>> 
>> > Does the last patch work for you BTW?
>> >
>> 
>> it works.
> 
> 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.

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

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

[Rafael J. Wysocki]

On Tuesday, 19 of August 2008, Simon Arlott wrote:
> On 18/08/08 23:08, Rafael J. Wysocki wrote:
> > On Monday, 18 of August 2008, Yinghai Lu wrote:
> >> On Mon, Aug 18, 2008 at 3:22 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> 
> >> >> > >  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
> >> >> > >
> >> >> >
> >> 
> >> > Does the last patch work for you BTW?
> >> >
> >> 
> >> it works.
> > 
> > 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.
> 
> I tried to test that patch but even without it standby/resume has stopped working:

Which kernel is this?

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

Rafael

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

[Simon Arlott]

On 19/08/08 18:58, Rafael J. Wysocki wrote:
> On Tuesday, 19 of August 2008, Simon Arlott wrote:
>> On 18/08/08 23:08, Rafael J. Wysocki wrote:
>> > On Monday, 18 of August 2008, Yinghai Lu wrote:
>> >> On Mon, Aug 18, 2008 at 3:22 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> 
>> >> >> > >  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
>> >> >> > >
>> >> >> >
>> >> 
>> >> > Does the last patch work for you BTW?
>> >> >
>> >> 
>> >> it works.
>> > 
>> > 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.
>> 
>> I tried to test that patch but even without it standby/resume has stopped working:
> 
> Which kernel is this?

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

[PATCH] forcedeth: Fix kexec regression

[Rafael J. Wysocki]

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

Re: [PATCH] forcedeth: Fix kexec regression

[Andrew Morton]

On Tue, 19 Aug 2008 20:45:53 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> 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

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);
 
_

Re: [PATCH] forcedeth: Fix kexec regression

[Rafael J. Wysocki]

On Tuesday, 19 of August 2008, Andrew Morton wrote:
> On Tue, 19 Aug 2008 20:45:53 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > 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
> 
> hm, I wonder if this has any relation to
> forcedeth-add-pci_enable_device-to-nv_resume.patch:

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.

Anyway, the patch below fixes things for Simon only if MSI are disabled.

> 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);
>  
> _

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

[Rafael J. Wysocki]

On Tuesday, 19 of August 2008, Simon Arlott wrote:
> On 19/08/08 18:58, Rafael J. Wysocki wrote:
> > On Tuesday, 19 of August 2008, Simon Arlott wrote:
> >> On 18/08/08 23:08, Rafael J. Wysocki wrote:
> >> > On Monday, 18 of August 2008, Yinghai Lu wrote:
> >> >> On Mon, Aug 18, 2008 at 3:22 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >> 
> >> >> >> > >  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
> >> >> >> > >
> >> >> >> >
> >> >> 
> >> >> > Does the last patch work for you BTW?
> >> >> >
> >> >> 
> >> >> it works.
> >> > 
> >> > 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.
> >> 
> >> I tried to test that patch but even without it standby/resume has stopped working:
> > 
> > Which kernel is this?
> 
> 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.

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

Re: [PATCH] forcedeth: Fix kexec regression

[Eric W. Biederman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

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

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

Re: [PATCH] forcedeth: Fix kexec regression

[Rafael J. Wysocki]

On Wednesday, 20 of August 2008, Eric W. Biederman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > 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.
> 
> Any chance we can fix this by teaching the forcedeth driver to
> bring a card out of PCI_D3hot?

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

Yinghai Lu thinks that the hardware has a problem with that, though.

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

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

Thanks,
Rafael

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

[Simon Arlott]

On 19/08/08 22:09, Rafael J. Wysocki wrote:
> On Tuesday, 19 of August 2008, Simon Arlott wrote:
>> On 19/08/08 18:58, Rafael J. Wysocki wrote:
>> > On Tuesday, 19 of August 2008, Simon Arlott wrote:
>> >> On 18/08/08 23:08, Rafael J. Wysocki wrote:
>> >> > On Monday, 18 of August 2008, Yinghai Lu wrote:
>> >> >> On Mon, Aug 18, 2008 at 3:22 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> >> 
>> >> >> >> > >  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
>> >> >> >> > >
>> >> >> >> >
>> >> >> 
>> >> >> > Does the last patch work for you BTW?
>> >> >> >
>> >> >> 
>> >> >> it works.
>> >> > 
>> >> > 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.
>> >> 
>> >> I tried to test that patch but even without it standby/resume has stopped working:
>> > 
>> > Which kernel is this?
>> 
>> 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.
> 
> 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.

It appears to be working again as of ee096f75b69913dbad0e6f7f2572513de5c90002.

-- 
Simon Arlott