[PATCH] igb: fix kexec with igb

[PATCH] igb: fix kexec with igb

[Yinghai Lu]

Impact: could probe igb

Found one system with 82575EB, in the kernel that is kexeced, probe igb
failed with -2.

it looks like the same behavior happened on forcedeth.

try to check system_state to make sure if put it on D3

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/net/igb/igb_main.c
===================================================================
--- linux-2.6.orig/drivers/net/igb/igb_main.c
+++ linux-2.6/drivers/net/igb/igb_main.c
@@ -4358,14 +4358,22 @@ static int igb_suspend(struct pci_dev *p
 		wr32(E1000_WUFC, 0);
 	}
 
+	/*
+	 * Apparently it is not possible to reinitialise from D3 hot,
+	 * only put the device into D3 if we really go for poweroff.
+	 */
 	/* make sure adapter isn't asleep if manageability/wol is enabled */
 	if (wufc || adapter->en_mng_pt) {
-		pci_enable_wake(pdev, PCI_D3hot, 1);
-		pci_enable_wake(pdev, PCI_D3cold, 1);
+		if (system_state == SYSTEM_POWER_OFF) {
+			pci_enable_wake(pdev, PCI_D3hot, 1);
+			pci_enable_wake(pdev, PCI_D3cold, 1);
+		}
 	} else {
 		igb_shutdown_fiber_serdes_link_82575(hw);
-		pci_enable_wake(pdev, PCI_D3hot, 0);
-		pci_enable_wake(pdev, PCI_D3cold, 0);
+		if (system_state == SYSTEM_POWER_OFF) {
+			pci_enable_wake(pdev, PCI_D3hot, 0);
+			pci_enable_wake(pdev, PCI_D3cold, 0);
+		}
 	}
 
 	/* Release control of h/w to f/w.  If f/w is AMT enabled, this
@@ -4374,7 +4382,8 @@ static int igb_suspend(struct pci_dev *p
 
 	pci_disable_device(pdev);
 
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
+	if (system_state == SYSTEM_POWER_OFF)
+		pci_set_power_state(pdev, pci_choose_state(pdev, state));
 
 	return 0;
 }

Re: [PATCH] igb: fix kexec with igb

[Jesse Brandeburg]

On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>
> Impact: could probe igb
>
> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> failed with -2.
>
> it looks like the same behavior happened on forcedeth.
>
> try to check system_state to make sure if put it on D3
>
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>
> ---
>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)

I see the point of the patch, but I know for a fact that ixgbe when
enabled for MSI-X also doesn't work with kexec.

so my questions are:
are you going to change every driver?
why can't this be fixed in core kernel code instead?
Shouldn't pci_enable_device take it out of D3?
Or maybe it should be taken out of D3 immediately if someone tries to
ioremap any of the BARx registers?

Jesse

Re: [PATCH] igb: fix kexec with igb

[Yinghai Lu]

On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
<jesse.brandeburg@gmail.com> wrote:
> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>
>> Impact: could probe igb
>>
>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
>> failed with -2.
>>
>> it looks like the same behavior happened on forcedeth.
>>
>> try to check system_state to make sure if put it on D3
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> I see the point of the patch, but I know for a fact that ixgbe when
> enabled for MSI-X also doesn't work with kexec.
>
> so my questions are:
> are you going to change every driver?

i tend to only change driver that i have related HW.

> why can't this be fixed in core kernel code instead?
will check it.

> Shouldn't pci_enable_device take it out of D3?
> Or maybe it should be taken out of D3 immediately if someone tries to
> ioremap any of the BARx registers?


looks like second kernel can not detect the state any more.

YH

Re: [PATCH] igb: fix kexec with igb

[Eric W. Biederman]

Yinghai Lu <yinghai@kernel.org> writes:

> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> <jesse.brandeburg@gmail.com> wrote:
>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>
>>> Impact: could probe igb
>>>
>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
>>> failed with -2.
>>>
>>> it looks like the same behavior happened on forcedeth.
>>>
>>> try to check system_state to make sure if put it on D3
>>>
>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>
>>> ---
>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> I see the point of the patch, but I know for a fact that ixgbe when
>> enabled for MSI-X also doesn't work with kexec.
>>
>> so my questions are:
>> are you going to change every driver?
>
> i tend to only change driver that i have related HW.
>
>> why can't this be fixed in core kernel code instead?
> will check it.
>
>> Shouldn't pci_enable_device take it out of D3?
>> Or maybe it should be taken out of D3 immediately if someone tries to
>> ioremap any of the BARx registers?
>
>
> looks like second kernel can not detect the state any more.

I know this has historically been a problem with the e1000 NICs.
Placing the hardware in a state they can not get them out of on
the reboot path.

Last I heard (a couple of weeks ago?) we had code to bring devices out
of a low power state that was working for the e1000 driver.

YH can you look and see if you can find that code and if it works?

<rant>
Frankly I don't understand why anyone would want to power down a device
when they are rebooting or shutting down a computer.  That is a
system state change.  But it seems to be bleed over from the confusion
that has been the power management code.
</rant>

If we can teach the kernel to handle this case with the proper enables
and disables that would be great.  Otherwise let's look at getting the
responsibilities of the various methods sorted out so we can at least
say with certainty what the various methods are supposed to do and
not do.

Eric

Re: [PATCH] igb: fix kexec with igb

[Yinghai Lu]

On Sat, Mar 7, 2009 at 10:20 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Yinghai Lu <yinghai@kernel.org> writes:
>
>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
>> <jesse.brandeburg@gmail.com> wrote:
>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>>
>>>> Impact: could probe igb
>>>>
>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
>>>> failed with -2.
>>>>
>>>> it looks like the same behavior happened on forcedeth.
>>>>
>>>> try to check system_state to make sure if put it on D3
>>>>
>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>>
>>>> ---
>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>
>>> I see the point of the patch, but I know for a fact that ixgbe when
>>> enabled for MSI-X also doesn't work with kexec.
>>>
>>> so my questions are:
>>> are you going to change every driver?
>>
>> i tend to only change driver that i have related HW.
>>
>>> why can't this be fixed in core kernel code instead?
>> will check it.
>>
>>> Shouldn't pci_enable_device take it out of D3?
>>> Or maybe it should be taken out of D3 immediately if someone tries to
>>> ioremap any of the BARx registers?
>>
>>
>> looks like second kernel can not detect the state any more.
>
> I know this has historically been a problem with the e1000 NICs.
> Placing the hardware in a state they can not get them out of on
> the reboot path.
>
> Last I heard (a couple of weeks ago?) we had code to bring devices out
> of a low power state that was working for the e1000 driver.

in net-next tree?

>
> YH can you look and see if you can find that code and if it works?

it seems e1000 and e1000e works well for a long time.

>
> <rant>
> Frankly I don't understand why anyone would want to power down a device
> when they are rebooting or shutting down a computer.  That is a
> system state change.  But it seems to be bleed over from the confusion
> that has been the power management code.
> </rant>

agreed.

>
> If we can teach the kernel to handle this case with the proper enables
> and disables that would be great.  Otherwise let's look at getting the
> responsibilities of the various methods sorted out so we can at least
> say with certainty what the various methods are supposed to do and
> not do.

root cause could be some BIOS/acpi lying to Kernel about device state querying.

for igb, it only has problem on one platform. but other one platform is ok.
for forcedeth, it seems all platform has the problem, aka you can not
put it in D3 in kexec path.

YH

[PATCH] pci: fix kexec with power state D3

[Yinghai Lu]

Impact: second kernel by kexec will have some pci devices working

Found one system with 82575EB, in the kernel that is kexeced, probe igb
failed with -2.

it looks like the same behavior happened on forcedeth.
try to check system_state to make sure if put it on D3

Jesse Brandeburg said that we should do that check in core code instead of
every device driver.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/pci.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -593,6 +593,14 @@ int pci_set_power_state(struct pci_dev *
 	if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
 		return 0;
 
+	/*
+	 * Apparently it is not possible to reinitialise from D3 hot,
+	 * only put the device into D3 if we really go for poweroff.
+	 */
+	if (system_state != SYSTEM_POWER_OFF &&
+	    (state == PCI_D3hot || state == PCI_D3cold))
+		return 0;
+
 	error = pci_raw_set_power_state(dev, state, true);
 
 	if (state > PCI_D0 && platform_pci_power_manageable(dev)) {
@@ -1124,6 +1132,15 @@ int pci_enable_wake(struct pci_dev *dev,
 	int error = 0;
 	bool pme_done = false;
 
+	/*
+	 * Apparently it is not possible to reinitialise from D3 hot,
+	 * only put the device into D3 if we really go for poweroff.
+	 * we only need to enable wake when we are going to power off
+	 */
+	if (enable && system_state != SYSTEM_POWER_OFF &&
+	    (state == PCI_D3hot || state == PCI_D3cold))
+		return 0;
+
 	if (enable && !device_may_wakeup(&dev->dev))
 		return -EINVAL;
 

Re: [PATCH] pci: fix kexec with power state D3

[Rafael J. Wysocki]

On Sunday 08 March 2009, Yinghai Lu wrote:
> 
> Impact: second kernel by kexec will have some pci devices working
> 
> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> failed with -2.
> 
> it looks like the same behavior happened on forcedeth.
> try to check system_state to make sure if put it on D3

This is not enough, because the PM code doesn't change system_state and
it uses pci_set_power_state too.

> Jesse Brandeburg said that we should do that check in core code instead of
> every device driver.

Well, I'm not really sure.  The drivers are where the bug is.

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/pci.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> Index: linux-2.6/drivers/pci/pci.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.c
> +++ linux-2.6/drivers/pci/pci.c
> @@ -593,6 +593,14 @@ int pci_set_power_state(struct pci_dev *
>  	if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
>  		return 0;
>  
> +	/*
> +	 * Apparently it is not possible to reinitialise from D3 hot,
> +	 * only put the device into D3 if we really go for poweroff.
> +	 */
> +	if (system_state != SYSTEM_POWER_OFF &&
> +	    (state == PCI_D3hot || state == PCI_D3cold))
> +		return 0;
> +

This breaks suspend/hibernation, doesn't it?  Surely, we want to put devices
into D3 when going for suspend, for example.

That's apart from the fact that the 'state == PCI_D3cold' is redundant.

>  	error = pci_raw_set_power_state(dev, state, true);
>  
>  	if (state > PCI_D0 && platform_pci_power_manageable(dev)) {
> @@ -1124,6 +1132,15 @@ int pci_enable_wake(struct pci_dev *dev,
>  	int error = 0;
>  	bool pme_done = false;
>  
> +	/*
> +	 * Apparently it is not possible to reinitialise from D3 hot,
> +	 * only put the device into D3 if we really go for poweroff.
> +	 * we only need to enable wake when we are going to power off
> +	 */
> +	if (enable && system_state != SYSTEM_POWER_OFF &&
> +	    (state == PCI_D3hot || state == PCI_D3cold))
> +		return 0;
> +
>  	if (enable && !device_may_wakeup(&dev->dev))
>  		return -EINVAL;

I don't like this at all, sorry.

I thought we were supposed to avoid using system_state in such a way,
apart from the other things.

Thanks,
Rafael

Re: [PATCH] pci: fix kexec with power state D3

[Rafael J. Wysocki]

On Sunday 08 March 2009, Yinghai Lu wrote:
> 
> Impact: second kernel by kexec will have some pci devices working
> 
> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> failed with -2.
> 
> it looks like the same behavior happened on forcedeth.
> try to check system_state to make sure if put it on D3

This is not enough, because the PM code doesn't change system_state and
it uses pci_set_power_state too.

> Jesse Brandeburg said that we should do that check in core code instead of
> every device driver.

Well, I'm not really sure.  The drivers are where the bug is.

> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/pci/pci.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> Index: linux-2.6/drivers/pci/pci.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.c
> +++ linux-2.6/drivers/pci/pci.c
> @@ -593,6 +593,14 @@ int pci_set_power_state(struct pci_dev *
>  	if (state == PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
>  		return 0;
>  
> +	/*
> +	 * Apparently it is not possible to reinitialise from D3 hot,
> +	 * only put the device into D3 if we really go for poweroff.
> +	 */
> +	if (system_state != SYSTEM_POWER_OFF &&
> +	    (state == PCI_D3hot || state == PCI_D3cold))
> +		return 0;
> +

This breaks suspend/hibernation, doesn't it?  Surely, we want to put devices
into D3 when going for suspend, for example.

That's apart from the fact that the 'state == PCI_D3cold' is redundant.

>  	error = pci_raw_set_power_state(dev, state, true);
>  
>  	if (state > PCI_D0 && platform_pci_power_manageable(dev)) {
> @@ -1124,6 +1132,15 @@ int pci_enable_wake(struct pci_dev *dev,
>  	int error = 0;
>  	bool pme_done = false;
>  
> +	/*
> +	 * Apparently it is not possible to reinitialise from D3 hot,
> +	 * only put the device into D3 if we really go for poweroff.
> +	 * we only need to enable wake when we are going to power off
> +	 */
> +	if (enable && system_state != SYSTEM_POWER_OFF &&
> +	    (state == PCI_D3hot || state == PCI_D3cold))
> +		return 0;
> +
>  	if (enable && !device_may_wakeup(&dev->dev))
>  		return -EINVAL;

I don't like this at all, sorry.

I thought we were supposed to avoid using system_state in such a way,
apart from the other things.

Thanks,
Rafael

Re: [PATCH] pci: fix kexec with power state D3

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Sunday 08 March 2009, Yinghai Lu wrote:
> > 
> > Impact: second kernel by kexec will have some pci devices working
> > 
> > Found one system with 82575EB, in the kernel that is kexeced, probe igb
> > failed with -2.
> > 
> > it looks like the same behavior happened on forcedeth.
> > try to check system_state to make sure if put it on D3
> 
> This is not enough, because the PM code doesn't change 
> system_state and it uses pci_set_power_state too.
> 
> > Jesse Brandeburg said that we should do that check in core 
> > code instead of every device driver.
> 
> Well, I'm not really sure.  The drivers are where the bug is.

So Yinghai has now written a driver fix and a generic code fix 
as well and both are being rejected pointing to the other side? 

This really sucks for users (who'd be happy with any of the 
patches) and the disagreement needs to be resolved ASAP.

	Ingo

Re: [PATCH] pci: fix kexec with power state D3

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Sunday 08 March 2009, Yinghai Lu wrote:
> > 
> > Impact: second kernel by kexec will have some pci devices working
> > 
> > Found one system with 82575EB, in the kernel that is kexeced, probe igb
> > failed with -2.
> > 
> > it looks like the same behavior happened on forcedeth.
> > try to check system_state to make sure if put it on D3
> 
> This is not enough, because the PM code doesn't change 
> system_state and it uses pci_set_power_state too.
> 
> > Jesse Brandeburg said that we should do that check in core 
> > code instead of every device driver.
> 
> Well, I'm not really sure.  The drivers are where the bug is.

So Yinghai has now written a driver fix and a generic code fix 
as well and both are being rejected pointing to the other side? 

This really sucks for users (who'd be happy with any of the 
patches) and the disagreement needs to be resolved ASAP.

	Ingo

Re: [PATCH] pci: fix kexec with power state D3

[Rafael J. Wysocki]

On Sunday 08 March 2009, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > On Sunday 08 March 2009, Yinghai Lu wrote:
> > > 
> > > Impact: second kernel by kexec will have some pci devices working
> > > 
> > > Found one system with 82575EB, in the kernel that is kexeced, probe igb
> > > failed with -2.
> > > 
> > > it looks like the same behavior happened on forcedeth.
> > > try to check system_state to make sure if put it on D3
> > 
> > This is not enough, because the PM code doesn't change 
> > system_state and it uses pci_set_power_state too.
> > 
> > > Jesse Brandeburg said that we should do that check in core 
> > > code instead of every device driver.
> > 
> > Well, I'm not really sure.  The drivers are where the bug is.
> 
> So Yinghai has now written a driver fix and a generic code fix 
> as well and both are being rejected pointing to the other side? 
> 
> This really sucks for users (who'd be happy with any of the 
> patches) and the disagreement needs to be resolved ASAP.

I hope you read my other comment (that you deleted from the reply)?

If we're going to fix this at the core level along the Yinghai's patch lines,
we'll have to introduce a few additional values for system_state and make
suspend and hibernation code use them.

Do you think it's worth the effort, given that only a couple of drivers have
been found to be affected so far?

Rafael

Re: [PATCH] pci: fix kexec with power state D3

[Rafael J. Wysocki]

On Sunday 08 March 2009, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > On Sunday 08 March 2009, Yinghai Lu wrote:
> > > 
> > > Impact: second kernel by kexec will have some pci devices working
> > > 
> > > Found one system with 82575EB, in the kernel that is kexeced, probe igb
> > > failed with -2.
> > > 
> > > it looks like the same behavior happened on forcedeth.
> > > try to check system_state to make sure if put it on D3
> > 
> > This is not enough, because the PM code doesn't change 
> > system_state and it uses pci_set_power_state too.
> > 
> > > Jesse Brandeburg said that we should do that check in core 
> > > code instead of every device driver.
> > 
> > Well, I'm not really sure.  The drivers are where the bug is.
> 
> So Yinghai has now written a driver fix and a generic code fix 
> as well and both are being rejected pointing to the other side? 
> 
> This really sucks for users (who'd be happy with any of the 
> patches) and the disagreement needs to be resolved ASAP.

I hope you read my other comment (that you deleted from the reply)?

If we're going to fix this at the core level along the Yinghai's patch lines,
we'll have to introduce a few additional values for system_state and make
suspend and hibernation code use them.

Do you think it's worth the effort, given that only a couple of drivers have
been found to be affected so far?

Rafael

Re: [PATCH] pci: fix kexec with power state D3

[Rafael J. Wysocki]

On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> On Sunday 08 March 2009, Ingo Molnar wrote:
> > 
> > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > 
> > > On Sunday 08 March 2009, Yinghai Lu wrote:
> > > > 
> > > > Impact: second kernel by kexec will have some pci devices working
> > > > 
> > > > Found one system with 82575EB, in the kernel that is kexeced, probe igb
> > > > failed with -2.
> > > > 
> > > > it looks like the same behavior happened on forcedeth.
> > > > try to check system_state to make sure if put it on D3
> > > 
> > > This is not enough, because the PM code doesn't change 
> > > system_state and it uses pci_set_power_state too.
> > > 
> > > > Jesse Brandeburg said that we should do that check in core 
> > > > code instead of every device driver.
> > > 
> > > Well, I'm not really sure.  The drivers are where the bug is.
> > 
> > So Yinghai has now written a driver fix and a generic code fix 
> > as well and both are being rejected pointing to the other side? 
> > 
> > This really sucks for users (who'd be happy with any of the 
> > patches) and the disagreement needs to be resolved ASAP.
> 
> I hope you read my other comment (that you deleted from the reply)?
> 
> If we're going to fix this at the core level along the Yinghai's patch lines,
> we'll have to introduce a few additional values for system_state and make
> suspend and hibernation code use them.
> 
> Do you think it's worth the effort, given that only a couple of drivers have
> been found to be affected so far?

And, quite frankly, I'm not sure if users will be happy with the $subject
patch, because it _really_ breaks things (well, the kexec users who don't use
suspend might be, but surely suspend users who don't use kexec won't).

Thanks,
Rafael

Re: [PATCH] pci: fix kexec with power state D3

[Rafael J. Wysocki]

On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> On Sunday 08 March 2009, Ingo Molnar wrote:
> > 
> > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > 
> > > On Sunday 08 March 2009, Yinghai Lu wrote:
> > > > 
> > > > Impact: second kernel by kexec will have some pci devices working
> > > > 
> > > > Found one system with 82575EB, in the kernel that is kexeced, probe igb
> > > > failed with -2.
> > > > 
> > > > it looks like the same behavior happened on forcedeth.
> > > > try to check system_state to make sure if put it on D3
> > > 
> > > This is not enough, because the PM code doesn't change 
> > > system_state and it uses pci_set_power_state too.
> > > 
> > > > Jesse Brandeburg said that we should do that check in core 
> > > > code instead of every device driver.
> > > 
> > > Well, I'm not really sure.  The drivers are where the bug is.
> > 
> > So Yinghai has now written a driver fix and a generic code fix 
> > as well and both are being rejected pointing to the other side? 
> > 
> > This really sucks for users (who'd be happy with any of the 
> > patches) and the disagreement needs to be resolved ASAP.
> 
> I hope you read my other comment (that you deleted from the reply)?
> 
> If we're going to fix this at the core level along the Yinghai's patch lines,
> we'll have to introduce a few additional values for system_state and make
> suspend and hibernation code use them.
> 
> Do you think it's worth the effort, given that only a couple of drivers have
> been found to be affected so far?

And, quite frankly, I'm not sure if users will be happy with the $subject
patch, because it _really_ breaks things (well, the kexec users who don't use
suspend might be, but surely suspend users who don't use kexec won't).

Thanks,
Rafael

Re: [PATCH] igb: fix kexec with igb

[Rafael J. Wysocki]

On Saturday 07 March 2009, Yinghai Lu wrote:
> On Sat, Mar 7, 2009 at 10:20 AM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
> > Yinghai Lu <yinghai@kernel.org> writes:
> >
> >> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> >> <jesse.brandeburg@gmail.com> wrote:
> >>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >>>>
> >>>> Impact: could probe igb
> >>>>
> >>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> >>>> failed with -2.
> >>>>
> >>>> it looks like the same behavior happened on forcedeth.
> >>>>
> >>>> try to check system_state to make sure if put it on D3
> >>>>
> >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >>>>
> >>>> ---
> >>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
> >>>>  1 file changed, 14 insertions(+), 5 deletions(-)
> >>>
> >>> I see the point of the patch, but I know for a fact that ixgbe when
> >>> enabled for MSI-X also doesn't work with kexec.
> >>>
> >>> so my questions are:
> >>> are you going to change every driver?
> >>
> >> i tend to only change driver that i have related HW.
> >>
> >>> why can't this be fixed in core kernel code instead?
> >> will check it.
> >>
> >>> Shouldn't pci_enable_device take it out of D3?
> >>> Or maybe it should be taken out of D3 immediately if someone tries to
> >>> ioremap any of the BARx registers?
> >>
> >>
> >> looks like second kernel can not detect the state any more.
> >
> > I know this has historically been a problem with the e1000 NICs.
> > Placing the hardware in a state they can not get them out of on
> > the reboot path.
> >
> > Last I heard (a couple of weeks ago?) we had code to bring devices out
> > of a low power state that was working for the e1000 driver.
> 
> in net-next tree?
> 
> >
> > YH can you look and see if you can find that code and if it works?
> 
> it seems e1000 and e1000e works well for a long time.
> 
> >
> > <rant>
> > Frankly I don't understand why anyone would want to power down a device
> > when they are rebooting or shutting down a computer.  That is a
> > system state change.  But it seems to be bleed over from the confusion
> > that has been the power management code.
> > </rant>
> 
> agreed.

Well, well.

IMHO, the confusion is that ->shutdown() is used for too many _different_
things, because kexec is sufficiently different from power off, for example, to
be handled by a separate driver callback.  I'm totally unsure what power
management has to do with it, though.

Thanks,
Rafael

Re: [PATCH] igb: fix kexec with igb

[Rafael J. Wysocki]

On Saturday 07 March 2009, Yinghai Lu wrote:
> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> <jesse.brandeburg@gmail.com> wrote:
> > On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >>
> >> Impact: could probe igb
> >>
> >> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> >> failed with -2.
> >>
> >> it looks like the same behavior happened on forcedeth.
> >>
> >> try to check system_state to make sure if put it on D3
> >>
> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >>
> >> ---
> >>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
> >>  1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > I see the point of the patch, but I know for a fact that ixgbe when
> > enabled for MSI-X also doesn't work with kexec.
> >
> > so my questions are:
> > are you going to change every driver?
> 
> i tend to only change driver that i have related HW.
> 
> > why can't this be fixed in core kernel code instead?
> will check it.
> 
> > Shouldn't pci_enable_device take it out of D3?
> > Or maybe it should be taken out of D3 immediately if someone tries to
> > ioremap any of the BARx registers?
> 
> 
> looks like second kernel can not detect the state any more.

In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
thing.  The question is why it doesn't work as expected.

What kernel(s) have you tested?

Thanks,
Rafael

Re: [PATCH] pci: fix kexec with power state D3

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> > On Sunday 08 March 2009, Ingo Molnar wrote:
> > > 
> > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > 
> > > > On Sunday 08 March 2009, Yinghai Lu wrote:
> > > > > 
> > > > > Impact: second kernel by kexec will have some pci devices working
> > > > > 
> > > > > Found one system with 82575EB, in the kernel that is kexeced, probe igb
> > > > > failed with -2.
> > > > > 
> > > > > it looks like the same behavior happened on forcedeth.
> > > > > try to check system_state to make sure if put it on D3
> > > > 
> > > > This is not enough, because the PM code doesn't change 
> > > > system_state and it uses pci_set_power_state too.
> > > > 
> > > > > Jesse Brandeburg said that we should do that check in core 
> > > > > code instead of every device driver.
> > > > 
> > > > Well, I'm not really sure.  The drivers are where the bug is.
> > > 
> > > So Yinghai has now written a driver fix and a generic code fix 
> > > as well and both are being rejected pointing to the other side? 
> > > 
> > > This really sucks for users (who'd be happy with any of the 
> > > patches) and the disagreement needs to be resolved ASAP.
> > 
> > I hope you read my other comment (that you deleted from the reply)?
> > 
> > If we're going to fix this at the core level along the Yinghai's patch lines,
> > we'll have to introduce a few additional values for system_state and make
> > suspend and hibernation code use them.
> > 
> > Do you think it's worth the effort, given that only a couple of drivers have
> > been found to be affected so far?
> 
> And, quite frankly, I'm not sure if users will be happy with 
> the $subject patch, because it _really_ breaks things (well, 
> the kexec users who don't use suspend might be, but surely 
> suspend users who don't use kexec won't).

Please note that i havent reviewed the patches and i did not 
take any sides in the discussion - i just flagged the maintainer 
ping-pong. As long as we pick one of the patches (or a third 
one) within a bound amount of time we should be fine :)

	Ingo

Re: [PATCH] pci: fix kexec with power state D3

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> > On Sunday 08 March 2009, Ingo Molnar wrote:
> > > 
> > > * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > > 
> > > > On Sunday 08 March 2009, Yinghai Lu wrote:
> > > > > 
> > > > > Impact: second kernel by kexec will have some pci devices working
> > > > > 
> > > > > Found one system with 82575EB, in the kernel that is kexeced, probe igb
> > > > > failed with -2.
> > > > > 
> > > > > it looks like the same behavior happened on forcedeth.
> > > > > try to check system_state to make sure if put it on D3
> > > > 
> > > > This is not enough, because the PM code doesn't change 
> > > > system_state and it uses pci_set_power_state too.
> > > > 
> > > > > Jesse Brandeburg said that we should do that check in core 
> > > > > code instead of every device driver.
> > > > 
> > > > Well, I'm not really sure.  The drivers are where the bug is.
> > > 
> > > So Yinghai has now written a driver fix and a generic code fix 
> > > as well and both are being rejected pointing to the other side? 
> > > 
> > > This really sucks for users (who'd be happy with any of the 
> > > patches) and the disagreement needs to be resolved ASAP.
> > 
> > I hope you read my other comment (that you deleted from the reply)?
> > 
> > If we're going to fix this at the core level along the Yinghai's patch lines,
> > we'll have to introduce a few additional values for system_state and make
> > suspend and hibernation code use them.
> > 
> > Do you think it's worth the effort, given that only a couple of drivers have
> > been found to be affected so far?
> 
> And, quite frankly, I'm not sure if users will be happy with 
> the $subject patch, because it _really_ breaks things (well, 
> the kexec users who don't use suspend might be, but surely 
> suspend users who don't use kexec won't).

Please note that i havent reviewed the patches and i did not 
take any sides in the discussion - i just flagged the maintainer 
ping-pong. As long as we pick one of the patches (or a third 
one) within a bound amount of time we should be fine :)

	Ingo

Re: [PATCH] igb: fix kexec with igb

[Yinghai Lu]

Rafael J. Wysocki wrote:

>>
>> looks like second kernel can not detect the state any more.
> 
> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
> thing.  The question is why it doesn't work as expected.
> 
> What kernel(s) have you tested?

tip/master

also RHEL 5.3 with igb + kexec have the same problem.

YH

Re: [PATCH] igb: fix kexec with igb

[Yinghai Lu]

Rafael J. Wysocki wrote:
> On Saturday 07 March 2009, Yinghai Lu wrote:
>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
>> <jesse.brandeburg@gmail.com> wrote:
>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>> Impact: could probe igb
>>>>
>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
>>>> failed with -2.
>>>>
>>>> it looks like the same behavior happened on forcedeth.
>>>>
>>>> try to check system_state to make sure if put it on D3
>>>>
>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>>
>>>> ---
>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>> I see the point of the patch, but I know for a fact that ixgbe when
>>> enabled for MSI-X also doesn't work with kexec.
>>>
>>> so my questions are:
>>> are you going to change every driver?
>> i tend to only change driver that i have related HW.
>>
>>> why can't this be fixed in core kernel code instead?
>> will check it.
>>
>>> Shouldn't pci_enable_device take it out of D3?
>>> Or maybe it should be taken out of D3 immediately if someone tries to
>>> ioremap any of the BARx registers?
>>
>> looks like second kernel can not detect the state any more.
> 
> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
> thing.  The question is why it doesn't work as expected.

not sure... please check the version for forcedeth that you made.

commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date:   Fri Sep 5 14:00:19 2008 -0700

    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>
    Cc: Ayaz Abdulla <aabdulla@nvidia.com>
    Acked-by: Jeff Garzik <jeff@garzik.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 331b86b..0b6ecef 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -5975,10 +5975,12 @@ static void nv_shutdown(struct pci_dev *pdev)
        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] igb: fix kexec with igb

[Rafael J. Wysocki]

On Sunday 08 March 2009, Yinghai Lu wrote:
> Rafael J. Wysocki wrote:
> > On Saturday 07 March 2009, Yinghai Lu wrote:
> >> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> >> <jesse.brandeburg@gmail.com> wrote:
> >>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >>>> Impact: could probe igb
> >>>>
> >>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> >>>> failed with -2.
> >>>>
> >>>> it looks like the same behavior happened on forcedeth.
> >>>>
> >>>> try to check system_state to make sure if put it on D3
> >>>>
> >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >>>>
> >>>> ---
> >>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
> >>>>  1 file changed, 14 insertions(+), 5 deletions(-)
> >>> I see the point of the patch, but I know for a fact that ixgbe when
> >>> enabled for MSI-X also doesn't work with kexec.
> >>>
> >>> so my questions are:
> >>> are you going to change every driver?
> >> i tend to only change driver that i have related HW.
> >>
> >>> why can't this be fixed in core kernel code instead?
> >> will check it.
> >>
> >>> Shouldn't pci_enable_device take it out of D3?
> >>> Or maybe it should be taken out of D3 immediately if someone tries to
> >>> ioremap any of the BARx registers?
> >>
> >> looks like second kernel can not detect the state any more.
> > 
> > In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
> > thing.  The question is why it doesn't work as expected.
> 
> not sure... please check the version for forcedeth that you made.
> 
> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
> Author: Rafael J. Wysocki <rjw@sisk.pl>
> Date:   Fri Sep 5 14:00:19 2008 -0700
> 
>     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.

Thanks, I remember now.

This appears to be quirky hardware, doesn't it?

Rafael

Re: [PATCH] igb: fix kexec with igb

[Yinghai Lu]

Rafael J. Wysocki wrote:
> On Sunday 08 March 2009, Yinghai Lu wrote:
>> Rafael J. Wysocki wrote:
>>> On Saturday 07 March 2009, Yinghai Lu wrote:
>>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
>>>> <jesse.brandeburg@gmail.com> wrote:
>>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>>>> Impact: could probe igb
>>>>>>
>>>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
>>>>>> failed with -2.
>>>>>>
>>>>>> it looks like the same behavior happened on forcedeth.
>>>>>>
>>>>>> try to check system_state to make sure if put it on D3
>>>>>>
>>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>>>>
>>>>>> ---
>>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>>> I see the point of the patch, but I know for a fact that ixgbe when
>>>>> enabled for MSI-X also doesn't work with kexec.
>>>>>
>>>>> so my questions are:
>>>>> are you going to change every driver?
>>>> i tend to only change driver that i have related HW.
>>>>
>>>>> why can't this be fixed in core kernel code instead?
>>>> will check it.
>>>>
>>>>> Shouldn't pci_enable_device take it out of D3?
>>>>> Or maybe it should be taken out of D3 immediately if someone tries to
>>>>> ioremap any of the BARx registers?
>>>> looks like second kernel can not detect the state any more.
>>> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
>>> thing.  The question is why it doesn't work as expected.
>> not sure... please check the version for forcedeth that you made.
>>
>> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
>> Author: Rafael J. Wysocki <rjw@sisk.pl>
>> Date:   Fri Sep 5 14:00:19 2008 -0700
>>
>>     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.
> 
> Thanks, I remember now.
> 
> This appears to be quirky hardware, doesn't it?
> 
all my systems with ck804 and mcp55 need this trick.

for igb: one system need this trick, and other doesn't need this trick.
could BIOS/ACPI difference or the nic option rom difference.

but for kexec path, we really don't need put pci devices to D3.

YH

Re: [PATCH] igb: fix kexec with igb

[Rafael J. Wysocki]

On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> On Sunday 08 March 2009, Yinghai Lu wrote:
> > Rafael J. Wysocki wrote:
> > > On Saturday 07 March 2009, Yinghai Lu wrote:
> > >> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> > >> <jesse.brandeburg@gmail.com> wrote:
> > >>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > >>>> Impact: could probe igb
> > >>>>
> > >>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> > >>>> failed with -2.
> > >>>>
> > >>>> it looks like the same behavior happened on forcedeth.
> > >>>>
> > >>>> try to check system_state to make sure if put it on D3
> > >>>>
> > >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> > >>>>
> > >>>> ---
> > >>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
> > >>>>  1 file changed, 14 insertions(+), 5 deletions(-)
> > >>> I see the point of the patch, but I know for a fact that ixgbe when
> > >>> enabled for MSI-X also doesn't work with kexec.
> > >>>
> > >>> so my questions are:
> > >>> are you going to change every driver?
> > >> i tend to only change driver that i have related HW.
> > >>
> > >>> why can't this be fixed in core kernel code instead?
> > >> will check it.
> > >>
> > >>> Shouldn't pci_enable_device take it out of D3?
> > >>> Or maybe it should be taken out of D3 immediately if someone tries to
> > >>> ioremap any of the BARx registers?
> > >>
> > >> looks like second kernel can not detect the state any more.
> > > 
> > > In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
> > > thing.  The question is why it doesn't work as expected.
> > 
> > not sure... please check the version for forcedeth that you made.
> > 
> > commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
> > Author: Rafael J. Wysocki <rjw@sisk.pl>
> > Date:   Fri Sep 5 14:00:19 2008 -0700
> > 
> >     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.
> 
> Thanks, I remember now.

In which case you need to rework igb_shutdown() rather than igb_suspend().

Something like the patch below, perhaps (totally untested).

Thanks,
Rafael

---
 drivers/net/igb/igb_main.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/net/igb/igb_main.c
===================================================================
--- linux-2.6.orig/drivers/net/igb/igb_main.c
+++ linux-2.6/drivers/net/igb/igb_main.c
@@ -4299,7 +4299,7 @@ int igb_set_spd_dplx(struct igb_adapter
 }
 
 
-static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __igb_shutdown(struct pci_dev *pdev, bool enable_wake)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct igb_adapter *adapter = netdev_priv(netdev);
@@ -4359,7 +4359,7 @@ static int igb_suspend(struct pci_dev *p
 	}
 
 	/* make sure adapter isn't asleep if manageability/wol is enabled */
-	if (wufc || adapter->en_mng_pt) {
+	if ((wufc || adapter->en_mng_pt) && enable_wake) {
 		pci_enable_wake(pdev, PCI_D3hot, 1);
 		pci_enable_wake(pdev, PCI_D3cold, 1);
 	} else {
@@ -4374,12 +4374,21 @@ static int igb_suspend(struct pci_dev *p
 
 	pci_disable_device(pdev);
 
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
-
 	return 0;
 }
 
 #ifdef CONFIG_PM
+static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+	int retval;
+
+	retval = __igb_shutdown(pdev, true);
+	if (!retval)
+		pci_set_power_state(pdev, PCI_D3hot);
+
+	return retval;
+}
+
 static int igb_resume(struct pci_dev *pdev)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
@@ -4434,7 +4443,12 @@ static int igb_resume(struct pci_dev *pd
 
 static void igb_shutdown(struct pci_dev *pdev)
 {
-	igb_suspend(pdev, PMSG_SUSPEND);
+	if (system_state == SYSTEM_POWER_OFF) {
+		__igb_shutdown(pdev, true);
+		pci_set_power_state(pdev, PCI_D3hot);
+	} else {
+		__igb_shutdown(pdev, false);
+	}
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER

Re: [PATCH] igb: fix kexec with igb

[Rafael J. Wysocki]

On Sunday 08 March 2009, Yinghai Lu wrote:
> Rafael J. Wysocki wrote:
> > On Sunday 08 March 2009, Yinghai Lu wrote:
> >> Rafael J. Wysocki wrote:
> >>> On Saturday 07 March 2009, Yinghai Lu wrote:
> >>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> >>>> <jesse.brandeburg@gmail.com> wrote:
> >>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >>>>>> Impact: could probe igb
> >>>>>>
> >>>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> >>>>>> failed with -2.
> >>>>>>
> >>>>>> it looks like the same behavior happened on forcedeth.
> >>>>>>
> >>>>>> try to check system_state to make sure if put it on D3
> >>>>>>
> >>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >>>>>>
> >>>>>> ---
> >>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
> >>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
> >>>>> I see the point of the patch, but I know for a fact that ixgbe when
> >>>>> enabled for MSI-X also doesn't work with kexec.
> >>>>>
> >>>>> so my questions are:
> >>>>> are you going to change every driver?
> >>>> i tend to only change driver that i have related HW.
> >>>>
> >>>>> why can't this be fixed in core kernel code instead?
> >>>> will check it.
> >>>>
> >>>>> Shouldn't pci_enable_device take it out of D3?
> >>>>> Or maybe it should be taken out of D3 immediately if someone tries to
> >>>>> ioremap any of the BARx registers?
> >>>> looks like second kernel can not detect the state any more.
> >>> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
> >>> thing.  The question is why it doesn't work as expected.
> >> not sure... please check the version for forcedeth that you made.
> >>
> >> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
> >> Author: Rafael J. Wysocki <rjw@sisk.pl>
> >> Date:   Fri Sep 5 14:00:19 2008 -0700
> >>
> >>     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.
> > 
> > Thanks, I remember now.
> > 
> > This appears to be quirky hardware, doesn't it?
> > 
> all my systems with ck804 and mcp55 need this trick.
> 
> for igb: one system need this trick, and other doesn't need this trick.
> could BIOS/ACPI difference or the nic option rom difference.
> 
> but for kexec path, we really don't need put pci devices to D3.

Agreed, but pci_set_power_state() may be called for too many different reasons
to be a suitable place for a fix IMO.

Thanks,
Rafael

Re: [PATCH] igb: fix kexec with igb

[Yinghai Lu]

On Sun, Mar 8, 2009 at 2:32 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
>> On Sunday 08 March 2009, Yinghai Lu wrote:
>> > Rafael J. Wysocki wrote:
>> > > On Saturday 07 March 2009, Yinghai Lu wrote:
>> > >> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
>> > >> <jesse.brandeburg@gmail.com> wrote:
>> > >>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> > >>>> Impact: could probe igb
>> > >>>>
>> > >>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
>> > >>>> failed with -2.
>> > >>>>
>> > >>>> it looks like the same behavior happened on forcedeth.
>> > >>>>
>> > >>>> try to check system_state to make sure if put it on D3
>> > >>>>
>> > >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> > >>>>
>> > >>>> ---
>> > >>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>> > >>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>> > >>> I see the point of the patch, but I know for a fact that ixgbe when
>> > >>> enabled for MSI-X also doesn't work with kexec.
>> > >>>
>> > >>> so my questions are:
>> > >>> are you going to change every driver?
>> > >> i tend to only change driver that i have related HW.
>> > >>
>> > >>> why can't this be fixed in core kernel code instead?
>> > >> will check it.
>> > >>
>> > >>> Shouldn't pci_enable_device take it out of D3?
>> > >>> Or maybe it should be taken out of D3 immediately if someone tries to
>> > >>> ioremap any of the BARx registers?
>> > >>
>> > >> looks like second kernel can not detect the state any more.
>> > >
>> > > In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
>> > > thing.  The question is why it doesn't work as expected.
>> >
>> > not sure... please check the version for forcedeth that you made.
>> >
>> > commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
>> > Author: Rafael J. Wysocki <rjw@sisk.pl>
>> > Date:   Fri Sep 5 14:00:19 2008 -0700
>> >
>> >     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.
>>
>> Thanks, I remember now.
>
> In which case you need to rework igb_shutdown() rather than igb_suspend().
>
> Something like the patch below, perhaps (totally untested).
>
> Thanks,
> Rafael
>
> ---
>  drivers/net/igb/igb_main.c |   24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> Index: linux-2.6/drivers/net/igb/igb_main.c
> ===================================================================
> --- linux-2.6.orig/drivers/net/igb/igb_main.c
> +++ linux-2.6/drivers/net/igb/igb_main.c
> @@ -4299,7 +4299,7 @@ int igb_set_spd_dplx(struct igb_adapter
>  }
>
>
> -static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __igb_shutdown(struct pci_dev *pdev, bool enable_wake)
>  {
>        struct net_device *netdev = pci_get_drvdata(pdev);
>        struct igb_adapter *adapter = netdev_priv(netdev);
> @@ -4359,7 +4359,7 @@ static int igb_suspend(struct pci_dev *p
>        }
>
>        /* make sure adapter isn't asleep if manageability/wol is enabled */
> -       if (wufc || adapter->en_mng_pt) {
> +       if ((wufc || adapter->en_mng_pt) && enable_wake) {
>                pci_enable_wake(pdev, PCI_D3hot, 1);
>                pci_enable_wake(pdev, PCI_D3cold, 1);
>        } else {
> @@ -4374,12 +4374,21 @@ static int igb_suspend(struct pci_dev *p
>
>        pci_disable_device(pdev);
>
> -       pci_set_power_state(pdev, pci_choose_state(pdev, state));
> -
>        return 0;
>  }
>
>  #ifdef CONFIG_PM
> +static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> +       int retval;
> +
> +       retval = __igb_shutdown(pdev, true);
> +       if (!retval)
> +               pci_set_power_state(pdev, PCI_D3hot);
> +
> +       return retval;
> +}
> +
>  static int igb_resume(struct pci_dev *pdev)
>  {
>        struct net_device *netdev = pci_get_drvdata(pdev);
> @@ -4434,7 +4443,12 @@ static int igb_resume(struct pci_dev *pd
>
>  static void igb_shutdown(struct pci_dev *pdev)
>  {
> -       igb_suspend(pdev, PMSG_SUSPEND);
> +       if (system_state == SYSTEM_POWER_OFF) {
> +               __igb_shutdown(pdev, true);
> +               pci_set_power_state(pdev, PCI_D3hot);

you don't need to use pci_choose_state(pdev, state), but use PCI-D3hot directly?

YH

Re: [PATCH] igb: fix kexec with igb

[Rafael J. Wysocki]

On Sunday 08 March 2009, Yinghai Lu wrote:
> On Sun, Mar 8, 2009 at 2:32 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> >> On Sunday 08 March 2009, Yinghai Lu wrote:
> >> > Rafael J. Wysocki wrote:
> >> > > On Saturday 07 March 2009, Yinghai Lu wrote:
> >> > >> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> >> > >> <jesse.brandeburg@gmail.com> wrote:
> >> > >>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> > >>>> Impact: could probe igb
> >> > >>>>
> >> > >>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> >> > >>>> failed with -2.
> >> > >>>>
> >> > >>>> it looks like the same behavior happened on forcedeth.
> >> > >>>>
> >> > >>>> try to check system_state to make sure if put it on D3
> >> > >>>>
> >> > >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> > >>>>
> >> > >>>> ---
> >> > >>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
> >> > >>>>  1 file changed, 14 insertions(+), 5 deletions(-)
> >> > >>> I see the point of the patch, but I know for a fact that ixgbe when
> >> > >>> enabled for MSI-X also doesn't work with kexec.
> >> > >>>
> >> > >>> so my questions are:
> >> > >>> are you going to change every driver?
> >> > >> i tend to only change driver that i have related HW.
> >> > >>
> >> > >>> why can't this be fixed in core kernel code instead?
> >> > >> will check it.
> >> > >>
> >> > >>> Shouldn't pci_enable_device take it out of D3?
> >> > >>> Or maybe it should be taken out of D3 immediately if someone tries to
> >> > >>> ioremap any of the BARx registers?
> >> > >>
> >> > >> looks like second kernel can not detect the state any more.
> >> > >
> >> > > In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
> >> > > thing.  The question is why it doesn't work as expected.
> >> >
> >> > not sure... please check the version for forcedeth that you made.
> >> >
> >> > commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
> >> > Author: Rafael J. Wysocki <rjw@sisk.pl>
> >> > Date:   Fri Sep 5 14:00:19 2008 -0700
> >> >
> >> >     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.
> >>
> >> Thanks, I remember now.
> >
> > In which case you need to rework igb_shutdown() rather than igb_suspend().
> >
> > Something like the patch below, perhaps (totally untested).
> >
> > Thanks,
> > Rafael
> >
> > ---
> >  drivers/net/igb/igb_main.c |   24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > Index: linux-2.6/drivers/net/igb/igb_main.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/net/igb/igb_main.c
> > +++ linux-2.6/drivers/net/igb/igb_main.c
> > @@ -4299,7 +4299,7 @@ int igb_set_spd_dplx(struct igb_adapter
> >  }
> >
> >
> > -static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> > +static int __igb_shutdown(struct pci_dev *pdev, bool enable_wake)
> >  {
> >        struct net_device *netdev = pci_get_drvdata(pdev);
> >        struct igb_adapter *adapter = netdev_priv(netdev);
> > @@ -4359,7 +4359,7 @@ static int igb_suspend(struct pci_dev *p
> >        }
> >
> >        /* make sure adapter isn't asleep if manageability/wol is enabled */
> > -       if (wufc || adapter->en_mng_pt) {
> > +       if ((wufc || adapter->en_mng_pt) && enable_wake) {
> >                pci_enable_wake(pdev, PCI_D3hot, 1);
> >                pci_enable_wake(pdev, PCI_D3cold, 1);
> >        } else {
> > @@ -4374,12 +4374,21 @@ static int igb_suspend(struct pci_dev *p
> >
> >        pci_disable_device(pdev);
> >
> > -       pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > -
> >        return 0;
> >  }
> >
> >  #ifdef CONFIG_PM
> > +static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> > +{
> > +       int retval;
> > +
> > +       retval = __igb_shutdown(pdev, true);
> > +       if (!retval)
> > +               pci_set_power_state(pdev, PCI_D3hot);
> > +
> > +       return retval;
> > +}
> > +
> >  static int igb_resume(struct pci_dev *pdev)
> >  {
> >        struct net_device *netdev = pci_get_drvdata(pdev);
> > @@ -4434,7 +4443,12 @@ static int igb_resume(struct pci_dev *pd
> >
> >  static void igb_shutdown(struct pci_dev *pdev)
> >  {
> > -       igb_suspend(pdev, PMSG_SUSPEND);
> > +       if (system_state == SYSTEM_POWER_OFF) {
> > +               __igb_shutdown(pdev, true);
> > +               pci_set_power_state(pdev, PCI_D3hot);
> 
> you don't need to use pci_choose_state(pdev, state), but use PCI-D3hot directly?

Yes, because we've already decided we'd put the device into D3_hot by calling
pci_enable_wake(pdev, PCI_D3hot, 1) in __igb_shutdown().

In fact we should first determine the target state and _then_ call
pci_enable_wake() and pci_set_power_state() with that as the argument, but this
is a separate issue.  For now, IMO, it's better to use D3_hot in both places
directly.

On a slightly related note, the sequence

                pci_enable_wake(pdev, PCI_D3hot, 1);
                pci_enable_wake(pdev, PCI_D3cold, 1);

is (slightly) incorrect by itself, because ACPI doesn't distinguish bewteen
D3_hot and D3_cold, so this just causes the same platform code to run twice,
which may or may not work (it theory it should always work, but still).

This is yet another problem, though.

Thanks,
Rafael

Re: [PATCH] igb: fix kexec with igb

[Yinghai Lu]

On Sun, Mar 8, 2009 at 3:57 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday 08 March 2009, Yinghai Lu wrote:
>> On Sun, Mar 8, 2009 at 2:32 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Sunday 08 March 2009, Rafael J. Wysocki wrote:
>> >> On Sunday 08 March 2009, Yinghai Lu wrote:
>> >> > Rafael J. Wysocki wrote:
>> >> > > On Saturday 07 March 2009, Yinghai Lu wrote:
>> >> > >> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
>> >> > >> <jesse.brandeburg@gmail.com> wrote:
>> >> > >>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> >> > >>>> Impact: could probe igb
>> >> > >>>>
>> >> > >>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
>> >> > >>>> failed with -2.
>> >> > >>>>
>> >> > >>>> it looks like the same behavior happened on forcedeth.
>> >> > >>>>
>> >> > >>>> try to check system_state to make sure if put it on D3
>> >> > >>>>
>> >> > >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> >> > >>>>
>> >> > >>>> ---
>> >> > >>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>> >> > >>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>> >> > >>> I see the point of the patch, but I know for a fact that ixgbe when
>> >> > >>> enabled for MSI-X also doesn't work with kexec.
>> >> > >>>
>> >> > >>> so my questions are:
>> >> > >>> are you going to change every driver?
>> >> > >> i tend to only change driver that i have related HW.
>> >> > >>
>> >> > >>> why can't this be fixed in core kernel code instead?
>> >> > >> will check it.
>> >> > >>
>> >> > >>> Shouldn't pci_enable_device take it out of D3?
>> >> > >>> Or maybe it should be taken out of D3 immediately if someone tries to
>> >> > >>> ioremap any of the BARx registers?
>> >> > >>
>> >> > >> looks like second kernel can not detect the state any more.
>> >> > >
>> >> > > In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
>> >> > > thing.  The question is why it doesn't work as expected.
>> >> >
>> >> > not sure... please check the version for forcedeth that you made.
>> >> >
>> >> > commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
>> >> > Author: Rafael J. Wysocki <rjw@sisk.pl>
>> >> > Date:   Fri Sep 5 14:00:19 2008 -0700
>> >> >
>> >> >     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.
>> >>
>> >> Thanks, I remember now.
>> >
>> > In which case you need to rework igb_shutdown() rather than igb_suspend().
>> >
>> > Something like the patch below, perhaps (totally untested).
>> >
>> > Thanks,
>> > Rafael
>> >
>> > ---
>> >  drivers/net/igb/igb_main.c |   24 +++++++++++++++++++-----
>> >  1 file changed, 19 insertions(+), 5 deletions(-)
>> >
>> > Index: linux-2.6/drivers/net/igb/igb_main.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/net/igb/igb_main.c
>> > +++ linux-2.6/drivers/net/igb/igb_main.c
>> > @@ -4299,7 +4299,7 @@ int igb_set_spd_dplx(struct igb_adapter
>> >  }
>> >
>> >
>> > -static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
>> > +static int __igb_shutdown(struct pci_dev *pdev, bool enable_wake)
>> >  {
>> >        struct net_device *netdev = pci_get_drvdata(pdev);
>> >        struct igb_adapter *adapter = netdev_priv(netdev);
>> > @@ -4359,7 +4359,7 @@ static int igb_suspend(struct pci_dev *p
>> >        }
>> >
>> >        /* make sure adapter isn't asleep if manageability/wol is enabled */
>> > -       if (wufc || adapter->en_mng_pt) {
>> > +       if ((wufc || adapter->en_mng_pt) && enable_wake) {
>> >                pci_enable_wake(pdev, PCI_D3hot, 1);
>> >                pci_enable_wake(pdev, PCI_D3cold, 1);
>> >        } else {
>> > @@ -4374,12 +4374,21 @@ static int igb_suspend(struct pci_dev *p
>> >
>> >        pci_disable_device(pdev);
>> >
>> > -       pci_set_power_state(pdev, pci_choose_state(pdev, state));
>> > -
>> >        return 0;
>> >  }
>> >
>> >  #ifdef CONFIG_PM
>> > +static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
>> > +{
>> > +       int retval;
>> > +
>> > +       retval = __igb_shutdown(pdev, true);
>> > +       if (!retval)
>> > +               pci_set_power_state(pdev, PCI_D3hot);
>> > +
>> > +       return retval;
>> > +}
>> > +
>> >  static int igb_resume(struct pci_dev *pdev)
>> >  {
>> >        struct net_device *netdev = pci_get_drvdata(pdev);
>> > @@ -4434,7 +4443,12 @@ static int igb_resume(struct pci_dev *pd
>> >
>> >  static void igb_shutdown(struct pci_dev *pdev)
>> >  {
>> > -       igb_suspend(pdev, PMSG_SUSPEND);
>> > +       if (system_state == SYSTEM_POWER_OFF) {
>> > +               __igb_shutdown(pdev, true);
>> > +               pci_set_power_state(pdev, PCI_D3hot);
>>
>> you don't need to use pci_choose_state(pdev, state), but use PCI-D3hot directly?
>
> Yes, because we've already decided we'd put the device into D3_hot by calling
> pci_enable_wake(pdev, PCI_D3hot, 1) in __igb_shutdown().
>
> In fact we should first determine the target state and _then_ call
> pci_enable_wake() and pci_set_power_state() with that as the argument, but this
> is a separate issue.  For now, IMO, it's better to use D3_hot in both places
> directly.
>
> On a slightly related note, the sequence
>
>                pci_enable_wake(pdev, PCI_D3hot, 1);
>                pci_enable_wake(pdev, PCI_D3cold, 1);
>
> is (slightly) incorrect by itself, because ACPI doesn't distinguish bewteen
> D3_hot and D3_cold, so this just causes the same platform code to run twice,
> which may or may not work (it theory it should always work, but still).
>
> This is yet another problem, though.

can you go through e1000, e1000e, ixgb, ixgbe too?

YH

Re: [PATCH] igb: fix kexec with igb

[Rafael J. Wysocki]

On Monday 09 March 2009, Yinghai Lu wrote:
> On Sun, Mar 8, 2009 at 3:57 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday 08 March 2009, Yinghai Lu wrote:
> >> On Sun, Mar 8, 2009 at 2:32 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> >> >> On Sunday 08 March 2009, Yinghai Lu wrote:
> >> >> > Rafael J. Wysocki wrote:
> >> >> > > On Saturday 07 March 2009, Yinghai Lu wrote:
> >> >> > >> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> >> >> > >> <jesse.brandeburg@gmail.com> wrote:
> >> >> > >>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> >> > >>>> Impact: could probe igb
> >> >> > >>>>
> >> >> > >>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> >> >> > >>>> failed with -2.
> >> >> > >>>>
> >> >> > >>>> it looks like the same behavior happened on forcedeth.
> >> >> > >>>>
> >> >> > >>>> try to check system_state to make sure if put it on D3
> >> >> > >>>>
> >> >> > >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> >> > >>>>
> >> >> > >>>> ---
> >> >> > >>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
> >> >> > >>>>  1 file changed, 14 insertions(+), 5 deletions(-)
> >> >> > >>> I see the point of the patch, but I know for a fact that ixgbe when
> >> >> > >>> enabled for MSI-X also doesn't work with kexec.
> >> >> > >>>
> >> >> > >>> so my questions are:
> >> >> > >>> are you going to change every driver?
> >> >> > >> i tend to only change driver that i have related HW.
> >> >> > >>
> >> >> > >>> why can't this be fixed in core kernel code instead?
> >> >> > >> will check it.
> >> >> > >>
> >> >> > >>> Shouldn't pci_enable_device take it out of D3?
> >> >> > >>> Or maybe it should be taken out of D3 immediately if someone tries to
> >> >> > >>> ioremap any of the BARx registers?
> >> >> > >>
> >> >> > >> looks like second kernel can not detect the state any more.
> >> >> > >
> >> >> > > In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
> >> >> > > thing.  The question is why it doesn't work as expected.
> >> >> >
> >> >> > not sure... please check the version for forcedeth that you made.
> >> >> >
> >> >> > commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
> >> >> > Author: Rafael J. Wysocki <rjw@sisk.pl>
> >> >> > Date:   Fri Sep 5 14:00:19 2008 -0700
> >> >> >
> >> >> >     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.
> >> >>
> >> >> Thanks, I remember now.
> >> >
> >> > In which case you need to rework igb_shutdown() rather than igb_suspend().
> >> >
> >> > Something like the patch below, perhaps (totally untested).
> >> >
> >> > Thanks,
> >> > Rafael
> >> >
> >> > ---
> >> >  drivers/net/igb/igb_main.c |   24 +++++++++++++++++++-----
> >> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >> >
> >> > Index: linux-2.6/drivers/net/igb/igb_main.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/drivers/net/igb/igb_main.c
> >> > +++ linux-2.6/drivers/net/igb/igb_main.c
> >> > @@ -4299,7 +4299,7 @@ int igb_set_spd_dplx(struct igb_adapter
> >> >  }
> >> >
> >> >
> >> > -static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> >> > +static int __igb_shutdown(struct pci_dev *pdev, bool enable_wake)
> >> >  {
> >> >        struct net_device *netdev = pci_get_drvdata(pdev);
> >> >        struct igb_adapter *adapter = netdev_priv(netdev);
> >> > @@ -4359,7 +4359,7 @@ static int igb_suspend(struct pci_dev *p
> >> >        }
> >> >
> >> >        /* make sure adapter isn't asleep if manageability/wol is enabled */
> >> > -       if (wufc || adapter->en_mng_pt) {
> >> > +       if ((wufc || adapter->en_mng_pt) && enable_wake) {
> >> >                pci_enable_wake(pdev, PCI_D3hot, 1);
> >> >                pci_enable_wake(pdev, PCI_D3cold, 1);
> >> >        } else {
> >> > @@ -4374,12 +4374,21 @@ static int igb_suspend(struct pci_dev *p
> >> >
> >> >        pci_disable_device(pdev);
> >> >
> >> > -       pci_set_power_state(pdev, pci_choose_state(pdev, state));
> >> > -
> >> >        return 0;
> >> >  }
> >> >
> >> >  #ifdef CONFIG_PM
> >> > +static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> >> > +{
> >> > +       int retval;
> >> > +
> >> > +       retval = __igb_shutdown(pdev, true);
> >> > +       if (!retval)
> >> > +               pci_set_power_state(pdev, PCI_D3hot);
> >> > +
> >> > +       return retval;
> >> > +}
> >> > +
> >> >  static int igb_resume(struct pci_dev *pdev)
> >> >  {
> >> >        struct net_device *netdev = pci_get_drvdata(pdev);
> >> > @@ -4434,7 +4443,12 @@ static int igb_resume(struct pci_dev *pd
> >> >
> >> >  static void igb_shutdown(struct pci_dev *pdev)
> >> >  {
> >> > -       igb_suspend(pdev, PMSG_SUSPEND);
> >> > +       if (system_state == SYSTEM_POWER_OFF) {
> >> > +               __igb_shutdown(pdev, true);
> >> > +               pci_set_power_state(pdev, PCI_D3hot);
> >>
> >> you don't need to use pci_choose_state(pdev, state), but use PCI-D3hot directly?
> >
> > Yes, because we've already decided we'd put the device into D3_hot by calling
> > pci_enable_wake(pdev, PCI_D3hot, 1) in __igb_shutdown().
> >
> > In fact we should first determine the target state and _then_ call
> > pci_enable_wake() and pci_set_power_state() with that as the argument, but this
> > is a separate issue.  For now, IMO, it's better to use D3_hot in both places
> > directly.
> >
> > On a slightly related note, the sequence
> >
> >                pci_enable_wake(pdev, PCI_D3hot, 1);
> >                pci_enable_wake(pdev, PCI_D3cold, 1);
> >
> > is (slightly) incorrect by itself, because ACPI doesn't distinguish bewteen
> > D3_hot and D3_cold, so this just causes the same platform code to run twice,
> > which may or may not work (it theory it should always work, but still).
> >
> > This is yet another problem, though.
> 
> can you go through e1000, e1000e, ixgb, ixgbe too?

OK, I will, but it's going to take some time.

Thanks,
Rafael

Re: [PATCH] igb: fix kexec with igb

[Yinghai Lu]

Rafael J. Wysocki wrote:
> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
>> On Sunday 08 March 2009, Yinghai Lu wrote:
>>> Rafael J. Wysocki wrote:
>>>> On Saturday 07 March 2009, Yinghai Lu wrote:
>>>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
>>>>> <jesse.brandeburg@gmail.com> wrote:
>>>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>>>>> Impact: could probe igb
>>>>>>>
>>>>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
>>>>>>> failed with -2.
>>>>>>>
>>>>>>> it looks like the same behavior happened on forcedeth.
>>>>>>>
>>>>>>> try to check system_state to make sure if put it on D3
>>>>>>>
>>>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>>>>>
>>>>>>> ---
>>>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>>>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>>>> I see the point of the patch, but I know for a fact that ixgbe when
>>>>>> enabled for MSI-X also doesn't work with kexec.
>>>>>>
>>>>>> so my questions are:
>>>>>> are you going to change every driver?
>>>>> i tend to only change driver that i have related HW.
>>>>>
>>>>>> why can't this be fixed in core kernel code instead?
>>>>> will check it.
>>>>>
>>>>>> Shouldn't pci_enable_device take it out of D3?
>>>>>> Or maybe it should be taken out of D3 immediately if someone tries to
>>>>>> ioremap any of the BARx registers?
>>>>> looks like second kernel can not detect the state any more.
>>>> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
>>>> thing.  The question is why it doesn't work as expected.
>>> not sure... please check the version for forcedeth that you made.
>>>
>>> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
>>> Author: Rafael J. Wysocki <rjw@sisk.pl>
>>> Date:   Fri Sep 5 14:00:19 2008 -0700
>>>
>>>     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.
>> Thanks, I remember now.
> 
> In which case you need to rework igb_shutdown() rather than igb_suspend().
> 
> Something like the patch below, perhaps (totally untested).

it works, David, can you picked it up

---

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

[PATCH] igb: fix kexec with igb -v2

Impact: could probe igb

Found one system with 82575EB, in the kernel that is kexeced, probe igb
failed with -2.

it looks like the same behavior happened on forcedeth.

try to check system_state to make sure if put it on D3

v2: Rafael J. Wysocki seperate igb_shutdown and igb_suspend code

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
drivers/net/igb/igb_main.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/net/igb/igb_main.c
===================================================================
--- linux-2.6.orig/drivers/net/igb/igb_main.c
+++ linux-2.6/drivers/net/igb/igb_main.c
@@ -4299,7 +4299,7 @@ int igb_set_spd_dplx(struct igb_adapter
 }
 
 
-static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __igb_shutdown(struct pci_dev *pdev, bool enable_wake)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct igb_adapter *adapter = netdev_priv(netdev);
@@ -4359,7 +4359,7 @@ static int igb_suspend(struct pci_dev *p
 	}
 
 	/* make sure adapter isn't asleep if manageability/wol is enabled */
-	if (wufc || adapter->en_mng_pt) {
+	if ((wufc || adapter->en_mng_pt) && enable_wake) {
 		pci_enable_wake(pdev, PCI_D3hot, 1);
 		pci_enable_wake(pdev, PCI_D3cold, 1);
 	} else {
@@ -4374,12 +4374,21 @@ static int igb_suspend(struct pci_dev *p
 
 	pci_disable_device(pdev);
 
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
-
 	return 0;
 }
 
 #ifdef CONFIG_PM
+static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+	int retval;
+
+	retval = __igb_shutdown(pdev, true);
+	if (!retval)
+		pci_set_power_state(pdev, PCI_D3hot);
+
+	return retval;
+}
+
 static int igb_resume(struct pci_dev *pdev)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
@@ -4434,7 +4443,12 @@ static int igb_resume(struct pci_dev *pd
 
 static void igb_shutdown(struct pci_dev *pdev)
 {
-	igb_suspend(pdev, PMSG_SUSPEND);
+	if (system_state == SYSTEM_POWER_OFF) {
+		__igb_shutdown(pdev, true);
+		pci_set_power_state(pdev, PCI_D3hot);
+	} else {
+		__igb_shutdown(pdev, false);
+	}
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER

Re: [PATCH] pci: fix kexec with power state D3

[Jesse Barnes]

> > And, quite frankly, I'm not sure if users will be happy with 
> > the $subject patch, because it _really_ breaks things (well, 
> > the kexec users who don't use suspend might be, but surely 
> > suspend users who don't use kexec won't).
> 
> Please note that i havent reviewed the patches and i did not 
> take any sides in the discussion - i just flagged the maintainer 
> ping-pong. As long as we pick one of the patches (or a third 
> one) within a bound amount of time we should be fine :)

I'll defer to Rafael here; he's been working the most in this area.
The changelog wasn't very complete for the original patch, but it
sounds like in the kexec case the newly booted kernel will get an igb
device in D3 which it can't handle?  That really does sound like a
driver bug, not something we should mess with in the core.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] pci: fix kexec with power state D3

[Jesse Barnes]

> > And, quite frankly, I'm not sure if users will be happy with 
> > the $subject patch, because it _really_ breaks things (well, 
> > the kexec users who don't use suspend might be, but surely 
> > suspend users who don't use kexec won't).
> 
> Please note that i havent reviewed the patches and i did not 
> take any sides in the discussion - i just flagged the maintainer 
> ping-pong. As long as we pick one of the patches (or a third 
> one) within a bound amount of time we should be fine :)

I'll defer to Rafael here; he's been working the most in this area.
The changelog wasn't very complete for the original patch, but it
sounds like in the kexec case the newly booted kernel will get an igb
device in D3 which it can't handle?  That really does sound like a
driver bug, not something we should mess with in the core.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] pci: fix kexec with power state D3

[Rafael J. Wysocki]

On Friday 20 March 2009, Jesse Barnes wrote:
> > > And, quite frankly, I'm not sure if users will be happy with 
> > > the $subject patch, because it _really_ breaks things (well, 
> > > the kexec users who don't use suspend might be, but surely 
> > > suspend users who don't use kexec won't).
> > 
> > Please note that i havent reviewed the patches and i did not 
> > take any sides in the discussion - i just flagged the maintainer 
> > ping-pong. As long as we pick one of the patches (or a third 
> > one) within a bound amount of time we should be fine :)
> 
> I'll defer to Rafael here; he's been working the most in this area.
> The changelog wasn't very complete for the original patch, but it
> sounds like in the kexec case the newly booted kernel will get an igb
> device in D3 which it can't handle?  That really does sound like a
> driver bug, not something we should mess with in the core.

I have already posted an alternative patch for igb that's been reported to
fix the problem.

Thanks,
Rafael

Re: [PATCH] pci: fix kexec with power state D3

[Rafael J. Wysocki]

On Friday 20 March 2009, Jesse Barnes wrote:
> > > And, quite frankly, I'm not sure if users will be happy with 
> > > the $subject patch, because it _really_ breaks things (well, 
> > > the kexec users who don't use suspend might be, but surely 
> > > suspend users who don't use kexec won't).
> > 
> > Please note that i havent reviewed the patches and i did not 
> > take any sides in the discussion - i just flagged the maintainer 
> > ping-pong. As long as we pick one of the patches (or a third 
> > one) within a bound amount of time we should be fine :)
> 
> I'll defer to Rafael here; he's been working the most in this area.
> The changelog wasn't very complete for the original patch, but it
> sounds like in the kexec case the newly booted kernel will get an igb
> device in D3 which it can't handle?  That really does sound like a
> driver bug, not something we should mess with in the core.

I have already posted an alternative patch for igb that's been reported to
fix the problem.

Thanks,
Rafael

[Updated patch] Re: [PATCH] igb: fix kexec with igb

[Rafael J. Wysocki]

On Thursday 12 March 2009, Yinghai Lu wrote:
> Rafael J. Wysocki wrote:
> > On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> >> On Sunday 08 March 2009, Yinghai Lu wrote:
> >>> Rafael J. Wysocki wrote:
> >>>> On Saturday 07 March 2009, Yinghai Lu wrote:
> >>>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> >>>>> <jesse.brandeburg@gmail.com> wrote:
> >>>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >>>>>>> Impact: could probe igb
> >>>>>>>
> >>>>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> >>>>>>> failed with -2.
> >>>>>>>
> >>>>>>> it looks like the same behavior happened on forcedeth.
> >>>>>>>
> >>>>>>> try to check system_state to make sure if put it on D3
> >>>>>>>
> >>>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >>>>>>>
> >>>>>>> ---
> >>>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
> >>>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
> >>>>>> I see the point of the patch, but I know for a fact that ixgbe when
> >>>>>> enabled for MSI-X also doesn't work with kexec.
> >>>>>>
> >>>>>> so my questions are:
> >>>>>> are you going to change every driver?
> >>>>> i tend to only change driver that i have related HW.
> >>>>>
> >>>>>> why can't this be fixed in core kernel code instead?
> >>>>> will check it.
> >>>>>
> >>>>>> Shouldn't pci_enable_device take it out of D3?
> >>>>>> Or maybe it should be taken out of D3 immediately if someone tries to
> >>>>>> ioremap any of the BARx registers?
> >>>>> looks like second kernel can not detect the state any more.
> >>>> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
> >>>> thing.  The question is why it doesn't work as expected.
> >>> not sure... please check the version for forcedeth that you made.
> >>>
> >>> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
> >>> Author: Rafael J. Wysocki <rjw@sisk.pl>
> >>> Date:   Fri Sep 5 14:00:19 2008 -0700
> >>>
> >>>     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.
> >> Thanks, I remember now.
> > 
> > In which case you need to rework igb_shutdown() rather than igb_suspend().
> > 
> > Something like the patch below, perhaps (totally untested).
> 
> it works, David, can you picked it up

Still, Yinghai, can you please also test the patch below?  It fixes all
shortcomings in the driver's suspend and shutdown methods I was talking about
in one of the previous messages.  If it works, IMO it will be a preferable fix
(in particular, it would be good to check if WoL still works with it,
but I don't have the hardware).

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: net/igb: Fix kexec with igb (rev. 3)
Impact: Fix

Yinghai Lu found one system with 82575EB where, in the kernel that is
kexeced, probe igb failed with -2, the reason being that the adapter
could not be brought back from D3 by the kexec kernel, most probably
due to quirky hardware (it looks like the same behavior happened on
forcedeth).

Prevent igb from putting the adapter into D3 during shutdown except
when we going to power off the system.  For this purpose, seperate
igb_shutdown() from igb_suspend() and use the appropriate PCI PM
callbacks in both of them.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Reported-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/net/igb/igb_main.c |   42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

Index: linux-2.6/drivers/net/igb/igb_main.c
===================================================================
--- linux-2.6.orig/drivers/net/igb/igb_main.c
+++ linux-2.6/drivers/net/igb/igb_main.c
@@ -4277,7 +4277,7 @@ int igb_set_spd_dplx(struct igb_adapter 
 }
 
 
-static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct igb_adapter *adapter = netdev_priv(netdev);
@@ -4336,15 +4336,9 @@ static int igb_suspend(struct pci_dev *p
 		wr32(E1000_WUFC, 0);
 	}
 
-	/* make sure adapter isn't asleep if manageability/wol is enabled */
-	if (wufc || adapter->en_mng_pt) {
-		pci_enable_wake(pdev, PCI_D3hot, 1);
-		pci_enable_wake(pdev, PCI_D3cold, 1);
-	} else {
+	*enable_wake = wufc || adapter->en_mng_pt;
+	if (!*enable_wake)
 		igb_shutdown_fiber_serdes_link_82575(hw);
-		pci_enable_wake(pdev, PCI_D3hot, 0);
-		pci_enable_wake(pdev, PCI_D3cold, 0);
-	}
 
 	/* Release control of h/w to f/w.  If f/w is AMT enabled, this
 	 * would have already happened in close and is redundant. */
@@ -4352,12 +4346,29 @@ static int igb_suspend(struct pci_dev *p
 
 	pci_disable_device(pdev);
 
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
-
 	return 0;
 }
 
 #ifdef CONFIG_PM
+static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+	int retval;
+	bool wake;
+
+	retval = __igb_shutdown(pdev, &wake);
+	if (retval)
+		return retval;
+
+	if (wake) {
+		pci_prepare_to_sleep(pdev);
+	} else {
+		pci_wake_from_d3(pdev, false);
+		pci_set_power_state(pdev, PCI_D3hot);
+	}
+
+	return 0;
+}
+
 static int igb_resume(struct pci_dev *pdev)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
@@ -4412,7 +4423,14 @@ static int igb_resume(struct pci_dev *pd
 
 static void igb_shutdown(struct pci_dev *pdev)
 {
-	igb_suspend(pdev, PMSG_SUSPEND);
+	bool wake;
+
+	__igb_shutdown(pdev, &wake);
+
+	if (system_state == SYSTEM_POWER_OFF) {
+		pci_wake_from_d3(pdev, wake);
+		pci_set_power_state(pdev, PCI_D3hot);
+	}
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER

Re: [Updated patch] Re: [PATCH] igb: fix kexec with igb

[Yinghai Lu]

Rafael J. Wysocki wrote:
> On Thursday 12 March 2009, Yinghai Lu wrote:
>> Rafael J. Wysocki wrote:
>>> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
>>>> On Sunday 08 March 2009, Yinghai Lu wrote:
>>>>> Rafael J. Wysocki wrote:
>>>>>> On Saturday 07 March 2009, Yinghai Lu wrote:
>>>>>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
>>>>>>> <jesse.brandeburg@gmail.com> wrote:
>>>>>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>>>>>>>> Impact: could probe igb
>>>>>>>>>
>>>>>>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
>>>>>>>>> failed with -2.
>>>>>>>>>
>>>>>>>>> it looks like the same behavior happened on forcedeth.
>>>>>>>>>
>>>>>>>>> try to check system_state to make sure if put it on D3
>>>>>>>>>
>>>>>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>>>>>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>>>>>> I see the point of the patch, but I know for a fact that ixgbe when
>>>>>>>> enabled for MSI-X also doesn't work with kexec.
>>>>>>>>
>>>>>>>> so my questions are:
>>>>>>>> are you going to change every driver?
>>>>>>> i tend to only change driver that i have related HW.
>>>>>>>
>>>>>>>> why can't this be fixed in core kernel code instead?
>>>>>>> will check it.
>>>>>>>
>>>>>>>> Shouldn't pci_enable_device take it out of D3?
>>>>>>>> Or maybe it should be taken out of D3 immediately if someone tries to
>>>>>>>> ioremap any of the BARx registers?
>>>>>>> looks like second kernel can not detect the state any more.
>>>>>> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
>>>>>> thing.  The question is why it doesn't work as expected.
>>>>> not sure... please check the version for forcedeth that you made.
>>>>>
>>>>> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
>>>>> Author: Rafael J. Wysocki <rjw@sisk.pl>
>>>>> Date:   Fri Sep 5 14:00:19 2008 -0700
>>>>>
>>>>>     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.
>>>> Thanks, I remember now.
>>> In which case you need to rework igb_shutdown() rather than igb_suspend().
>>>
>>> Something like the patch below, perhaps (totally untested).
>> it works, David, can you picked it up
> 
> Still, Yinghai, can you please also test the patch below?  It fixes all
> shortcomings in the driver's suspend and shutdown methods I was talking about
> in one of the previous messages.  If it works, IMO it will be a preferable fix
> (in particular, it would be good to check if WoL still works with it,
> but I don't have the hardware).
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: net/igb: Fix kexec with igb (rev. 3)
> Impact: Fix
> 
> Yinghai Lu found one system with 82575EB where, in the kernel that is
> kexeced, probe igb failed with -2, the reason being that the adapter
> could not be brought back from D3 by the kexec kernel, most probably
> due to quirky hardware (it looks like the same behavior happened on
> forcedeth).
> 
> Prevent igb from putting the adapter into D3 during shutdown except
> when we going to power off the system.  For this purpose, seperate
> igb_shutdown() from igb_suspend() and use the appropriate PCI PM
> callbacks in both of them.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> Reported-by: Yinghai Lu <yinghai@kernel.org>
> ---
>  drivers/net/igb/igb_main.c |   42 ++++++++++++++++++++++++++++++------------
>  1 file changed, 30 insertions(+), 12 deletions(-)
> 
> Index: linux-2.6/drivers/net/igb/igb_main.c
> ===================================================================
> --- linux-2.6.orig/drivers/net/igb/igb_main.c
> +++ linux-2.6/drivers/net/igb/igb_main.c
> @@ -4277,7 +4277,7 @@ int igb_set_spd_dplx(struct igb_adapter 
>  }
>  
>  
> -static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake)
>  {
>  	struct net_device *netdev = pci_get_drvdata(pdev);
>  	struct igb_adapter *adapter = netdev_priv(netdev);
> @@ -4336,15 +4336,9 @@ static int igb_suspend(struct pci_dev *p
>  		wr32(E1000_WUFC, 0);
>  	}
>  
> -	/* make sure adapter isn't asleep if manageability/wol is enabled */
> -	if (wufc || adapter->en_mng_pt) {
> -		pci_enable_wake(pdev, PCI_D3hot, 1);
> -		pci_enable_wake(pdev, PCI_D3cold, 1);
> -	} else {
> +	*enable_wake = wufc || adapter->en_mng_pt;
> +	if (!*enable_wake)
>  		igb_shutdown_fiber_serdes_link_82575(hw);
> -		pci_enable_wake(pdev, PCI_D3hot, 0);
> -		pci_enable_wake(pdev, PCI_D3cold, 0);
> -	}
>  
>  	/* Release control of h/w to f/w.  If f/w is AMT enabled, this
>  	 * would have already happened in close and is redundant. */
> @@ -4352,12 +4346,29 @@ static int igb_suspend(struct pci_dev *p
>  
>  	pci_disable_device(pdev);
>  
> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> -
>  	return 0;
>  }
>  
>  #ifdef CONFIG_PM
> +static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> +	int retval;
> +	bool wake;
> +
> +	retval = __igb_shutdown(pdev, &wake);
> +	if (retval)
> +		return retval;
> +
> +	if (wake) {
> +		pci_prepare_to_sleep(pdev);
> +	} else {
> +		pci_wake_from_d3(pdev, false);
> +		pci_set_power_state(pdev, PCI_D3hot);
> +	}
> +
> +	return 0;
> +}
> +
>  static int igb_resume(struct pci_dev *pdev)
>  {
>  	struct net_device *netdev = pci_get_drvdata(pdev);
> @@ -4412,7 +4423,14 @@ static int igb_resume(struct pci_dev *pd
>  
>  static void igb_shutdown(struct pci_dev *pdev)
>  {
> -	igb_suspend(pdev, PMSG_SUSPEND);
> +	bool wake;
> +
> +	__igb_shutdown(pdev, &wake);
> +
> +	if (system_state == SYSTEM_POWER_OFF) {
> +		pci_wake_from_d3(pdev, wake);
> +		pci_set_power_state(pdev, PCI_D3hot);
> +	}
>  }
>  
>  #ifdef CONFIG_NET_POLL_CONTROLLER

it works too.

YH

Re: [Updated patch] Re: [PATCH] igb: fix kexec with igb

[Rafael J. Wysocki]

On Tuesday 24 March 2009, Yinghai Lu wrote:
> Rafael J. Wysocki wrote:
> > On Thursday 12 March 2009, Yinghai Lu wrote:
> >> Rafael J. Wysocki wrote:
> >>> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> >>>> On Sunday 08 March 2009, Yinghai Lu wrote:
> >>>>> Rafael J. Wysocki wrote:
> >>>>>> On Saturday 07 March 2009, Yinghai Lu wrote:
> >>>>>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> >>>>>>> <jesse.brandeburg@gmail.com> wrote:
> >>>>>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >>>>>>>>> Impact: could probe igb
> >>>>>>>>>
> >>>>>>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> >>>>>>>>> failed with -2.
> >>>>>>>>>
> >>>>>>>>> it looks like the same behavior happened on forcedeth.
> >>>>>>>>>
> >>>>>>>>> try to check system_state to make sure if put it on D3
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >>>>>>>>>
> >>>>>>>>> ---
> >>>>>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
> >>>>>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
> >>>>>>>> I see the point of the patch, but I know for a fact that ixgbe when
> >>>>>>>> enabled for MSI-X also doesn't work with kexec.
> >>>>>>>>
> >>>>>>>> so my questions are:
> >>>>>>>> are you going to change every driver?
> >>>>>>> i tend to only change driver that i have related HW.
> >>>>>>>
> >>>>>>>> why can't this be fixed in core kernel code instead?
> >>>>>>> will check it.
> >>>>>>>
> >>>>>>>> Shouldn't pci_enable_device take it out of D3?
> >>>>>>>> Or maybe it should be taken out of D3 immediately if someone tries to
> >>>>>>>> ioremap any of the BARx registers?
> >>>>>>> looks like second kernel can not detect the state any more.
> >>>>>> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
> >>>>>> thing.  The question is why it doesn't work as expected.
> >>>>> not sure... please check the version for forcedeth that you made.
> >>>>>
> >>>>> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
> >>>>> Author: Rafael J. Wysocki <rjw@sisk.pl>
> >>>>> Date:   Fri Sep 5 14:00:19 2008 -0700
> >>>>>
> >>>>>     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.
> >>>> Thanks, I remember now.
> >>> In which case you need to rework igb_shutdown() rather than igb_suspend().
> >>>
> >>> Something like the patch below, perhaps (totally untested).
> >> it works, David, can you picked it up
> > 
> > Still, Yinghai, can you please also test the patch below?  It fixes all
> > shortcomings in the driver's suspend and shutdown methods I was talking about
> > in one of the previous messages.  If it works, IMO it will be a preferable fix
> > (in particular, it would be good to check if WoL still works with it,
> > but I don't have the hardware).
> > 
> > Thanks,
> > Rafael
> > 
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: net/igb: Fix kexec with igb (rev. 3)
> > Impact: Fix
> > 
> > Yinghai Lu found one system with 82575EB where, in the kernel that is
> > kexeced, probe igb failed with -2, the reason being that the adapter
> > could not be brought back from D3 by the kexec kernel, most probably
> > due to quirky hardware (it looks like the same behavior happened on
> > forcedeth).
> > 
> > Prevent igb from putting the adapter into D3 during shutdown except
> > when we going to power off the system.  For this purpose, seperate
> > igb_shutdown() from igb_suspend() and use the appropriate PCI PM
> > callbacks in both of them.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > Reported-by: Yinghai Lu <yinghai@kernel.org>
> > ---
> >  drivers/net/igb/igb_main.c |   42 ++++++++++++++++++++++++++++++------------
> >  1 file changed, 30 insertions(+), 12 deletions(-)
> > 
> > Index: linux-2.6/drivers/net/igb/igb_main.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/net/igb/igb_main.c
> > +++ linux-2.6/drivers/net/igb/igb_main.c
> > @@ -4277,7 +4277,7 @@ int igb_set_spd_dplx(struct igb_adapter 
> >  }
> >  
> >  
> > -static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> > +static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake)
> >  {
> >  	struct net_device *netdev = pci_get_drvdata(pdev);
> >  	struct igb_adapter *adapter = netdev_priv(netdev);
> > @@ -4336,15 +4336,9 @@ static int igb_suspend(struct pci_dev *p
> >  		wr32(E1000_WUFC, 0);
> >  	}
> >  
> > -	/* make sure adapter isn't asleep if manageability/wol is enabled */
> > -	if (wufc || adapter->en_mng_pt) {
> > -		pci_enable_wake(pdev, PCI_D3hot, 1);
> > -		pci_enable_wake(pdev, PCI_D3cold, 1);
> > -	} else {
> > +	*enable_wake = wufc || adapter->en_mng_pt;
> > +	if (!*enable_wake)
> >  		igb_shutdown_fiber_serdes_link_82575(hw);
> > -		pci_enable_wake(pdev, PCI_D3hot, 0);
> > -		pci_enable_wake(pdev, PCI_D3cold, 0);
> > -	}
> >  
> >  	/* Release control of h/w to f/w.  If f/w is AMT enabled, this
> >  	 * would have already happened in close and is redundant. */
> > @@ -4352,12 +4346,29 @@ static int igb_suspend(struct pci_dev *p
> >  
> >  	pci_disable_device(pdev);
> >  
> > -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> > -
> >  	return 0;
> >  }
> >  
> >  #ifdef CONFIG_PM
> > +static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> > +{
> > +	int retval;
> > +	bool wake;
> > +
> > +	retval = __igb_shutdown(pdev, &wake);
> > +	if (retval)
> > +		return retval;
> > +
> > +	if (wake) {
> > +		pci_prepare_to_sleep(pdev);
> > +	} else {
> > +		pci_wake_from_d3(pdev, false);
> > +		pci_set_power_state(pdev, PCI_D3hot);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int igb_resume(struct pci_dev *pdev)
> >  {
> >  	struct net_device *netdev = pci_get_drvdata(pdev);
> > @@ -4412,7 +4423,14 @@ static int igb_resume(struct pci_dev *pd
> >  
> >  static void igb_shutdown(struct pci_dev *pdev)
> >  {
> > -	igb_suspend(pdev, PMSG_SUSPEND);
> > +	bool wake;
> > +
> > +	__igb_shutdown(pdev, &wake);
> > +
> > +	if (system_state == SYSTEM_POWER_OFF) {
> > +		pci_wake_from_d3(pdev, wake);
> > +		pci_set_power_state(pdev, PCI_D3hot);
> > +	}
> >  }
> >  
> >  #ifdef CONFIG_NET_POLL_CONTROLLER
> 
> it works too.

Great, thanks for testing.

Jeff, Jesse, is the patch fine with you?

Rafael

Re: [Updated patch] Re: [PATCH] igb: fix kexec with igb

[Jeff Kirsher]

On Sat, Mar 28, 2009 at 2:27 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Tuesday 24 March 2009, Yinghai Lu wrote:
>> Rafael J. Wysocki wrote:
>> > On Thursday 12 March 2009, Yinghai Lu wrote:
>> >> Rafael J. Wysocki wrote:
>> >>> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
>> >>>> On Sunday 08 March 2009, Yinghai Lu wrote:
>> >>>>> Rafael J. Wysocki wrote:
>> >>>>>> On Saturday 07 March 2009, Yinghai Lu wrote:
>> >>>>>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
>> >>>>>>> <jesse.brandeburg@gmail.com> wrote:
>> >>>>>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> >>>>>>>>> Impact: could probe igb
>> >>>>>>>>>
>> >>>>>>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
>> >>>>>>>>> failed with -2.
>> >>>>>>>>>
>> >>>>>>>>> it looks like the same behavior happened on forcedeth.
>> >>>>>>>>>
>> >>>>>>>>> try to check system_state to make sure if put it on D3
>> >>>>>>>>>
>> >>>>>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> >>>>>>>>>
>> >>>>>>>>> ---
>> >>>>>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>> >>>>>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>> >>>>>>>> I see the point of the patch, but I know for a fact that ixgbe when
>> >>>>>>>> enabled for MSI-X also doesn't work with kexec.
>> >>>>>>>>
>> >>>>>>>> so my questions are:
>> >>>>>>>> are you going to change every driver?
>> >>>>>>> i tend to only change driver that i have related HW.
>> >>>>>>>
>> >>>>>>>> why can't this be fixed in core kernel code instead?
>> >>>>>>> will check it.
>> >>>>>>>
>> >>>>>>>> Shouldn't pci_enable_device take it out of D3?
>> >>>>>>>> Or maybe it should be taken out of D3 immediately if someone tries to
>> >>>>>>>> ioremap any of the BARx registers?
>> >>>>>>> looks like second kernel can not detect the state any more.
>> >>>>>> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
>> >>>>>> thing.  The question is why it doesn't work as expected.
>> >>>>> not sure... please check the version for forcedeth that you made.
>> >>>>>
>> >>>>> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
>> >>>>> Author: Rafael J. Wysocki <rjw@sisk.pl>
>> >>>>> Date:   Fri Sep 5 14:00:19 2008 -0700
>> >>>>>
>> >>>>>     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.
>> >>>> Thanks, I remember now.
>> >>> In which case you need to rework igb_shutdown() rather than igb_suspend().
>> >>>
>> >>> Something like the patch below, perhaps (totally untested).
>> >> it works, David, can you picked it up
>> >
>> > Still, Yinghai, can you please also test the patch below?  It fixes all
>> > shortcomings in the driver's suspend and shutdown methods I was talking about
>> > in one of the previous messages.  If it works, IMO it will be a preferable fix
>> > (in particular, it would be good to check if WoL still works with it,
>> > but I don't have the hardware).
>> >
>> > Thanks,
>> > Rafael
>> >
>> > ---
>> > From: Rafael J. Wysocki <rjw@sisk.pl>
>> > Subject: net/igb: Fix kexec with igb (rev. 3)
>> > Impact: Fix
>> >
>> > Yinghai Lu found one system with 82575EB where, in the kernel that is
>> > kexeced, probe igb failed with -2, the reason being that the adapter
>> > could not be brought back from D3 by the kexec kernel, most probably
>> > due to quirky hardware (it looks like the same behavior happened on
>> > forcedeth).
>> >
>> > Prevent igb from putting the adapter into D3 during shutdown except
>> > when we going to power off the system.  For this purpose, seperate
>> > igb_shutdown() from igb_suspend() and use the appropriate PCI PM
>> > callbacks in both of them.
>> >
>> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>> > Reported-by: Yinghai Lu <yinghai@kernel.org>
>> > ---
>> >  drivers/net/igb/igb_main.c |   42 ++++++++++++++++++++++++++++++------------
>> >  1 file changed, 30 insertions(+), 12 deletions(-)
>> >
>> > Index: linux-2.6/drivers/net/igb/igb_main.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/net/igb/igb_main.c
>> > +++ linux-2.6/drivers/net/igb/igb_main.c
>> > @@ -4277,7 +4277,7 @@ int igb_set_spd_dplx(struct igb_adapter
>> >  }
>> >
>> >
>> > -static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
>> > +static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake)
>> >  {
>> >     struct net_device *netdev = pci_get_drvdata(pdev);
>> >     struct igb_adapter *adapter = netdev_priv(netdev);
>> > @@ -4336,15 +4336,9 @@ static int igb_suspend(struct pci_dev *p
>> >             wr32(E1000_WUFC, 0);
>> >     }
>> >
>> > -   /* make sure adapter isn't asleep if manageability/wol is enabled */
>> > -   if (wufc || adapter->en_mng_pt) {
>> > -           pci_enable_wake(pdev, PCI_D3hot, 1);
>> > -           pci_enable_wake(pdev, PCI_D3cold, 1);
>> > -   } else {
>> > +   *enable_wake = wufc || adapter->en_mng_pt;
>> > +   if (!*enable_wake)
>> >             igb_shutdown_fiber_serdes_link_82575(hw);
>> > -           pci_enable_wake(pdev, PCI_D3hot, 0);
>> > -           pci_enable_wake(pdev, PCI_D3cold, 0);
>> > -   }
>> >
>> >     /* Release control of h/w to f/w.  If f/w is AMT enabled, this
>> >      * would have already happened in close and is redundant. */
>> > @@ -4352,12 +4346,29 @@ static int igb_suspend(struct pci_dev *p
>> >
>> >     pci_disable_device(pdev);
>> >
>> > -   pci_set_power_state(pdev, pci_choose_state(pdev, state));
>> > -
>> >     return 0;
>> >  }
>> >
>> >  #ifdef CONFIG_PM
>> > +static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
>> > +{
>> > +   int retval;
>> > +   bool wake;
>> > +
>> > +   retval = __igb_shutdown(pdev, &wake);
>> > +   if (retval)
>> > +           return retval;
>> > +
>> > +   if (wake) {
>> > +           pci_prepare_to_sleep(pdev);
>> > +   } else {
>> > +           pci_wake_from_d3(pdev, false);
>> > +           pci_set_power_state(pdev, PCI_D3hot);
>> > +   }
>> > +
>> > +   return 0;
>> > +}
>> > +
>> >  static int igb_resume(struct pci_dev *pdev)
>> >  {
>> >     struct net_device *netdev = pci_get_drvdata(pdev);
>> > @@ -4412,7 +4423,14 @@ static int igb_resume(struct pci_dev *pd
>> >
>> >  static void igb_shutdown(struct pci_dev *pdev)
>> >  {
>> > -   igb_suspend(pdev, PMSG_SUSPEND);
>> > +   bool wake;
>> > +
>> > +   __igb_shutdown(pdev, &wake);
>> > +
>> > +   if (system_state == SYSTEM_POWER_OFF) {
>> > +           pci_wake_from_d3(pdev, wake);
>> > +           pci_set_power_state(pdev, PCI_D3hot);
>> > +   }
>> >  }
>> >
>> >  #ifdef CONFIG_NET_POLL_CONTROLLER
>>
>> it works too.
>
> Great, thanks for testing.
>
> Jeff, Jesse, is the patch fine with you?
>
> Rafael

Let me talk with Jesse on Monday about it.  I know that Jesse's
concern's were that there were more than one driver afflicted with the
same or similar issue and that it made more sense to fix this in the
core for all drivers.  For instance ixgbe, IIRC, but because Yinghai
did not have any other hardware, it was unclear if this was an issue
on other drivers.

After discussing this with Jesse, one of us will respond.  I will
apply this patch to my tree anyway and if we are alright with the
patch, I will push it out with my other patches to Dave.

-- 
Cheers,
Jeff

Re: [Updated patch] Re: [PATCH] igb: fix kexec with igb

[Rafael J. Wysocki]

On Sunday 29 March 2009, Jeff Kirsher wrote:
> On Sat, Mar 28, 2009 at 2:27 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Tuesday 24 March 2009, Yinghai Lu wrote:
> >> Rafael J. Wysocki wrote:
> >> > On Thursday 12 March 2009, Yinghai Lu wrote:
> >> >> Rafael J. Wysocki wrote:
> >> >>> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> >> >>>> On Sunday 08 March 2009, Yinghai Lu wrote:
> >> >>>>> Rafael J. Wysocki wrote:
> >> >>>>>> On Saturday 07 March 2009, Yinghai Lu wrote:
> >> >>>>>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> >> >>>>>>> <jesse.brandeburg@gmail.com> wrote:
> >> >>>>>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> >>>>>>>>> Impact: could probe igb
> >> >>>>>>>>>
> >> >>>>>>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> >> >>>>>>>>> failed with -2.
> >> >>>>>>>>>
> >> >>>>>>>>> it looks like the same behavior happened on forcedeth.
> >> >>>>>>>>>
> >> >>>>>>>>> try to check system_state to make sure if put it on D3
> >> >>>>>>>>>
> >> >>>>>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> >>>>>>>>>
> >> >>>>>>>>> ---
> >> >>>>>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
> >> >>>>>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
> >> >>>>>>>> I see the point of the patch, but I know for a fact that ixgbe when
> >> >>>>>>>> enabled for MSI-X also doesn't work with kexec.
> >> >>>>>>>>
> >> >>>>>>>> so my questions are:
> >> >>>>>>>> are you going to change every driver?
> >> >>>>>>> i tend to only change driver that i have related HW.
> >> >>>>>>>
> >> >>>>>>>> why can't this be fixed in core kernel code instead?
> >> >>>>>>> will check it.
> >> >>>>>>>
> >> >>>>>>>> Shouldn't pci_enable_device take it out of D3?
> >> >>>>>>>> Or maybe it should be taken out of D3 immediately if someone tries to
> >> >>>>>>>> ioremap any of the BARx registers?
> >> >>>>>>> looks like second kernel can not detect the state any more.
> >> >>>>>> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
> >> >>>>>> thing.  The question is why it doesn't work as expected.
> >> >>>>> not sure... please check the version for forcedeth that you made.
> >> >>>>>
> >> >>>>> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
> >> >>>>> Author: Rafael J. Wysocki <rjw@sisk.pl>
> >> >>>>> Date:   Fri Sep 5 14:00:19 2008 -0700
> >> >>>>>
> >> >>>>>     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.
> >> >>>> Thanks, I remember now.
> >> >>> In which case you need to rework igb_shutdown() rather than igb_suspend().
> >> >>>
> >> >>> Something like the patch below, perhaps (totally untested).
> >> >> it works, David, can you picked it up
> >> >
> >> > Still, Yinghai, can you please also test the patch below?  It fixes all
> >> > shortcomings in the driver's suspend and shutdown methods I was talking about
> >> > in one of the previous messages.  If it works, IMO it will be a preferable fix
> >> > (in particular, it would be good to check if WoL still works with it,
> >> > but I don't have the hardware).
> >> >
> >> > Thanks,
> >> > Rafael
> >> >
> >> > ---
> >> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >> > Subject: net/igb: Fix kexec with igb (rev. 3)
> >> > Impact: Fix
> >> >
> >> > Yinghai Lu found one system with 82575EB where, in the kernel that is
> >> > kexeced, probe igb failed with -2, the reason being that the adapter
> >> > could not be brought back from D3 by the kexec kernel, most probably
> >> > due to quirky hardware (it looks like the same behavior happened on
> >> > forcedeth).
> >> >
> >> > Prevent igb from putting the adapter into D3 during shutdown except
> >> > when we going to power off the system.  For this purpose, seperate
> >> > igb_shutdown() from igb_suspend() and use the appropriate PCI PM
> >> > callbacks in both of them.
> >> >
> >> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >> > Reported-by: Yinghai Lu <yinghai@kernel.org>
> >> > ---
> >> >  drivers/net/igb/igb_main.c |   42 ++++++++++++++++++++++++++++++------------
> >> >  1 file changed, 30 insertions(+), 12 deletions(-)
> >> >
> >> > Index: linux-2.6/drivers/net/igb/igb_main.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/drivers/net/igb/igb_main.c
> >> > +++ linux-2.6/drivers/net/igb/igb_main.c
> >> > @@ -4277,7 +4277,7 @@ int igb_set_spd_dplx(struct igb_adapter
> >> >  }
> >> >
> >> >
> >> > -static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> >> > +static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake)
> >> >  {
> >> >     struct net_device *netdev = pci_get_drvdata(pdev);
> >> >     struct igb_adapter *adapter = netdev_priv(netdev);
> >> > @@ -4336,15 +4336,9 @@ static int igb_suspend(struct pci_dev *p
> >> >             wr32(E1000_WUFC, 0);
> >> >     }
> >> >
> >> > -   /* make sure adapter isn't asleep if manageability/wol is enabled */
> >> > -   if (wufc || adapter->en_mng_pt) {
> >> > -           pci_enable_wake(pdev, PCI_D3hot, 1);
> >> > -           pci_enable_wake(pdev, PCI_D3cold, 1);
> >> > -   } else {
> >> > +   *enable_wake = wufc || adapter->en_mng_pt;
> >> > +   if (!*enable_wake)
> >> >             igb_shutdown_fiber_serdes_link_82575(hw);
> >> > -           pci_enable_wake(pdev, PCI_D3hot, 0);
> >> > -           pci_enable_wake(pdev, PCI_D3cold, 0);
> >> > -   }
> >> >
> >> >     /* Release control of h/w to f/w.  If f/w is AMT enabled, this
> >> >      * would have already happened in close and is redundant. */
> >> > @@ -4352,12 +4346,29 @@ static int igb_suspend(struct pci_dev *p
> >> >
> >> >     pci_disable_device(pdev);
> >> >
> >> > -   pci_set_power_state(pdev, pci_choose_state(pdev, state));
> >> > -
> >> >     return 0;
> >> >  }
> >> >
> >> >  #ifdef CONFIG_PM
> >> > +static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> >> > +{
> >> > +   int retval;
> >> > +   bool wake;
> >> > +
> >> > +   retval = __igb_shutdown(pdev, &wake);
> >> > +   if (retval)
> >> > +           return retval;
> >> > +
> >> > +   if (wake) {
> >> > +           pci_prepare_to_sleep(pdev);
> >> > +   } else {
> >> > +           pci_wake_from_d3(pdev, false);
> >> > +           pci_set_power_state(pdev, PCI_D3hot);
> >> > +   }
> >> > +
> >> > +   return 0;
> >> > +}
> >> > +
> >> >  static int igb_resume(struct pci_dev *pdev)
> >> >  {
> >> >     struct net_device *netdev = pci_get_drvdata(pdev);
> >> > @@ -4412,7 +4423,14 @@ static int igb_resume(struct pci_dev *pd
> >> >
> >> >  static void igb_shutdown(struct pci_dev *pdev)
> >> >  {
> >> > -   igb_suspend(pdev, PMSG_SUSPEND);
> >> > +   bool wake;
> >> > +
> >> > +   __igb_shutdown(pdev, &wake);
> >> > +
> >> > +   if (system_state == SYSTEM_POWER_OFF) {
> >> > +           pci_wake_from_d3(pdev, wake);
> >> > +           pci_set_power_state(pdev, PCI_D3hot);
> >> > +   }
> >> >  }
> >> >
> >> >  #ifdef CONFIG_NET_POLL_CONTROLLER
> >>
> >> it works too.
> >
> > Great, thanks for testing.
> >
> > Jeff, Jesse, is the patch fine with you?
> >
> > Rafael
> 
> Let me talk with Jesse on Monday about it.  I know that Jesse's
> concern's were that there were more than one driver afflicted with the
> same or similar issue and that it made more sense to fix this in the
> core for all drivers.  For instance ixgbe, IIRC, but because Yinghai
> did not have any other hardware, it was unclear if this was an issue
> on other drivers.

I think it was, at least theoretically.  We should generally avoid powering
off things on shutdown when it's not really necessary.  Also, the use of
pci_enable_wake() in these drivers is not really correct.

And in fact this is why I'm asking, because I have analogouns patches for some
other drivers in the works too.  If you are fine with the approach, I'll post
the whole series.

> After discussing this with Jesse, one of us will respond.  I will
> apply this patch to my tree anyway and if we are alright with the
> patch, I will push it out with my other patches to Dave.

Thanks!

Best,
Rafael

Re: [Updated patch] Re: [PATCH] igb: fix kexec with igb

[Jeff Kirsher]

On Sun, Mar 29, 2009 at 4:19 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday 29 March 2009, Jeff Kirsher wrote:
>> On Sat, Mar 28, 2009 at 2:27 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Tuesday 24 March 2009, Yinghai Lu wrote:
>> >> Rafael J. Wysocki wrote:
>> >> > On Thursday 12 March 2009, Yinghai Lu wrote:
>> >> >> Rafael J. Wysocki wrote:
>> >> >>> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
>> >> >>>> On Sunday 08 March 2009, Yinghai Lu wrote:
>> >> >>>>> Rafael J. Wysocki wrote:
>> >> >>>>>> On Saturday 07 March 2009, Yinghai Lu wrote:
>> >> >>>>>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
>> >> >>>>>>> <jesse.brandeburg@gmail.com> wrote:
>> >> >>>>>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> >> >>>>>>>>> Impact: could probe igb
>> >> >>>>>>>>>
>> >> >>>>>>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
>> >> >>>>>>>>> failed with -2.
>> >> >>>>>>>>>
>> >> >>>>>>>>> it looks like the same behavior happened on forcedeth.
>> >> >>>>>>>>>
>> >> >>>>>>>>> try to check system_state to make sure if put it on D3
>> >> >>>>>>>>>
>> >> >>>>>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> >> >>>>>>>>>
>> >> >>>>>>>>> ---
>> >> >>>>>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>> >> >>>>>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>> >> >>>>>>>> I see the point of the patch, but I know for a fact that ixgbe when
>> >> >>>>>>>> enabled for MSI-X also doesn't work with kexec.
>> >> >>>>>>>>
>> >> >>>>>>>> so my questions are:
>> >> >>>>>>>> are you going to change every driver?
>> >> >>>>>>> i tend to only change driver that i have related HW.
>> >> >>>>>>>
>> >> >>>>>>>> why can't this be fixed in core kernel code instead?
>> >> >>>>>>> will check it.
>> >> >>>>>>>
>> >> >>>>>>>> Shouldn't pci_enable_device take it out of D3?
>> >> >>>>>>>> Or maybe it should be taken out of D3 immediately if someone tries to
>> >> >>>>>>>> ioremap any of the BARx registers?
>> >> >>>>>>> looks like second kernel can not detect the state any more.
>> >> >>>>>> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
>> >> >>>>>> thing.  The question is why it doesn't work as expected.
>> >> >>>>> not sure... please check the version for forcedeth that you made.
>> >> >>>>>
>> >> >>>>> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
>> >> >>>>> Author: Rafael J. Wysocki <rjw@sisk.pl>
>> >> >>>>> Date:   Fri Sep 5 14:00:19 2008 -0700
>> >> >>>>>
>> >> >>>>>     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.
>> >> >>>> Thanks, I remember now.
>> >> >>> In which case you need to rework igb_shutdown() rather than igb_suspend().
>> >> >>>
>> >> >>> Something like the patch below, perhaps (totally untested).
>> >> >> it works, David, can you picked it up
>> >> >
>> >> > Still, Yinghai, can you please also test the patch below?  It fixes all
>> >> > shortcomings in the driver's suspend and shutdown methods I was talking about
>> >> > in one of the previous messages.  If it works, IMO it will be a preferable fix
>> >> > (in particular, it would be good to check if WoL still works with it,
>> >> > but I don't have the hardware).
>> >> >
>> >> > Thanks,
>> >> > Rafael
>> >> >
>> >> > ---
>> >> > From: Rafael J. Wysocki <rjw@sisk.pl>
>> >> > Subject: net/igb: Fix kexec with igb (rev. 3)
>> >> > Impact: Fix
>> >> >
>> >> > Yinghai Lu found one system with 82575EB where, in the kernel that is
>> >> > kexeced, probe igb failed with -2, the reason being that the adapter
>> >> > could not be brought back from D3 by the kexec kernel, most probably
>> >> > due to quirky hardware (it looks like the same behavior happened on
>> >> > forcedeth).
>> >> >
>> >> > Prevent igb from putting the adapter into D3 during shutdown except
>> >> > when we going to power off the system.  For this purpose, seperate
>> >> > igb_shutdown() from igb_suspend() and use the appropriate PCI PM
>> >> > callbacks in both of them.
>> >> >
>> >> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>> >> > Reported-by: Yinghai Lu <yinghai@kernel.org>
>> >> > ---
>> >> >  drivers/net/igb/igb_main.c |   42 ++++++++++++++++++++++++++++++------------
>> >> >  1 file changed, 30 insertions(+), 12 deletions(-)
>> >> >
>> >> > Index: linux-2.6/drivers/net/igb/igb_main.c
>> >> > ===================================================================
>> >> > --- linux-2.6.orig/drivers/net/igb/igb_main.c
>> >> > +++ linux-2.6/drivers/net/igb/igb_main.c
>> >> > @@ -4277,7 +4277,7 @@ int igb_set_spd_dplx(struct igb_adapter
>> >> >  }
>> >> >
>> >> >
>> >> > -static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
>> >> > +static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake)
>> >> >  {
>> >> >     struct net_device *netdev = pci_get_drvdata(pdev);
>> >> >     struct igb_adapter *adapter = netdev_priv(netdev);
>> >> > @@ -4336,15 +4336,9 @@ static int igb_suspend(struct pci_dev *p
>> >> >             wr32(E1000_WUFC, 0);
>> >> >     }
>> >> >
>> >> > -   /* make sure adapter isn't asleep if manageability/wol is enabled */
>> >> > -   if (wufc || adapter->en_mng_pt) {
>> >> > -           pci_enable_wake(pdev, PCI_D3hot, 1);
>> >> > -           pci_enable_wake(pdev, PCI_D3cold, 1);
>> >> > -   } else {
>> >> > +   *enable_wake = wufc || adapter->en_mng_pt;
>> >> > +   if (!*enable_wake)
>> >> >             igb_shutdown_fiber_serdes_link_82575(hw);
>> >> > -           pci_enable_wake(pdev, PCI_D3hot, 0);
>> >> > -           pci_enable_wake(pdev, PCI_D3cold, 0);
>> >> > -   }
>> >> >
>> >> >     /* Release control of h/w to f/w.  If f/w is AMT enabled, this
>> >> >      * would have already happened in close and is redundant. */
>> >> > @@ -4352,12 +4346,29 @@ static int igb_suspend(struct pci_dev *p
>> >> >
>> >> >     pci_disable_device(pdev);
>> >> >
>> >> > -   pci_set_power_state(pdev, pci_choose_state(pdev, state));
>> >> > -
>> >> >     return 0;
>> >> >  }
>> >> >
>> >> >  #ifdef CONFIG_PM
>> >> > +static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
>> >> > +{
>> >> > +   int retval;
>> >> > +   bool wake;
>> >> > +
>> >> > +   retval = __igb_shutdown(pdev, &wake);
>> >> > +   if (retval)
>> >> > +           return retval;
>> >> > +
>> >> > +   if (wake) {
>> >> > +           pci_prepare_to_sleep(pdev);
>> >> > +   } else {
>> >> > +           pci_wake_from_d3(pdev, false);
>> >> > +           pci_set_power_state(pdev, PCI_D3hot);
>> >> > +   }
>> >> > +
>> >> > +   return 0;
>> >> > +}
>> >> > +
>> >> >  static int igb_resume(struct pci_dev *pdev)
>> >> >  {
>> >> >     struct net_device *netdev = pci_get_drvdata(pdev);
>> >> > @@ -4412,7 +4423,14 @@ static int igb_resume(struct pci_dev *pd
>> >> >
>> >> >  static void igb_shutdown(struct pci_dev *pdev)
>> >> >  {
>> >> > -   igb_suspend(pdev, PMSG_SUSPEND);
>> >> > +   bool wake;
>> >> > +
>> >> > +   __igb_shutdown(pdev, &wake);
>> >> > +
>> >> > +   if (system_state == SYSTEM_POWER_OFF) {
>> >> > +           pci_wake_from_d3(pdev, wake);
>> >> > +           pci_set_power_state(pdev, PCI_D3hot);
>> >> > +   }
>> >> >  }
>> >> >
>> >> >  #ifdef CONFIG_NET_POLL_CONTROLLER
>> >>
>> >> it works too.
>> >
>> > Great, thanks for testing.
>> >
>> > Jeff, Jesse, is the patch fine with you?
>> >
>> > Rafael
>>
>> Let me talk with Jesse on Monday about it.  I know that Jesse's
>> concern's were that there were more than one driver afflicted with the
>> same or similar issue and that it made more sense to fix this in the
>> core for all drivers.  For instance ixgbe, IIRC, but because Yinghai
>> did not have any other hardware, it was unclear if this was an issue
>> on other drivers.
>
> I think it was, at least theoretically.  We should generally avoid powering
> off things on shutdown when it's not really necessary.  Also, the use of
> pci_enable_wake() in these drivers is not really correct.
>
> And in fact this is why I'm asking, because I have analogouns patches for some
> other drivers in the works too.  If you are fine with the approach, I'll post
> the whole series.
>
>> After discussing this with Jesse, one of us will respond.  I will
>> apply this patch to my tree anyway and if we are alright with the
>> patch, I will push it out with my other patches to Dave.
>
> Thanks!
>
> Best,
> Rafael
> --

After talking with Jesse, we are fine with it if our testers buy off
on it.  I have the patch in my tree and so Emil will run a barrage of
regression/stress tests on the patch overnight.  If everything looks
good, I will send it out with the other igb patches I have.

Thanks.

-- 
Cheers,
Jeff

Re: [Updated patch] Re: [PATCH] igb: fix kexec with igb

[Rafael J. Wysocki]

On Monday 30 March 2009, Jeff Kirsher wrote:
> On Sun, Mar 29, 2009 at 4:19 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday 29 March 2009, Jeff Kirsher wrote:
> >> On Sat, Mar 28, 2009 at 2:27 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > On Tuesday 24 March 2009, Yinghai Lu wrote:
> >> >> Rafael J. Wysocki wrote:
> >> >> > On Thursday 12 March 2009, Yinghai Lu wrote:
> >> >> >> Rafael J. Wysocki wrote:
> >> >> >>> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
> >> >> >>>> On Sunday 08 March 2009, Yinghai Lu wrote:
> >> >> >>>>> Rafael J. Wysocki wrote:
> >> >> >>>>>> On Saturday 07 March 2009, Yinghai Lu wrote:
> >> >> >>>>>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
> >> >> >>>>>>> <jesse.brandeburg@gmail.com> wrote:
> >> >> >>>>>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> >> >>>>>>>>> Impact: could probe igb
> >> >> >>>>>>>>>
> >> >> >>>>>>>>> Found one system with 82575EB, in the kernel that is kexeced, probe igb
> >> >> >>>>>>>>> failed with -2.
> >> >> >>>>>>>>>
> >> >> >>>>>>>>> it looks like the same behavior happened on forcedeth.
> >> >> >>>>>>>>>
> >> >> >>>>>>>>> try to check system_state to make sure if put it on D3
> >> >> >>>>>>>>>
> >> >> >>>>>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> >> >>>>>>>>>
> >> >> >>>>>>>>> ---
> >> >> >>>>>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
> >> >> >>>>>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
> >> >> >>>>>>>> I see the point of the patch, but I know for a fact that ixgbe when
> >> >> >>>>>>>> enabled for MSI-X also doesn't work with kexec.
> >> >> >>>>>>>>
> >> >> >>>>>>>> so my questions are:
> >> >> >>>>>>>> are you going to change every driver?
> >> >> >>>>>>> i tend to only change driver that i have related HW.
> >> >> >>>>>>>
> >> >> >>>>>>>> why can't this be fixed in core kernel code instead?
> >> >> >>>>>>> will check it.
> >> >> >>>>>>>
> >> >> >>>>>>>> Shouldn't pci_enable_device take it out of D3?
> >> >> >>>>>>>> Or maybe it should be taken out of D3 immediately if someone tries to
> >> >> >>>>>>>> ioremap any of the BARx registers?
> >> >> >>>>>>> looks like second kernel can not detect the state any more.
> >> >> >>>>>> In fact pci_enable_device() calls pci_set_power_state(dev, PCI_D0) as the first
> >> >> >>>>>> thing.  The question is why it doesn't work as expected.
> >> >> >>>>> not sure... please check the version for forcedeth that you made.
> >> >> >>>>>
> >> >> >>>>> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
> >> >> >>>>> Author: Rafael J. Wysocki <rjw@sisk.pl>
> >> >> >>>>> Date:   Fri Sep 5 14:00:19 2008 -0700
> >> >> >>>>>
> >> >> >>>>>     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.
> >> >> >>>> Thanks, I remember now.
> >> >> >>> In which case you need to rework igb_shutdown() rather than igb_suspend().
> >> >> >>>
> >> >> >>> Something like the patch below, perhaps (totally untested).
> >> >> >> it works, David, can you picked it up
> >> >> >
> >> >> > Still, Yinghai, can you please also test the patch below?  It fixes all
> >> >> > shortcomings in the driver's suspend and shutdown methods I was talking about
> >> >> > in one of the previous messages.  If it works, IMO it will be a preferable fix
> >> >> > (in particular, it would be good to check if WoL still works with it,
> >> >> > but I don't have the hardware).
> >> >> >
> >> >> > Thanks,
> >> >> > Rafael
> >> >> >
> >> >> > ---
> >> >> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >> >> > Subject: net/igb: Fix kexec with igb (rev. 3)
> >> >> > Impact: Fix
> >> >> >
> >> >> > Yinghai Lu found one system with 82575EB where, in the kernel that is
> >> >> > kexeced, probe igb failed with -2, the reason being that the adapter
> >> >> > could not be brought back from D3 by the kexec kernel, most probably
> >> >> > due to quirky hardware (it looks like the same behavior happened on
> >> >> > forcedeth).
> >> >> >
> >> >> > Prevent igb from putting the adapter into D3 during shutdown except
> >> >> > when we going to power off the system.  For this purpose, seperate
> >> >> > igb_shutdown() from igb_suspend() and use the appropriate PCI PM
> >> >> > callbacks in both of them.
> >> >> >
> >> >> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >> >> > Reported-by: Yinghai Lu <yinghai@kernel.org>
> >> >> > ---
> >> >> >  drivers/net/igb/igb_main.c |   42 ++++++++++++++++++++++++++++++------------
> >> >> >  1 file changed, 30 insertions(+), 12 deletions(-)
> >> >> >
> >> >> > Index: linux-2.6/drivers/net/igb/igb_main.c
> >> >> > ===================================================================
> >> >> > --- linux-2.6.orig/drivers/net/igb/igb_main.c
> >> >> > +++ linux-2.6/drivers/net/igb/igb_main.c
> >> >> > @@ -4277,7 +4277,7 @@ int igb_set_spd_dplx(struct igb_adapter
> >> >> >  }
> >> >> >
> >> >> >
> >> >> > -static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> >> >> > +static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake)
> >> >> >  {
> >> >> >     struct net_device *netdev = pci_get_drvdata(pdev);
> >> >> >     struct igb_adapter *adapter = netdev_priv(netdev);
> >> >> > @@ -4336,15 +4336,9 @@ static int igb_suspend(struct pci_dev *p
> >> >> >             wr32(E1000_WUFC, 0);
> >> >> >     }
> >> >> >
> >> >> > -   /* make sure adapter isn't asleep if manageability/wol is enabled */
> >> >> > -   if (wufc || adapter->en_mng_pt) {
> >> >> > -           pci_enable_wake(pdev, PCI_D3hot, 1);
> >> >> > -           pci_enable_wake(pdev, PCI_D3cold, 1);
> >> >> > -   } else {
> >> >> > +   *enable_wake = wufc || adapter->en_mng_pt;
> >> >> > +   if (!*enable_wake)
> >> >> >             igb_shutdown_fiber_serdes_link_82575(hw);
> >> >> > -           pci_enable_wake(pdev, PCI_D3hot, 0);
> >> >> > -           pci_enable_wake(pdev, PCI_D3cold, 0);
> >> >> > -   }
> >> >> >
> >> >> >     /* Release control of h/w to f/w.  If f/w is AMT enabled, this
> >> >> >      * would have already happened in close and is redundant. */
> >> >> > @@ -4352,12 +4346,29 @@ static int igb_suspend(struct pci_dev *p
> >> >> >
> >> >> >     pci_disable_device(pdev);
> >> >> >
> >> >> > -   pci_set_power_state(pdev, pci_choose_state(pdev, state));
> >> >> > -
> >> >> >     return 0;
> >> >> >  }
> >> >> >
> >> >> >  #ifdef CONFIG_PM
> >> >> > +static int igb_suspend(struct pci_dev *pdev, pm_message_t state)
> >> >> > +{
> >> >> > +   int retval;
> >> >> > +   bool wake;
> >> >> > +
> >> >> > +   retval = __igb_shutdown(pdev, &wake);
> >> >> > +   if (retval)
> >> >> > +           return retval;
> >> >> > +
> >> >> > +   if (wake) {
> >> >> > +           pci_prepare_to_sleep(pdev);
> >> >> > +   } else {
> >> >> > +           pci_wake_from_d3(pdev, false);
> >> >> > +           pci_set_power_state(pdev, PCI_D3hot);
> >> >> > +   }
> >> >> > +
> >> >> > +   return 0;
> >> >> > +}
> >> >> > +
> >> >> >  static int igb_resume(struct pci_dev *pdev)
> >> >> >  {
> >> >> >     struct net_device *netdev = pci_get_drvdata(pdev);
> >> >> > @@ -4412,7 +4423,14 @@ static int igb_resume(struct pci_dev *pd
> >> >> >
> >> >> >  static void igb_shutdown(struct pci_dev *pdev)
> >> >> >  {
> >> >> > -   igb_suspend(pdev, PMSG_SUSPEND);
> >> >> > +   bool wake;
> >> >> > +
> >> >> > +   __igb_shutdown(pdev, &wake);
> >> >> > +
> >> >> > +   if (system_state == SYSTEM_POWER_OFF) {
> >> >> > +           pci_wake_from_d3(pdev, wake);
> >> >> > +           pci_set_power_state(pdev, PCI_D3hot);
> >> >> > +   }
> >> >> >  }
> >> >> >
> >> >> >  #ifdef CONFIG_NET_POLL_CONTROLLER
> >> >>
> >> >> it works too.
> >> >
> >> > Great, thanks for testing.
> >> >
> >> > Jeff, Jesse, is the patch fine with you?
> >> >
> >> > Rafael
> >>
> >> Let me talk with Jesse on Monday about it.  I know that Jesse's
> >> concern's were that there were more than one driver afflicted with the
> >> same or similar issue and that it made more sense to fix this in the
> >> core for all drivers.  For instance ixgbe, IIRC, but because Yinghai
> >> did not have any other hardware, it was unclear if this was an issue
> >> on other drivers.
> >
> > I think it was, at least theoretically.  We should generally avoid powering
> > off things on shutdown when it's not really necessary.  Also, the use of
> > pci_enable_wake() in these drivers is not really correct.
> >
> > And in fact this is why I'm asking, because I have analogouns patches for some
> > other drivers in the works too.  If you are fine with the approach, I'll post
> > the whole series.
> >
> >> After discussing this with Jesse, one of us will respond.  I will
> >> apply this patch to my tree anyway and if we are alright with the
> >> patch, I will push it out with my other patches to Dave.
> >
> > Thanks!
> >
> > Best,
> > Rafael
> > --
> 
> After talking with Jesse, we are fine with it if our testers buy off
> on it.  I have the patch in my tree and so Emil will run a barrage of
> regression/stress tests on the patch overnight.  If everything looks
> good, I will send it out with the other igb patches I have.

Great, thanks!

Rafael

RE: [Updated patch] Re: [PATCH] igb: fix kexec with igb

[Tantilov, Emil S]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 10653 bytes --]

Rafael J. Wysocki wrote:
> On Monday 30 March 2009, Jeff Kirsher wrote:
>> On Sun, Mar 29, 2009 at 4:19 AM, Rafael J. Wysocki <rjw@sisk.pl>
>> wrote: 
>>> On Sunday 29 March 2009, Jeff Kirsher wrote:
>>>> On Sat, Mar 28, 2009 at 2:27 PM, Rafael J. Wysocki <rjw@sisk.pl>
>>>> wrote: 
>>>>> On Tuesday 24 March 2009, Yinghai Lu wrote:
>>>>>> Rafael J. Wysocki wrote:
>>>>>>> On Thursday 12 March 2009, Yinghai Lu wrote:
>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
>>>>>>>>>> On Sunday 08 March 2009, Yinghai Lu wrote:
>>>>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>>>> On Saturday 07 March 2009, Yinghai Lu wrote:
>>>>>>>>>>>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
>>>>>>>>>>>>> <jesse.brandeburg@gmail.com> wrote:
>>>>>>>>>>>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu
>>>>>>>>>>>>>> <yinghai@kernel.org> wrote: 
>>>>>>>>>>>>>>> Impact: could probe igb
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Found one system with 82575EB, in the kernel that is
>>>>>>>>>>>>>>> kexeced, probe igb failed with -2. 
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> it looks like the same behavior happened on forcedeth.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> try to check system_state to make sure if put it on D3
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>>>>>>>>>>>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>>>>>>>>>>>> I see the point of the patch, but I know for a fact that
>>>>>>>>>>>>>> ixgbe when enabled for MSI-X also doesn't work with
>>>>>>>>>>>>>> kexec. 
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> so my questions are:
>>>>>>>>>>>>>> are you going to change every driver?
>>>>>>>>>>>>> i tend to only change driver that i have related HW.
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> why can't this be fixed in core kernel code instead?
>>>>>>>>>>>>>> will check it. 
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Shouldn't pci_enable_device take it out of D3?
>>>>>>>>>>>>>> Or maybe it should be taken out of D3 immediately if
>>>>>>>>>>>>>> someone tries to ioremap any of the BARx registers?
>>>>>>>>>>>>> looks like second kernel can not detect the state any
>>>>>>>>>>>>> more. 
>>>>>>>>>>>> In fact pci_enable_device() calls pci_set_power_state(dev,
>>>>>>>>>>>> PCI_D0) as the first thing.  The question is why it
>>>>>>>>>>>> doesn't work as expected. 
>>>>>>>>>>> not sure... please check the version for forcedeth that you
>>>>>>>>>>> made. 
>>>>>>>>>>> 
>>>>>>>>>>> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
>>>>>>>>>>> Author: Rafael J. Wysocki <rjw@sisk.pl>
>>>>>>>>>>> Date:   Fri Sep 5 14:00:19 2008 -0700
>>>>>>>>>>> 
>>>>>>>>>>>     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.    
>>>>>>>>>> Thanks, I remember now.
>>>>>>>>> In which case you need to rework igb_shutdown() rather than
>>>>>>>>> igb_suspend(). 
>>>>>>>>> 
>>>>>>>>> Something like the patch below, perhaps (totally untested).
>>>>>>>> it works, David, can you picked it up
>>>>>>> 
>>>>>>> Still, Yinghai, can you please also test the patch below?  It
>>>>>>> fixes all shortcomings in the driver's suspend and shutdown
>>>>>>> methods I was talking about in one of the previous messages. 
>>>>>>> If it works, IMO it will be a preferable fix (in particular, it
>>>>>>> would be good to check if WoL still works with it, 
>>>>>>> but I don't have the hardware).
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Rafael
>>>>>>> 
>>>>>>> ---
>>>>>>> From: Rafael J. Wysocki <rjw@sisk.pl>
>>>>>>> Subject: net/igb: Fix kexec with igb (rev. 3)
>>>>>>> Impact: Fix
>>>>>>> 
>>>>>>> Yinghai Lu found one system with 82575EB where, in the kernel
>>>>>>> that is kexeced, probe igb failed with -2, the reason being
>>>>>>> that the adapter 
>>>>>>> could not be brought back from D3 by the kexec kernel, most
>>>>>>> probably 
>>>>>>> due to quirky hardware (it looks like the same behavior
>>>>>>> happened on forcedeth). 
>>>>>>> 
>>>>>>> Prevent igb from putting the adapter into D3 during shutdown
>>>>>>> except 
>>>>>>> when we going to power off the system.  For this purpose,
>>>>>>> seperate igb_shutdown() from igb_suspend() and use the
>>>>>>> appropriate PCI PM 
>>>>>>> callbacks in both of them.
>>>>>>> 
>>>>>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>>>>>>> Reported-by: Yinghai Lu <yinghai@kernel.org>
>>>>>>> ---
>>>>>>>  drivers/net/igb/igb_main.c |   42
>>>>>>>  ++++++++++++++++++++++++++++++------------ 1 file changed, 30
>>>>>>> insertions(+), 12 deletions(-) 
>>>>>>> 
>>>>>>> Index: linux-2.6/drivers/net/igb/igb_main.c
>>>>>>> ===================================================================
>>>>>>> --- linux-2.6.orig/drivers/net/igb/igb_main.c
>>>>>>> +++ linux-2.6/drivers/net/igb/igb_main.c
>>>>>>> @@ -4277,7 +4277,7 @@ int igb_set_spd_dplx(struct igb_adapter  }
>>>>>>> 
>>>>>>> 
>>>>>>> -static int igb_suspend(struct pci_dev *pdev, pm_message_t
>>>>>>> state) +static int __igb_shutdown(struct pci_dev *pdev, bool
>>>>>>>     *enable_wake)  { struct net_device *netdev =
>>>>>>>     pci_get_drvdata(pdev); struct igb_adapter *adapter =
>>>>>>> netdev_priv(netdev); @@ -4336,15 +4336,9 @@ static int
>>>>>>>             igb_suspend(struct pci_dev *p wr32(E1000_WUFC, 0);
>>>>>>>     }
>>>>>>> 
>>>>>>> -   /* make sure adapter isn't asleep if manageability/wol is
>>>>>>> enabled */ 
>>>>>>> -   if (wufc || adapter->en_mng_pt) {
>>>>>>> -           pci_enable_wake(pdev, PCI_D3hot, 1);
>>>>>>> -           pci_enable_wake(pdev, PCI_D3cold, 1);
>>>>>>> -   } else {
>>>>>>> +   *enable_wake = wufc || adapter->en_mng_pt;
>>>>>>> +   if (!*enable_wake)
>>>>>>>             igb_shutdown_fiber_serdes_link_82575(hw);
>>>>>>> -           pci_enable_wake(pdev, PCI_D3hot, 0);
>>>>>>> -           pci_enable_wake(pdev, PCI_D3cold, 0);
>>>>>>> -   }
>>>>>>> 
>>>>>>>     /* Release control of h/w to f/w.  If f/w is AMT enabled,
>>>>>>>      this * would have already happened in close and is
>>>>>>> redundant. */ @@ -4352,12 +4346,29 @@ static int
>>>>>>> igb_suspend(struct pci_dev *p 
>>>>>>> 
>>>>>>>     pci_disable_device(pdev);
>>>>>>> 
>>>>>>> -   pci_set_power_state(pdev, pci_choose_state(pdev, state)); -
>>>>>>>     return 0;
>>>>>>>  }
>>>>>>> 
>>>>>>>  #ifdef CONFIG_PM
>>>>>>> +static int igb_suspend(struct pci_dev *pdev, pm_message_t
>>>>>>> state) +{ +   int retval;
>>>>>>> +   bool wake;
>>>>>>> +
>>>>>>> +   retval = __igb_shutdown(pdev, &wake);
>>>>>>> +   if (retval)
>>>>>>> +           return retval;
>>>>>>> +
>>>>>>> +   if (wake) {
>>>>>>> +           pci_prepare_to_sleep(pdev);
>>>>>>> +   } else {
>>>>>>> +           pci_wake_from_d3(pdev, false);
>>>>>>> +           pci_set_power_state(pdev, PCI_D3hot);
>>>>>>> +   }
>>>>>>> +
>>>>>>> +   return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>>  static int igb_resume(struct pci_dev *pdev)
>>>>>>>  {
>>>>>>>     struct net_device *netdev = pci_get_drvdata(pdev);
>>>>>>> @@ -4412,7 +4423,14 @@ static int igb_resume(struct pci_dev *pd
>>>>>>> 
>>>>>>>  static void igb_shutdown(struct pci_dev *pdev)
>>>>>>>  {
>>>>>>> -   igb_suspend(pdev, PMSG_SUSPEND);
>>>>>>> +   bool wake;
>>>>>>> +
>>>>>>> +   __igb_shutdown(pdev, &wake);
>>>>>>> +
>>>>>>> +   if (system_state == SYSTEM_POWER_OFF) {
>>>>>>> +           pci_wake_from_d3(pdev, wake);
>>>>>>> +           pci_set_power_state(pdev, PCI_D3hot);
>>>>>>> +   }
>>>>>>>  }
>>>>>>> 
>>>>>>>  #ifdef CONFIG_NET_POLL_CONTROLLER
>>>>>> 
>>>>>> it works too.
>>>>> 
>>>>> Great, thanks for testing.
>>>>> 
>>>>> Jeff, Jesse, is the patch fine with you?
>>>>> 
>>>>> Rafael
>>>> 
>>>> Let me talk with Jesse on Monday about it.  I know that Jesse's
>>>> concern's were that there were more than one driver afflicted with
>>>> the same or similar issue and that it made more sense to fix this
>>>> in the core for all drivers.  For instance ixgbe, IIRC, but
>>>> because Yinghai did not have any other hardware, it was unclear if
>>>> this was an issue on other drivers.
>>> 
>>> I think it was, at least theoretically.  We should generally avoid
>>> powering off things on shutdown when it's not really necessary. 
>>> Also, the use of pci_enable_wake() in these drivers is not really
>>> correct. 
>>> 
>>> And in fact this is why I'm asking, because I have analogouns
>>> patches for some other drivers in the works too.  If you are fine
>>> with the approach, I'll post the whole series. 
>>> 
>>>> After discussing this with Jesse, one of us will respond.  I will
>>>> apply this patch to my tree anyway and if we are alright with the
>>>> patch, I will push it out with my other patches to Dave.
>>> 
>>> Thanks!
>>> 
>>> Best,
>>> Rafael
>>> --
>> 
>> After talking with Jesse, we are fine with it if our testers buy off
>> on it.  I have the patch in my tree and so Emil will run a barrage of
>> regression/stress tests on the patch overnight.  If everything looks
>> good, I will send it out with the other igb patches I have.

The patch checks out. I tested suspend/resume (including WOL) and kexec. There is only a small issue with warning on compile when CONFIG_PM is disabled:

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 13fe162..0a4f8f4 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -135,8 +135,8 @@ static inline int igb_set_vf_rlpml(struct igb_adapter *, int, int);
 static int igb_set_vf_mac(struct igb_adapter *adapter, int, unsigned char *);
 static void igb_restore_vf_multicasts(struct igb_adapter *adapter);
 
-static int igb_suspend(struct pci_dev *, pm_message_t);
 #ifdef CONFIG_PM
+static int igb_suspend(struct pci_dev *, pm_message_t);
 static int igb_resume(struct pci_dev *);
 #endif
 static void igb_shutdown(struct pci_dev *); 

Thanks,
Emil
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [Updated patch] Re: [PATCH] igb: fix kexec with igb

[Tantilov, Emil S]

Rafael J. Wysocki wrote:
> On Monday 30 March 2009, Jeff Kirsher wrote:
>> On Sun, Mar 29, 2009 at 4:19 AM, Rafael J. Wysocki <rjw@sisk.pl>
>> wrote: 
>>> On Sunday 29 March 2009, Jeff Kirsher wrote:
>>>> On Sat, Mar 28, 2009 at 2:27 PM, Rafael J. Wysocki <rjw@sisk.pl>
>>>> wrote: 
>>>>> On Tuesday 24 March 2009, Yinghai Lu wrote:
>>>>>> Rafael J. Wysocki wrote:
>>>>>>> On Thursday 12 March 2009, Yinghai Lu wrote:
>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>> On Sunday 08 March 2009, Rafael J. Wysocki wrote:
>>>>>>>>>> On Sunday 08 March 2009, Yinghai Lu wrote:
>>>>>>>>>>> Rafael J. Wysocki wrote:
>>>>>>>>>>>> On Saturday 07 March 2009, Yinghai Lu wrote:
>>>>>>>>>>>>> On Fri, Mar 6, 2009 at 11:18 PM, Jesse Brandeburg
>>>>>>>>>>>>> <jesse.brandeburg@gmail.com> wrote:
>>>>>>>>>>>>>> On Fri, Mar 6, 2009 at 8:33 PM, Yinghai Lu
>>>>>>>>>>>>>> <yinghai@kernel.org> wrote: 
>>>>>>>>>>>>>>> Impact: could probe igb
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Found one system with 82575EB, in the kernel that is
>>>>>>>>>>>>>>> kexeced, probe igb failed with -2. 
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> it looks like the same behavior happened on forcedeth.
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> try to check system_state to make sure if put it on D3
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>  drivers/net/igb/igb_main.c |   19 ++++++++++++++-----
>>>>>>>>>>>>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>>>>>>>>>>>> I see the point of the patch, but I know for a fact that
>>>>>>>>>>>>>> ixgbe when enabled for MSI-X also doesn't work with
>>>>>>>>>>>>>> kexec. 
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> so my questions are:
>>>>>>>>>>>>>> are you going to change every driver?
>>>>>>>>>>>>> i tend to only change driver that i have related HW.
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> why can't this be fixed in core kernel code instead?
>>>>>>>>>>>>>> will check it. 
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Shouldn't pci_enable_device take it out of D3?
>>>>>>>>>>>>>> Or maybe it should be taken out of D3 immediately if
>>>>>>>>>>>>>> someone tries to ioremap any of the BARx registers?
>>>>>>>>>>>>> looks like second kernel can not detect the state any
>>>>>>>>>>>>> more. 
>>>>>>>>>>>> In fact pci_enable_device() calls pci_set_power_state(dev,
>>>>>>>>>>>> PCI_D0) as the first thing.  The question is why it
>>>>>>>>>>>> doesn't work as expected. 
>>>>>>>>>>> not sure... please check the version for forcedeth that you
>>>>>>>>>>> made. 
>>>>>>>>>>> 
>>>>>>>>>>> commit 3cb5599a84c557c0dd9a19feb63a3788268cf249
>>>>>>>>>>> Author: Rafael J. Wysocki <rjw@sisk.pl>
>>>>>>>>>>> Date:   Fri Sep 5 14:00:19 2008 -0700
>>>>>>>>>>> 
>>>>>>>>>>>     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.    
>>>>>>>>>> Thanks, I remember now.
>>>>>>>>> In which case you need to rework igb_shutdown() rather than
>>>>>>>>> igb_suspend(). 
>>>>>>>>> 
>>>>>>>>> Something like the patch below, perhaps (totally untested).
>>>>>>>> it works, David, can you picked it up
>>>>>>> 
>>>>>>> Still, Yinghai, can you please also test the patch below?  It
>>>>>>> fixes all shortcomings in the driver's suspend and shutdown
>>>>>>> methods I was talking about in one of the previous messages. 
>>>>>>> If it works, IMO it will be a preferable fix (in particular, it
>>>>>>> would be good to check if WoL still works with it, 
>>>>>>> but I don't have the hardware).
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Rafael
>>>>>>> 
>>>>>>> ---
>>>>>>> From: Rafael J. Wysocki <rjw@sisk.pl>
>>>>>>> Subject: net/igb: Fix kexec with igb (rev. 3)
>>>>>>> Impact: Fix
>>>>>>> 
>>>>>>> Yinghai Lu found one system with 82575EB where, in the kernel
>>>>>>> that is kexeced, probe igb failed with -2, the reason being
>>>>>>> that the adapter 
>>>>>>> could not be brought back from D3 by the kexec kernel, most
>>>>>>> probably 
>>>>>>> due to quirky hardware (it looks like the same behavior
>>>>>>> happened on forcedeth). 
>>>>>>> 
>>>>>>> Prevent igb from putting the adapter into D3 during shutdown
>>>>>>> except 
>>>>>>> when we going to power off the system.  For this purpose,
>>>>>>> seperate igb_shutdown() from igb_suspend() and use the
>>>>>>> appropriate PCI PM 
>>>>>>> callbacks in both of them.
>>>>>>> 
>>>>>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>>>>>>> Reported-by: Yinghai Lu <yinghai@kernel.org>
>>>>>>> ---
>>>>>>>  drivers/net/igb/igb_main.c |   42
>>>>>>>  ++++++++++++++++++++++++++++++------------ 1 file changed, 30
>>>>>>> insertions(+), 12 deletions(-) 
>>>>>>> 
>>>>>>> Index: linux-2.6/drivers/net/igb/igb_main.c
>>>>>>> ===================================================================
>>>>>>> --- linux-2.6.orig/drivers/net/igb/igb_main.c
>>>>>>> +++ linux-2.6/drivers/net/igb/igb_main.c
>>>>>>> @@ -4277,7 +4277,7 @@ int igb_set_spd_dplx(struct igb_adapter  }
>>>>>>> 
>>>>>>> 
>>>>>>> -static int igb_suspend(struct pci_dev *pdev, pm_message_t
>>>>>>> state) +static int __igb_shutdown(struct pci_dev *pdev, bool
>>>>>>>     *enable_wake)  { struct net_device *netdev =
>>>>>>>     pci_get_drvdata(pdev); struct igb_adapter *adapter =
>>>>>>> netdev_priv(netdev); @@ -4336,15 +4336,9 @@ static int
>>>>>>>             igb_suspend(struct pci_dev *p wr32(E1000_WUFC, 0);
>>>>>>>     }
>>>>>>> 
>>>>>>> -   /* make sure adapter isn't asleep if manageability/wol is
>>>>>>> enabled */ 
>>>>>>> -   if (wufc || adapter->en_mng_pt) {
>>>>>>> -           pci_enable_wake(pdev, PCI_D3hot, 1);
>>>>>>> -           pci_enable_wake(pdev, PCI_D3cold, 1);
>>>>>>> -   } else {
>>>>>>> +   *enable_wake = wufc || adapter->en_mng_pt;
>>>>>>> +   if (!*enable_wake)
>>>>>>>             igb_shutdown_fiber_serdes_link_82575(hw);
>>>>>>> -           pci_enable_wake(pdev, PCI_D3hot, 0);
>>>>>>> -           pci_enable_wake(pdev, PCI_D3cold, 0);
>>>>>>> -   }
>>>>>>> 
>>>>>>>     /* Release control of h/w to f/w.  If f/w is AMT enabled,
>>>>>>>      this * would have already happened in close and is
>>>>>>> redundant. */ @@ -4352,12 +4346,29 @@ static int
>>>>>>> igb_suspend(struct pci_dev *p 
>>>>>>> 
>>>>>>>     pci_disable_device(pdev);
>>>>>>> 
>>>>>>> -   pci_set_power_state(pdev, pci_choose_state(pdev, state)); -
>>>>>>>     return 0;
>>>>>>>  }
>>>>>>> 
>>>>>>>  #ifdef CONFIG_PM
>>>>>>> +static int igb_suspend(struct pci_dev *pdev, pm_message_t
>>>>>>> state) +{ +   int retval;
>>>>>>> +   bool wake;
>>>>>>> +
>>>>>>> +   retval = __igb_shutdown(pdev, &wake);
>>>>>>> +   if (retval)
>>>>>>> +           return retval;
>>>>>>> +
>>>>>>> +   if (wake) {
>>>>>>> +           pci_prepare_to_sleep(pdev);
>>>>>>> +   } else {
>>>>>>> +           pci_wake_from_d3(pdev, false);
>>>>>>> +           pci_set_power_state(pdev, PCI_D3hot);
>>>>>>> +   }
>>>>>>> +
>>>>>>> +   return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>>  static int igb_resume(struct pci_dev *pdev)
>>>>>>>  {
>>>>>>>     struct net_device *netdev = pci_get_drvdata(pdev);
>>>>>>> @@ -4412,7 +4423,14 @@ static int igb_resume(struct pci_dev *pd
>>>>>>> 
>>>>>>>  static void igb_shutdown(struct pci_dev *pdev)
>>>>>>>  {
>>>>>>> -   igb_suspend(pdev, PMSG_SUSPEND);
>>>>>>> +   bool wake;
>>>>>>> +
>>>>>>> +   __igb_shutdown(pdev, &wake);
>>>>>>> +
>>>>>>> +   if (system_state == SYSTEM_POWER_OFF) {
>>>>>>> +           pci_wake_from_d3(pdev, wake);
>>>>>>> +           pci_set_power_state(pdev, PCI_D3hot);
>>>>>>> +   }
>>>>>>>  }
>>>>>>> 
>>>>>>>  #ifdef CONFIG_NET_POLL_CONTROLLER
>>>>>> 
>>>>>> it works too.
>>>>> 
>>>>> Great, thanks for testing.
>>>>> 
>>>>> Jeff, Jesse, is the patch fine with you?
>>>>> 
>>>>> Rafael
>>>> 
>>>> Let me talk with Jesse on Monday about it.  I know that Jesse's
>>>> concern's were that there were more than one driver afflicted with
>>>> the same or similar issue and that it made more sense to fix this
>>>> in the core for all drivers.  For instance ixgbe, IIRC, but
>>>> because Yinghai did not have any other hardware, it was unclear if
>>>> this was an issue on other drivers.
>>> 
>>> I think it was, at least theoretically.  We should generally avoid
>>> powering off things on shutdown when it's not really necessary. 
>>> Also, the use of pci_enable_wake() in these drivers is not really
>>> correct. 
>>> 
>>> And in fact this is why I'm asking, because I have analogouns
>>> patches for some other drivers in the works too.  If you are fine
>>> with the approach, I'll post the whole series. 
>>> 
>>>> After discussing this with Jesse, one of us will respond.  I will
>>>> apply this patch to my tree anyway and if we are alright with the
>>>> patch, I will push it out with my other patches to Dave.
>>> 
>>> Thanks!
>>> 
>>> Best,
>>> Rafael
>>> --
>> 
>> After talking with Jesse, we are fine with it if our testers buy off
>> on it.  I have the patch in my tree and so Emil will run a barrage of
>> regression/stress tests on the patch overnight.  If everything looks
>> good, I will send it out with the other igb patches I have.

The patch checks out. I tested suspend/resume (including WOL) and kexec. There is only a small issue with warning on compile when CONFIG_PM is disabled:

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 13fe162..0a4f8f4 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -135,8 +135,8 @@ static inline int igb_set_vf_rlpml(struct igb_adapter *, int, int);
 static int igb_set_vf_mac(struct igb_adapter *adapter, int, unsigned char *);
 static void igb_restore_vf_multicasts(struct igb_adapter *adapter);
 
-static int igb_suspend(struct pci_dev *, pm_message_t);
 #ifdef CONFIG_PM
+static int igb_suspend(struct pci_dev *, pm_message_t);
 static int igb_resume(struct pci_dev *);
 #endif
 static void igb_shutdown(struct pci_dev *); 

Thanks,
Emil

Re: [Updated patch] Re: [PATCH] igb: fix kexec with igb

[Jeff Kirsher]

On Tue, Mar 31, 2009 at 12:14 PM, Tantilov, Emil S
<emil.s.tantilov@intel.com> wrote:
>
> The patch checks out. I tested suspend/resume (including WOL) and kexec. There is only a small issue with warning on compile when CONFIG_PM is disabled:
>
> diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
> index 13fe162..0a4f8f4 100644
> --- a/drivers/net/igb/igb_main.c
> +++ b/drivers/net/igb/igb_main.c
> @@ -135,8 +135,8 @@ static inline int igb_set_vf_rlpml(struct igb_adapter *, int, int);
>  static int igb_set_vf_mac(struct igb_adapter *adapter, int, unsigned char *);
>  static void igb_restore_vf_multicasts(struct igb_adapter *adapter);
>
> -static int igb_suspend(struct pci_dev *, pm_message_t);
>  #ifdef CONFIG_PM
> +static int igb_suspend(struct pci_dev *, pm_message_t);
>  static int igb_resume(struct pci_dev *);
>  #endif
>  static void igb_shutdown(struct pci_dev *);
>
> Thanks,
> Emil
>

Rafael - Based on Emil's testing, I will modify the patch with Emil's
suggestion and push to Dave, ok?

-- 
Cheers,
Jeff

Re: [Updated patch] Re: [PATCH] igb: fix kexec with igb

[Rafael J. Wysocki]

On Tuesday 31 March 2009, Jeff Kirsher wrote:
> On Tue, Mar 31, 2009 at 12:14 PM, Tantilov, Emil S
> <emil.s.tantilov@intel.com> wrote:
> >
> > The patch checks out. I tested suspend/resume (including WOL) and kexec. There is only a small issue with warning on compile when CONFIG_PM is disabled:
> >
> > diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
> > index 13fe162..0a4f8f4 100644
> > --- a/drivers/net/igb/igb_main.c
> > +++ b/drivers/net/igb/igb_main.c
> > @@ -135,8 +135,8 @@ static inline int igb_set_vf_rlpml(struct igb_adapter *, int, int);
> >  static int igb_set_vf_mac(struct igb_adapter *adapter, int, unsigned char *);
> >  static void igb_restore_vf_multicasts(struct igb_adapter *adapter);
> >
> > -static int igb_suspend(struct pci_dev *, pm_message_t);
> >  #ifdef CONFIG_PM
> > +static int igb_suspend(struct pci_dev *, pm_message_t);
> >  static int igb_resume(struct pci_dev *);
> >  #endif
> >  static void igb_shutdown(struct pci_dev *);
> >
> > Thanks,
> > Emil
> >
> 
> Rafael - Based on Emil's testing, I will modify the patch with Emil's
> suggestion and push to Dave, ok?

Fine with me.

Thanks,
Rafael