* [PATCH] igb/e1000e/e1000/e100: make wol usable
@ 2009-07-21 21:01 Andy Gospodarek
2009-07-21 21:59 ` Ronciak, John
0 siblings, 1 reply; 9+ messages in thread
From: Andy Gospodarek @ 2009-07-21 21:01 UTC (permalink / raw)
To: netdev
Cc: jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
peter.p.waskiewicz.jr, john.ronciak, stable
It seems that the patches that intended to use the new calls to check
status and capabilities for WOL and fix it up:
commit bc79fc8409b3dccbde072e8113cc1fb3fd876fc5
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date: Wed Oct 29 14:22:18 2008 -0700
e100: adapt to the reworked PCI PM
commit e1b86d8479f90aadee57a3d07d8e61c815c202d9
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date: Fri Nov 7 20:30:37 2008 +0000
igb: Use device_set_wakeup_enable
commit de1264896c8012a261c1cba17e6a61199c276ad3
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date: Fri Nov 7 20:30:19 2008 +0000
e1000: Use device_set_wakeup_enable
commit 6ff68026f4757d68461b7fbeca5c944e1f5f8b44
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date: Wed Nov 12 09:52:32 2008 +0000
e1000e: Use device_set_wakeup_enable
Unfortunately they left out some important bits. This should make sure
the e100, igb, e1000e, and e1000-based devices can actually enable WOL
rather than their current state which is reported as not supported.
This looks like it has been broken since 2.6.28.
Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
---
e100.c | 1 +
e1000/e1000_main.c | 1 +
e1000e/netdev.c | 1 +
igb/igb_main.c | 1 +
4 files changed, 4 insertions(+)
diff --git a/drivers/net/e100.c b/drivers/net/e100.c
index 569df19..8824952 100644
--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -2816,6 +2816,7 @@ static int __devinit e100_probe(struct pci_dev *pdev,
if ((nic->mac >= mac_82558_D101_A4) &&
(nic->eeprom[eeprom_id] & eeprom_id_wol)) {
nic->flags |= wol_magic;
+ device_init_wakeup(&pdev->dev, true);
device_set_wakeup_enable(&pdev->dev, true);
}
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index d7df00c..229d874 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -1208,6 +1208,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
/* initialize the wol settings based on the eeprom settings */
adapter->wol = adapter->eeprom_wol;
+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
/* print bus type/speed/width info */
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 63415bb..5c2edaa 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -5196,6 +5196,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
/* initialize the wol settings based on the eeprom settings */
adapter->wol = adapter->eeprom_wol;
+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
/* save off EEPROM version number */
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index adb09d3..8512a40 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1475,6 +1475,7 @@ static int __devinit igb_probe(struct pci_dev *pdev,
/* initialize the wol settings based on the eeprom settings */
adapter->wol = adapter->eeprom_wol;
+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
/* reset the hardware with the new settings */
^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH] igb/e1000e/e1000/e100: make wol usable
2009-07-21 21:01 [PATCH] igb/e1000e/e1000/e100: make wol usable Andy Gospodarek
@ 2009-07-21 21:59 ` Ronciak, John
2009-07-22 2:27 ` Andy Gospodarek
0 siblings, 1 reply; 9+ messages in thread
From: Ronciak, John @ 2009-07-21 21:59 UTC (permalink / raw)
To: Andy Gospodarek, netdev@vger.kernel.org
Cc: Kirsher, Jeffrey T, Brandeburg, Jesse, Allan, Bruce W,
Waskiewicz Jr, Peter P, stable@kernel.org
>-----Original Message-----
>From: Andy Gospodarek [mailto:andy@greyhouse.net]
>Sent: Tuesday, July 21, 2009 2:01 PM
>To: netdev@vger.kernel.org
>Cc: Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce W;
>Waskiewicz Jr, Peter P; Ronciak, John; stable@kernel.org
>Subject: [PATCH] igb/e1000e/e1000/e100: make wol usable
>
>
>It seems that the patches that intended to use the new calls to check
>status and capabilities for WOL and fix it up:
>
> commit bc79fc8409b3dccbde072e8113cc1fb3fd876fc5
> Author: Rafael J. Wysocki <rjw@sisk.pl>
> Date: Wed Oct 29 14:22:18 2008 -0700
>
> e100: adapt to the reworked PCI PM
>
> commit e1b86d8479f90aadee57a3d07d8e61c815c202d9
> Author: Rafael J. Wysocki <rjw@sisk.pl>
> Date: Fri Nov 7 20:30:37 2008 +0000
>
> igb: Use device_set_wakeup_enable
>
> commit de1264896c8012a261c1cba17e6a61199c276ad3
> Author: Rafael J. Wysocki <rjw@sisk.pl>
> Date: Fri Nov 7 20:30:19 2008 +0000
>
> e1000: Use device_set_wakeup_enable
>
> commit 6ff68026f4757d68461b7fbeca5c944e1f5f8b44
> Author: Rafael J. Wysocki <rjw@sisk.pl>
> Date: Wed Nov 12 09:52:32 2008 +0000
>
> e1000e: Use device_set_wakeup_enable
>
>Unfortunately they left out some important bits. This should make sure
>the e100, igb, e1000e, and e1000-based devices can actually enable WOL
>rather than their current state which is reported as not supported.
>
>This looks like it has been broken since 2.6.28.
>
>Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>---
>
> e100.c | 1 +
> e1000/e1000_main.c | 1 +
> e1000e/netdev.c | 1 +
> igb/igb_main.c | 1 +
> 4 files changed, 4 insertions(+)
>
>diff --git a/drivers/net/e100.c b/drivers/net/e100.c
>index 569df19..8824952 100644
>--- a/drivers/net/e100.c
>+++ b/drivers/net/e100.c
>@@ -2816,6 +2816,7 @@ static int __devinit e100_probe(struct
>pci_dev *pdev,
> if ((nic->mac >= mac_82558_D101_A4) &&
> (nic->eeprom[eeprom_id] & eeprom_id_wol)) {
> nic->flags |= wol_magic;
>+ device_init_wakeup(&pdev->dev, true);
> device_set_wakeup_enable(&pdev->dev, true);
> }
>
>diff --git a/drivers/net/e1000/e1000_main.c
>b/drivers/net/e1000/e1000_main.c
>index d7df00c..229d874 100644
>--- a/drivers/net/e1000/e1000_main.c
>+++ b/drivers/net/e1000/e1000_main.c
>@@ -1208,6 +1208,7 @@ static int __devinit e1000_probe(struct
>pci_dev *pdev,
>
> /* initialize the wol settings based on the eeprom settings */
> adapter->wol = adapter->eeprom_wol;
>+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
> device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
>
> /* print bus type/speed/width info */
>diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
>index 63415bb..5c2edaa 100644
>--- a/drivers/net/e1000e/netdev.c
>+++ b/drivers/net/e1000e/netdev.c
>@@ -5196,6 +5196,7 @@ static int __devinit e1000_probe(struct
>pci_dev *pdev,
>
> /* initialize the wol settings based on the eeprom settings */
> adapter->wol = adapter->eeprom_wol;
>+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
> device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
>
> /* save off EEPROM version number */
>diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
>index adb09d3..8512a40 100644
>--- a/drivers/net/igb/igb_main.c
>+++ b/drivers/net/igb/igb_main.c
>@@ -1475,6 +1475,7 @@ static int __devinit igb_probe(struct
>pci_dev *pdev,
>
> /* initialize the wol settings based on the eeprom settings */
> adapter->wol = adapter->eeprom_wol;
>+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
> device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
>
> /* reset the hardware with the new settings */
Andy,
WoL not supported is reported back when WoL is disabled in the EEPROM. So if this is happening to you, that system (or at least that interface) has WoL disabled. How are you testing this?
Cheers,
John
-----------------------------------------------------------
"...that your people will judge you on what you can build, not what you destroy.", B. Obama, 2009
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] igb/e1000e/e1000/e100: make wol usable
2009-07-21 21:59 ` Ronciak, John
@ 2009-07-22 2:27 ` Andy Gospodarek
2009-07-22 3:25 ` Alexander Duyck
0 siblings, 1 reply; 9+ messages in thread
From: Andy Gospodarek @ 2009-07-22 2:27 UTC (permalink / raw)
To: Ronciak, John
Cc: Andy Gospodarek, netdev@vger.kernel.org, Kirsher, Jeffrey T,
Brandeburg, Jesse, Allan, Bruce W, Waskiewicz Jr, Peter P,
stable@kernel.org
On Tue, Jul 21, 2009 at 02:59:31PM -0700, Ronciak, John wrote:
> >-----Original Message-----
> >From: Andy Gospodarek [mailto:andy@greyhouse.net]
> >Sent: Tuesday, July 21, 2009 2:01 PM
> >To: netdev@vger.kernel.org
> >Cc: Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce W;
> >Waskiewicz Jr, Peter P; Ronciak, John; stable@kernel.org
> >Subject: [PATCH] igb/e1000e/e1000/e100: make wol usable
> >
> >
> >It seems that the patches that intended to use the new calls to check
> >status and capabilities for WOL and fix it up:
> >
> > commit bc79fc8409b3dccbde072e8113cc1fb3fd876fc5
> > Author: Rafael J. Wysocki <rjw@sisk.pl>
> > Date: Wed Oct 29 14:22:18 2008 -0700
> >
> > e100: adapt to the reworked PCI PM
> >
> > commit e1b86d8479f90aadee57a3d07d8e61c815c202d9
> > Author: Rafael J. Wysocki <rjw@sisk.pl>
> > Date: Fri Nov 7 20:30:37 2008 +0000
> >
> > igb: Use device_set_wakeup_enable
> >
> > commit de1264896c8012a261c1cba17e6a61199c276ad3
> > Author: Rafael J. Wysocki <rjw@sisk.pl>
> > Date: Fri Nov 7 20:30:19 2008 +0000
> >
> > e1000: Use device_set_wakeup_enable
> >
> > commit 6ff68026f4757d68461b7fbeca5c944e1f5f8b44
> > Author: Rafael J. Wysocki <rjw@sisk.pl>
> > Date: Wed Nov 12 09:52:32 2008 +0000
> >
> > e1000e: Use device_set_wakeup_enable
> >
> >Unfortunately they left out some important bits. This should make sure
> >the e100, igb, e1000e, and e1000-based devices can actually enable WOL
> >rather than their current state which is reported as not supported.
> >
> >This looks like it has been broken since 2.6.28.
> >
> >Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
> >---
> >
> > e100.c | 1 +
> > e1000/e1000_main.c | 1 +
> > e1000e/netdev.c | 1 +
> > igb/igb_main.c | 1 +
> > 4 files changed, 4 insertions(+)
> >
> >diff --git a/drivers/net/e100.c b/drivers/net/e100.c
> >index 569df19..8824952 100644
> >--- a/drivers/net/e100.c
> >+++ b/drivers/net/e100.c
> >@@ -2816,6 +2816,7 @@ static int __devinit e100_probe(struct
> >pci_dev *pdev,
> > if ((nic->mac >= mac_82558_D101_A4) &&
> > (nic->eeprom[eeprom_id] & eeprom_id_wol)) {
> > nic->flags |= wol_magic;
> >+ device_init_wakeup(&pdev->dev, true);
> > device_set_wakeup_enable(&pdev->dev, true);
> > }
> >
> >diff --git a/drivers/net/e1000/e1000_main.c
> >b/drivers/net/e1000/e1000_main.c
> >index d7df00c..229d874 100644
> >--- a/drivers/net/e1000/e1000_main.c
> >+++ b/drivers/net/e1000/e1000_main.c
> >@@ -1208,6 +1208,7 @@ static int __devinit e1000_probe(struct
> >pci_dev *pdev,
> >
> > /* initialize the wol settings based on the eeprom settings */
> > adapter->wol = adapter->eeprom_wol;
> >+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
> > device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
> >
> > /* print bus type/speed/width info */
> >diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
> >index 63415bb..5c2edaa 100644
> >--- a/drivers/net/e1000e/netdev.c
> >+++ b/drivers/net/e1000e/netdev.c
> >@@ -5196,6 +5196,7 @@ static int __devinit e1000_probe(struct
> >pci_dev *pdev,
> >
> > /* initialize the wol settings based on the eeprom settings */
> > adapter->wol = adapter->eeprom_wol;
> >+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
> > device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
> >
> > /* save off EEPROM version number */
> >diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
> >index adb09d3..8512a40 100644
> >--- a/drivers/net/igb/igb_main.c
> >+++ b/drivers/net/igb/igb_main.c
> >@@ -1475,6 +1475,7 @@ static int __devinit igb_probe(struct
> >pci_dev *pdev,
> >
> > /* initialize the wol settings based on the eeprom settings */
> > adapter->wol = adapter->eeprom_wol;
> >+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
> > device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
> >
> > /* reset the hardware with the new settings */
> Andy,
>
> WoL not supported is reported back when WoL is disabled in the EEPROM. So if this is happening to you, that system (or at least that interface) has WoL disabled. How are you testing this?
>
> Cheers,
> John
John,
I'm not doing anything differently than what the eeprom is reporting as
support with these patches. In every one of the drivers above, the
enabling of ethtool first calls device_can_wakeup. That inline function
is pretty simple and all it does is return the value of
dev->power.can_wakeup. If never set, dev->power.can_wakeup is false and
wol status can never be set.
The proper way to set it would be to call dev_init_wakeup first (or
simply call it). Take a look at the code and you will see what I'm
talking about.
Now that I'm looking at it again, this patch would probably be better
anyway:
Replace device_set_wakeup_enable calls with device_init_wakeup, so that
calls to device_can_wakeup in each drivers' code will not fail each
time. The intent of the calls to device_can_wakeup were added to ensure
the device was enabled for WOL, but without the proper call to
device_init_wakeup, any call to device_can_wakeup will always return
false.
Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
---
e100.c | 2 +-
e1000/e1000_main.c | 2 +-
e1000e/netdev.c | 2 +-
igb/igb_main.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/e100.c b/drivers/net/e100.c
index 569df19..1d82d25 100644
--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -2816,7 +2816,7 @@ static int __devinit e100_probe(struct pci_dev *pdev,
if ((nic->mac >= mac_82558_D101_A4) &&
(nic->eeprom[eeprom_id] & eeprom_id_wol)) {
nic->flags |= wol_magic;
- device_set_wakeup_enable(&pdev->dev, true);
+ device_init_wakeup(&pdev->dev, true);
}
/* ack any pending wake events, disable PME */
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index d7df00c..5f22a85 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -1208,7 +1208,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
/* initialize the wol settings based on the eeprom settings */
adapter->wol = adapter->eeprom_wol;
- device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
/* print bus type/speed/width info */
DPRINTK(PROBE, INFO, "(PCI%s:%s:%s) ",
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 63415bb..62de3ad 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -5196,7 +5196,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
/* initialize the wol settings based on the eeprom settings */
adapter->wol = adapter->eeprom_wol;
- device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
/* save off EEPROM version number */
e1000_read_nvm(&adapter->hw, 5, 1, &adapter->eeprom_vers);
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index adb09d3..f761e67 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1475,7 +1475,7 @@ static int __devinit igb_probe(struct pci_dev *pdev,
/* initialize the wol settings based on the eeprom settings */
adapter->wol = adapter->eeprom_wol;
- device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
/* reset the hardware with the new settings */
igb_reset(adapter);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] igb/e1000e/e1000/e100: make wol usable
2009-07-22 2:27 ` Andy Gospodarek
@ 2009-07-22 3:25 ` Alexander Duyck
2009-07-22 3:56 ` David Miller
2009-07-22 19:34 ` Andy Gospodarek
0 siblings, 2 replies; 9+ messages in thread
From: Alexander Duyck @ 2009-07-22 3:25 UTC (permalink / raw)
To: Andy Gospodarek
Cc: Ronciak, John, netdev@vger.kernel.org, Kirsher, Jeffrey T,
Brandeburg, Jesse, Allan, Bruce W, Waskiewicz Jr, Peter P,
stable@kernel.org
My understanding was that the can_wakeup was supposed to be
initialized by pci_pm_init or platform_pci_wakeup_init based on the
pci-e capabilities. Is this not the case? It seems like this is
where you should be looking to determine why the the can_wakeup isn't
being initialized correctly.
The patch below won't solve the problem either. The problem is that
the can_wakeup value is being disabled when the EEPROM doesn't support
WOL.
If you have to do this in the drivers, then my suggestion is to take a
look at how ixgbe is doing it. You essentially need to initialize
can_wakeup to true, and then set the should_wakeup attribute based on
the EEPROM setting or via ethtool. This way you can still toggle the
should_wakeup option without being blocked by the EEPROM disabling it.
Thanks,
Alex
On Tue, Jul 21, 2009 at 7:27 PM, Andy Gospodarek<andy@greyhouse.net> wrote:
> On Tue, Jul 21, 2009 at 02:59:31PM -0700, Ronciak, John wrote:
>> >-----Original Message-----
>> >From: Andy Gospodarek [mailto:andy@greyhouse.net]
>> >Sent: Tuesday, July 21, 2009 2:01 PM
>> >To: netdev@vger.kernel.org
>> >Cc: Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce W;
>> >Waskiewicz Jr, Peter P; Ronciak, John; stable@kernel.org
>> >Subject: [PATCH] igb/e1000e/e1000/e100: make wol usable
>> >
>> >
>> >It seems that the patches that intended to use the new calls to check
>> >status and capabilities for WOL and fix it up:
>> >
>> > commit bc79fc8409b3dccbde072e8113cc1fb3fd876fc5
>> > Author: Rafael J. Wysocki <rjw@sisk.pl>
>> > Date: Wed Oct 29 14:22:18 2008 -0700
>> >
>> > e100: adapt to the reworked PCI PM
>> >
>> > commit e1b86d8479f90aadee57a3d07d8e61c815c202d9
>> > Author: Rafael J. Wysocki <rjw@sisk.pl>
>> > Date: Fri Nov 7 20:30:37 2008 +0000
>> >
>> > igb: Use device_set_wakeup_enable
>> >
>> > commit de1264896c8012a261c1cba17e6a61199c276ad3
>> > Author: Rafael J. Wysocki <rjw@sisk.pl>
>> > Date: Fri Nov 7 20:30:19 2008 +0000
>> >
>> > e1000: Use device_set_wakeup_enable
>> >
>> > commit 6ff68026f4757d68461b7fbeca5c944e1f5f8b44
>> > Author: Rafael J. Wysocki <rjw@sisk.pl>
>> > Date: Wed Nov 12 09:52:32 2008 +0000
>> >
>> > e1000e: Use device_set_wakeup_enable
>> >
>> >Unfortunately they left out some important bits. This should make sure
>> >the e100, igb, e1000e, and e1000-based devices can actually enable WOL
>> >rather than their current state which is reported as not supported.
>> >
>> >This looks like it has been broken since 2.6.28.
>> >
>> >Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>> >---
>> >
>> > e100.c | 1 +
>> > e1000/e1000_main.c | 1 +
>> > e1000e/netdev.c | 1 +
>> > igb/igb_main.c | 1 +
>> > 4 files changed, 4 insertions(+)
>> >
>> >diff --git a/drivers/net/e100.c b/drivers/net/e100.c
>> >index 569df19..8824952 100644
>> >--- a/drivers/net/e100.c
>> >+++ b/drivers/net/e100.c
>> >@@ -2816,6 +2816,7 @@ static int __devinit e100_probe(struct
>> >pci_dev *pdev,
>> > if ((nic->mac >= mac_82558_D101_A4) &&
>> > (nic->eeprom[eeprom_id] & eeprom_id_wol)) {
>> > nic->flags |= wol_magic;
>> >+ device_init_wakeup(&pdev->dev, true);
>> > device_set_wakeup_enable(&pdev->dev, true);
>> > }
>> >
>> >diff --git a/drivers/net/e1000/e1000_main.c
>> >b/drivers/net/e1000/e1000_main.c
>> >index d7df00c..229d874 100644
>> >--- a/drivers/net/e1000/e1000_main.c
>> >+++ b/drivers/net/e1000/e1000_main.c
>> >@@ -1208,6 +1208,7 @@ static int __devinit e1000_probe(struct
>> >pci_dev *pdev,
>> >
>> > /* initialize the wol settings based on the eeprom settings */
>> > adapter->wol = adapter->eeprom_wol;
>> >+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
>> > device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
>> >
>> > /* print bus type/speed/width info */
>> >diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
>> >index 63415bb..5c2edaa 100644
>> >--- a/drivers/net/e1000e/netdev.c
>> >+++ b/drivers/net/e1000e/netdev.c
>> >@@ -5196,6 +5196,7 @@ static int __devinit e1000_probe(struct
>> >pci_dev *pdev,
>> >
>> > /* initialize the wol settings based on the eeprom settings */
>> > adapter->wol = adapter->eeprom_wol;
>> >+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
>> > device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
>> >
>> > /* save off EEPROM version number */
>> >diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
>> >index adb09d3..8512a40 100644
>> >--- a/drivers/net/igb/igb_main.c
>> >+++ b/drivers/net/igb/igb_main.c
>> >@@ -1475,6 +1475,7 @@ static int __devinit igb_probe(struct
>> >pci_dev *pdev,
>> >
>> > /* initialize the wol settings based on the eeprom settings */
>> > adapter->wol = adapter->eeprom_wol;
>> >+ device_init_wakeup(&adapter->pdev->dev, adapter->wol);
>> > device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
>> >
>> > /* reset the hardware with the new settings */
>> Andy,
>>
>> WoL not supported is reported back when WoL is disabled in the EEPROM. So if this is happening to you, that system (or at least that interface) has WoL disabled. How are you testing this?
>>
>> Cheers,
>> John
>
> John,
>
> I'm not doing anything differently than what the eeprom is reporting as
> support with these patches. In every one of the drivers above, the
> enabling of ethtool first calls device_can_wakeup. That inline function
> is pretty simple and all it does is return the value of
> dev->power.can_wakeup. If never set, dev->power.can_wakeup is false and
> wol status can never be set.
>
> The proper way to set it would be to call dev_init_wakeup first (or
> simply call it). Take a look at the code and you will see what I'm
> talking about.
>
> Now that I'm looking at it again, this patch would probably be better
> anyway:
>
> Replace device_set_wakeup_enable calls with device_init_wakeup, so that
> calls to device_can_wakeup in each drivers' code will not fail each
> time. The intent of the calls to device_can_wakeup were added to ensure
> the device was enabled for WOL, but without the proper call to
> device_init_wakeup, any call to device_can_wakeup will always return
> false.
>
> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
> ---
>
> e100.c | 2 +-
> e1000/e1000_main.c | 2 +-
> e1000e/netdev.c | 2 +-
> igb/igb_main.c | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/e100.c b/drivers/net/e100.c
> index 569df19..1d82d25 100644
> --- a/drivers/net/e100.c
> +++ b/drivers/net/e100.c
> @@ -2816,7 +2816,7 @@ static int __devinit e100_probe(struct pci_dev *pdev,
> if ((nic->mac >= mac_82558_D101_A4) &&
> (nic->eeprom[eeprom_id] & eeprom_id_wol)) {
> nic->flags |= wol_magic;
> - device_set_wakeup_enable(&pdev->dev, true);
> + device_init_wakeup(&pdev->dev, true);
> }
>
> /* ack any pending wake events, disable PME */
> diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
> index d7df00c..5f22a85 100644
> --- a/drivers/net/e1000/e1000_main.c
> +++ b/drivers/net/e1000/e1000_main.c
> @@ -1208,7 +1208,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
>
> /* initialize the wol settings based on the eeprom settings */
> adapter->wol = adapter->eeprom_wol;
> - device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
> + device_init_wakeup(&adapter->pdev->dev, adapter->wol);
>
> /* print bus type/speed/width info */
> DPRINTK(PROBE, INFO, "(PCI%s:%s:%s) ",
> diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
> index 63415bb..62de3ad 100644
> --- a/drivers/net/e1000e/netdev.c
> +++ b/drivers/net/e1000e/netdev.c
> @@ -5196,7 +5196,7 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
>
> /* initialize the wol settings based on the eeprom settings */
> adapter->wol = adapter->eeprom_wol;
> - device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
> + device_init_wakeup(&adapter->pdev->dev, adapter->wol);
>
> /* save off EEPROM version number */
> e1000_read_nvm(&adapter->hw, 5, 1, &adapter->eeprom_vers);
> diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
> index adb09d3..f761e67 100644
> --- a/drivers/net/igb/igb_main.c
> +++ b/drivers/net/igb/igb_main.c
> @@ -1475,7 +1475,7 @@ static int __devinit igb_probe(struct pci_dev *pdev,
>
> /* initialize the wol settings based on the eeprom settings */
> adapter->wol = adapter->eeprom_wol;
> - device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
> + device_init_wakeup(&adapter->pdev->dev, adapter->wol);
>
> /* reset the hardware with the new settings */
> igb_reset(adapter);
>
>
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] igb/e1000e/e1000/e100: make wol usable
2009-07-22 3:25 ` Alexander Duyck
@ 2009-07-22 3:56 ` David Miller
2009-07-22 19:34 ` Andy Gospodarek
1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2009-07-22 3:56 UTC (permalink / raw)
To: alexander.duyck
Cc: andy, john.ronciak, netdev, jeffrey.t.kirsher, jesse.brandeburg,
bruce.w.allan, peter.p.waskiewicz.jr, stable
From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Tue, 21 Jul 2009 20:25:31 -0700
> My understanding was that the can_wakeup was supposed to be
> initialized by pci_pm_init or platform_pci_wakeup_init based on the
> pci-e capabilities. Is this not the case? It seems like this is
> where you should be looking to determine why the the can_wakeup isn't
> being initialized correctly.
>
> The patch below won't solve the problem either. The problem is that
> the can_wakeup value is being disabled when the EEPROM doesn't support
> WOL.
>
> If you have to do this in the drivers, then my suggestion is to take a
> look at how ixgbe is doing it. You essentially need to initialize
> can_wakeup to true, and then set the should_wakeup attribute based on
> the EEPROM setting or via ethtool. This way you can still toggle the
> should_wakeup option without being blocked by the EEPROM disabling it.
It looks like the approach needs to be worked out still, so
I'm tossing these patches from patchwork.
Let me know when a more final fix is ready.
Thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] igb/e1000e/e1000/e100: make wol usable
2009-07-22 3:25 ` Alexander Duyck
2009-07-22 3:56 ` David Miller
@ 2009-07-22 19:34 ` Andy Gospodarek
2009-07-22 20:00 ` Waskiewicz Jr, Peter P
2009-07-22 23:43 ` Waskiewicz Jr, Peter P
1 sibling, 2 replies; 9+ messages in thread
From: Andy Gospodarek @ 2009-07-22 19:34 UTC (permalink / raw)
To: Alexander Duyck
Cc: Andy Gospodarek, Ronciak, John, netdev@vger.kernel.org,
Kirsher, Jeffrey T, Brandeburg, Jesse, Allan, Bruce W,
Waskiewicz Jr, Peter P, stable@kernel.org
On Tue, Jul 21, 2009 at 08:25:31PM -0700, Alexander Duyck wrote:
> My understanding was that the can_wakeup was supposed to be
> initialized by pci_pm_init or platform_pci_wakeup_init based on the
> pci-e capabilities. Is this not the case? It seems like this is
> where you should be looking to determine why the the can_wakeup isn't
> being initialized correctly.
>
> The patch below won't solve the problem either. The problem is that
> the can_wakeup value is being disabled when the EEPROM doesn't support
> WOL.
The lack of EEPROM support for this was not the problem at all.
Ultimately it turns out that can_wakeup is set in pci_pm_init, so calls
to device_init_wakeup and device_set_wakeup_capable should not be needed
in any driver.
> If you have to do this in the drivers, then my suggestion is to take a
> look at how ixgbe is doing it. You essentially need to initialize
> can_wakeup to true, and then set the should_wakeup attribute based on
> the EEPROM setting or via ethtool. This way you can still toggle the
> should_wakeup option without being blocked by the EEPROM disabling it.
The first patch does exactly that (and ixgbe was part of my inspiration
for it). Since can_wakeup and should_wakeup are both set in
device_init_wakeup it's a suitable alternative in driver probe
functions, but probably not ideal when drivers check for
device_can_wakeup in their ethtool set_wol functions.
Anyway, I knew I would find a bug or unnecessary code between e1000e and
friends or ixgbe, so it looks like ixgbe has an unnecessary call to
device_init_wakeup. Feel free to consider this patch as an alternative.
[PATCH] ixgbe: remove unnecessary call to device_init_wakeup
Calls to device_init_wakeup should not be necessary in drivers that use
device_set_wakeup_enable since pci_pm_init will set the can_wakeup flag
for the device when initialized.
I can't test this since I don't have any of the 82599 KX4 interfaces
(the only ones capable of WOL), but I did instrument ixgbe_probe and
know that can_wakeup=1 when device_init_wakeup is removed.
Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
---
ixgbe_main.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 79f60e8..2f15abf 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -5640,7 +5640,6 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
adapter->wol = 0;
break;
}
- device_init_wakeup(&adapter->pdev->dev, true);
device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
/* pick up the PCI bus settings for reporting later */
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] igb/e1000e/e1000/e100: make wol usable
2009-07-22 19:34 ` Andy Gospodarek
@ 2009-07-22 20:00 ` Waskiewicz Jr, Peter P
2009-07-22 23:43 ` Waskiewicz Jr, Peter P
1 sibling, 0 replies; 9+ messages in thread
From: Waskiewicz Jr, Peter P @ 2009-07-22 20:00 UTC (permalink / raw)
To: Andy Gospodarek
Cc: Alexander Duyck, Ronciak, John, netdev@vger.kernel.org,
Kirsher, Jeffrey T, Brandeburg, Jesse, Allan, Bruce W,
Waskiewicz Jr, Peter P, stable@kernel.org
> [PATCH] ixgbe: remove unnecessary call to device_init_wakeup
>
> Calls to device_init_wakeup should not be necessary in drivers that use
> device_set_wakeup_enable since pci_pm_init will set the can_wakeup flag
> for the device when initialized.
>
> I can't test this since I don't have any of the 82599 KX4 interfaces
> (the only ones capable of WOL), but I did instrument ixgbe_probe and
> know that can_wakeup=1 when device_init_wakeup is removed.
>
> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>
> ---
>
> ixgbe_main.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> index 79f60e8..2f15abf 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -5640,7 +5640,6 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
> adapter->wol = 0;
> break;
> }
> - device_init_wakeup(&adapter->pdev->dev, true);
> device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
>
> /* pick up the PCI bus settings for reporting later */
I need to rebuild one of my boxes in the lab that I do WoL testing on for
ixgbe devices. That shouldn't take me long, and I'll test this. If it
works as expected, I'll send my ack.
Cheers,
-PJ Waskiewicz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] igb/e1000e/e1000/e100: make wol usable
2009-07-22 19:34 ` Andy Gospodarek
2009-07-22 20:00 ` Waskiewicz Jr, Peter P
@ 2009-07-22 23:43 ` Waskiewicz Jr, Peter P
2009-07-23 18:06 ` David Miller
1 sibling, 1 reply; 9+ messages in thread
From: Waskiewicz Jr, Peter P @ 2009-07-22 23:43 UTC (permalink / raw)
To: Andy Gospodarek
Cc: Alexander Duyck, Ronciak, John, netdev@vger.kernel.org,
Kirsher, Jeffrey T, Brandeburg, Jesse, Allan, Bruce W,
Waskiewicz Jr, Peter P, stable@kernel.org
On Wed, 22 Jul 2009, Andy Gospodarek wrote:
> On Tue, Jul 21, 2009 at 08:25:31PM -0700, Alexander Duyck wrote:
> > My understanding was that the can_wakeup was supposed to be
> > initialized by pci_pm_init or platform_pci_wakeup_init based on the
> > pci-e capabilities. Is this not the case? It seems like this is
> > where you should be looking to determine why the the can_wakeup isn't
> > being initialized correctly.
> >
> > The patch below won't solve the problem either. The problem is that
> > the can_wakeup value is being disabled when the EEPROM doesn't support
> > WOL.
>
> The lack of EEPROM support for this was not the problem at all.
> Ultimately it turns out that can_wakeup is set in pci_pm_init, so calls
> to device_init_wakeup and device_set_wakeup_capable should not be needed
> in any driver.
>
> > If you have to do this in the drivers, then my suggestion is to take a
> > look at how ixgbe is doing it. You essentially need to initialize
> > can_wakeup to true, and then set the should_wakeup attribute based on
> > the EEPROM setting or via ethtool. This way you can still toggle the
> > should_wakeup option without being blocked by the EEPROM disabling it.
>
> The first patch does exactly that (and ixgbe was part of my inspiration
> for it). Since can_wakeup and should_wakeup are both set in
> device_init_wakeup it's a suitable alternative in driver probe
> functions, but probably not ideal when drivers check for
> device_can_wakeup in their ethtool set_wol functions.
>
> Anyway, I knew I would find a bug or unnecessary code between e1000e and
> friends or ixgbe, so it looks like ixgbe has an unnecessary call to
> device_init_wakeup. Feel free to consider this patch as an alternative.
>
>
>
> [PATCH] ixgbe: remove unnecessary call to device_init_wakeup
>
> Calls to device_init_wakeup should not be necessary in drivers that use
> device_set_wakeup_enable since pci_pm_init will set the can_wakeup flag
> for the device when initialized.
>
> I can't test this since I don't have any of the 82599 KX4 interfaces
> (the only ones capable of WOL), but I did instrument ixgbe_probe and
> know that can_wakeup=1 when device_init_wakeup is removed.
>
> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>
> ---
>
> ixgbe_main.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
> index 79f60e8..2f15abf 100644
> --- a/drivers/net/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ixgbe/ixgbe_main.c
> @@ -5640,7 +5640,6 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
> adapter->wol = 0;
> break;
> }
> - device_init_wakeup(&adapter->pdev->dev, true);
> device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
>
> /* pick up the PCI bus settings for reporting later */
Works just fine on two different 82599 mezz cards that support WoL.
Thanks Andy for finding and fixing.
Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] igb/e1000e/e1000/e100: make wol usable
2009-07-22 23:43 ` Waskiewicz Jr, Peter P
@ 2009-07-23 18:06 ` David Miller
0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2009-07-23 18:06 UTC (permalink / raw)
To: peter.p.waskiewicz.jr
Cc: andy, alexander.duyck, john.ronciak, netdev, jeffrey.t.kirsher,
jesse.brandeburg, bruce.w.allan, stable
From: "Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@intel.com>
Date: Wed, 22 Jul 2009 16:43:03 -0700 (Pacific Daylight Time)
> On Wed, 22 Jul 2009, Andy Gospodarek wrote:
>
>> [PATCH] ixgbe: remove unnecessary call to device_init_wakeup
>>
>> Calls to device_init_wakeup should not be necessary in drivers that use
>> device_set_wakeup_enable since pci_pm_init will set the can_wakeup flag
>> for the device when initialized.
>>
>> I can't test this since I don't have any of the 82599 KX4 interfaces
>> (the only ones capable of WOL), but I did instrument ixgbe_probe and
>> know that can_wakeup=1 when device_init_wakeup is removed.
>>
>> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
...
> Works just fine on two different 82599 mezz cards that support WoL.
> Thanks Andy for finding and fixing.
>
> Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-07-23 18:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-07-21 21:01 [PATCH] igb/e1000e/e1000/e100: make wol usable Andy Gospodarek
2009-07-21 21:59 ` Ronciak, John
2009-07-22 2:27 ` Andy Gospodarek
2009-07-22 3:25 ` Alexander Duyck
2009-07-22 3:56 ` David Miller
2009-07-22 19:34 ` Andy Gospodarek
2009-07-22 20:00 ` Waskiewicz Jr, Peter P
2009-07-22 23:43 ` Waskiewicz Jr, Peter P
2009-07-23 18:06 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox