[PATCH 1/3] e1000e: Use device_set_wakeup_enable

[PATCH 1/3] e1000e: Use device_set_wakeup_enable

[Jeff Kirsher]

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

Since dev->power.should_wakeup bit is used by the PCI core to
decide whether the device should wake up the system from sleep
states, set/unset this bit whenever WOL is enabled/disabled using
e1000_set_wol().  Accordingly, use device_can_wakeup() for checking
if wake-up is supported by the device.

Signed-off-by: "Rafael J. Wysocki" <rjw@sisk.pl>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/e1000e/ethtool.c |    8 ++++++--
 drivers/net/e1000e/netdev.c  |    1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/e1000e/ethtool.c b/drivers/net/e1000e/ethtool.c
index 70c11c8..bfa40d3 100644
--- a/drivers/net/e1000e/ethtool.c
+++ b/drivers/net/e1000e/ethtool.c
@@ -1713,7 +1713,8 @@ static void e1000_get_wol(struct net_device *netdev,
 	wol->supported = 0;
 	wol->wolopts = 0;
 
-	if (!(adapter->flags & FLAG_HAS_WOL))
+	if (!(adapter->flags & FLAG_HAS_WOL) ||
+	    !device_can_wakeup(&adapter->pdev>dev))
 		return;
 
 	wol->supported = WAKE_UCAST | WAKE_MCAST |
@@ -1751,7 +1752,8 @@ static int e1000_set_wol(struct net_device *netdev,
 	if (wol->wolopts & WAKE_MAGICSECURE)
 		return -EOPNOTSUPP;
 
-	if (!(adapter->flags & FLAG_HAS_WOL))
+	if (!(adapter->flags & FLAG_HAS_WOL) ||
+	    !device_can_wakeup(&adapter->pdev>dev))
 		return wol->wolopts ? -EOPNOTSUPP : 0;
 
 	/* these settings will always override what we currently have */
@@ -1770,6 +1772,8 @@ static int e1000_set_wol(struct net_device *netdev,
 	if (wol->wolopts & WAKE_ARP)
 		adapter->wol |= E1000_WUFC_ARP;
 
+	device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
+
 	return 0;
 }
 
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index abd492b..2c8dffd 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -4970,6 +4970,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);
 
 	/* reset the hardware with the new settings */
 	e1000e_reset(adapter);

[PATCH 2/3] e1000: Use device_set_wakeup_enable

[Jeff Kirsher]

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

Since dev->power.should_wakeup bit is used by the PCI core to
decide whether the device should wake up the system from sleep
states, set/unset this bit whenever WOL is enabled/disabled using
e1000_set_wol().  Accordingly, use device_can_wakeup() for checking
if wake-up is supported by the device.

Signed-off-by: "Rafael J. Wysocki" <rjw@sisk.pl>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

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

diff --git a/drivers/net/e1000/e1000_ethtool.c b/drivers/net/e1000/e1000_ethtool.c
index 6a3893a..c854c96 100644
--- a/drivers/net/e1000/e1000_ethtool.c
+++ b/drivers/net/e1000/e1000_ethtool.c
@@ -1774,7 +1774,8 @@ static void e1000_get_wol(struct net_device *netdev,
 
 	/* this function will set ->supported = 0 and return 1 if wol is not
 	 * supported by this hardware */
-	if (e1000_wol_exclusion(adapter, wol))
+	if (e1000_wol_exclusion(adapter, wol) ||
+	    !device_can_wakeup(&adapter->pdev->dev))
 		return;
 
 	/* apply any specific unsupported masks here */
@@ -1811,7 +1812,8 @@ static int e1000_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 	if (wol->wolopts & (WAKE_PHY | WAKE_ARP | WAKE_MAGICSECURE))
 		return -EOPNOTSUPP;
 
-	if (e1000_wol_exclusion(adapter, wol))
+	if (e1000_wol_exclusion(adapter, wol) ||
+	    !device_can_wakeup(&adapter->pdev->dev))
 		return wol->wolopts ? -EOPNOTSUPP : 0;
 
 	switch (hw->device_id) {
@@ -1838,6 +1840,8 @@ static int e1000_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 	if (wol->wolopts & WAKE_MAGIC)
 		adapter->wol |= E1000_WUFC_MAG;
 
+	device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
+
 	return 0;
 }
 
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index fac8215..872799b 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -1179,6 +1179,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);
 
 	/* print bus type/speed/width info */
 	DPRINTK(PROBE, INFO, "(PCI%s:%s:%s) ",

[PATCH 3/3] igb: Use device_set_wakeup_enable

[Jeff Kirsher]

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

Since dev->power.should_wakeup bit is used by the PCI core to
decide whether the device should wake up the system from sleep
states, set/unset this bit whenever WOL is enabled/disabled using
igb_set_wol().  Accordingly, use device_can_wakeup() for checking
if wake-up is supported by the device.

Signed-off-by: "Rafael J. Wysocki" <rjw@sisk.pl>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/igb_ethtool.c |    8 ++++++--
 drivers/net/igb/igb_main.c    |    1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/igb/igb_ethtool.c b/drivers/net/igb/igb_ethtool.c
index 58906c9..89964fa 100644
--- a/drivers/net/igb/igb_ethtool.c
+++ b/drivers/net/igb/igb_ethtool.c
@@ -1776,7 +1776,8 @@ static void igb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 
 	/* this function will set ->supported = 0 and return 1 if wol is not
 	 * supported by this hardware */
-	if (igb_wol_exclusion(adapter, wol))
+	if (igb_wol_exclusion(adapter, wol) ||
+	    !device_can_wakeup(&adapter->pdev->dev))
 		return;
 
 	/* apply any specific unsupported masks here */
@@ -1805,7 +1806,8 @@ static int igb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 	if (wol->wolopts & (WAKE_PHY | WAKE_ARP | WAKE_MAGICSECURE))
 		return -EOPNOTSUPP;
 
-	if (igb_wol_exclusion(adapter, wol))
+	if (igb_wol_exclusion(adapter, wol) ||
+	    !device_can_wakeup(&adapter->pdev->dev))
 		return wol->wolopts ? -EOPNOTSUPP : 0;
 
 	switch (hw->device_id) {
@@ -1825,6 +1827,8 @@ static int igb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 	if (wol->wolopts & WAKE_MAGIC)
 		adapter->wol |= E1000_WUFC_MAG;
 
+	device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
+
 	return 0;
 }
 
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 1f397cd..0a98015 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1244,6 +1244,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);
 
 	/* reset the hardware with the new settings */
 	igb_reset(adapter);

Re: [Bugme-new] [Bug 11865] New: WOL for E100 Doesn't Work Anymore

[Rafael J. Wysocki]

On Tuesday, 4 of November 2008, bugme-daemon@bugzilla.kernel.org wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=11865
> 
> ------- Comment #5 from jesse.brandeburg@intel.com  2008-11-03 16:48 -------
> Rafael J. Wysocki wrote:
> >>>> 00:14.0 Ethernet controller: Intel Corporation 82557/8/9 [Ethernet
> >>>> Pro 100] (rev 08) 
> >>>> 
> >>>> Problem Description:
> >>>> As of kernel version 2.6.27, WOL has stopped working.  The box no
> >>>> longer powers on after the magic packet has been sent.  Rebooting
> >>>> using kernel-2.6.26*, WOL *does* work!
> 
> >> I think there is an outstanding patch set to "make device use the
> >> new power management api"
> >> http://marc.info/?l=linux-netdev&m=121874992800468&w=2 
> > 
> > There were three patches like this, one against sky2 and the others
> > for e100 and skge.  The sky2 one has been merged, the other two are
> > in the Jeff's tree AFAICS.
> 
> why did you pick those three drivers?

I could test them myself at that time.

> I'm glad you picked e100 as one of the ones to make the change to, but there
> are a lot of other drivers that support wake on lan, and are probably now
> broken. 

Well, many of them wouldn't work due to the missing ACPI support and the
changes in question were necessary to add that support.  Also, there were
_no_ problem reports related to this until now, except for the sky2 case that
was immediately handled.

> >> It doesn't appear this patch was in 2.6.27, I'm not sure why not,
> >> but I am relatively afraid that lots of devices ability to wake up
> >> got busted in 2.6.27.
> > 
> > Well, unfortunately the maintainers of the networking code were not
> > very interested in these patches, although I had sent them well
> > before 2.6.27 
> > for the first time.
> 
> unless I am mistaken the reason we have drivers in the kernel is that so
> when API changes are made to the kernel the driver is pulled along with
> it.  As I understand it, your changes to the core API should have been
> rejected until the drivers that used power management APIs were updated
> too, otherwise we just get generic user breakage.

Unfortunately, I had to fix some damage that had been caused by someone else
before and I wanted to send fixes for all of the drivers once the first batch
of my patches (sky2, e100, skge, tg3 - I forgot about this one, it's already
in) had been accepted, but that hasn't really happened yet.

> Would you have asked for our help directly (CC:) we probably can/could
> help with our drivers.

Sorry about that, my fault.

> > Generally, the sky2 commit at
> >
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commi
> t;h=9d731d77c9794bb0a264f58d35949a1ab6dcc41c
> > illustrates what needs to be done for other NICs supporting WOL (the
> > key part is to call device_set_wakeup_enable() with appropriate
> > arguments whenever 
> > WOL is enabled/disabled and using for the initialization).
> > 
> > I'm going to do this over time for all of the drivers, but if anyone
> > can help, please do so.
> 
> While that is a worthy goal, I don't think it is okay to have
> regressions until you have time to fix them.  This is a pretty big
> change in a subtly difficult area to both get right and to test.

This isn't really "until I have time to fix them", but "until my fixes are
accepted" thing.  I _really_ care about regressions, but in this case I failed
to make people realize the importance of the patches I was sending.

Moreover, I wasn't quite sure how many systems would _really_ be broken,
because many of them didn't work anyway (despite the support in the drivers),
as I said above.  For this reason, I thought I would fix problems as soon as
they were reported, but there was only one report before this one.

> Your work is appeciated, I just didn't realize it would break all
> devices that didn't implement it.

In fact it doesn't really break them, you can write 'enabled' to
/sys/devices/.../power/wakeup of the device in question and it will work.

> I'm open to corrections to my point of view above.

Well, before those changes WOL didn't work on any of my machines and now
it works on all of them (except for one that hasn't been tested) - with the
already merged patches and the two waiting for merging.

Sorry about the problems caused, I'm willing to fix them ASAP.

Thanks,
Rafael

[PATCH 0/3] WOL fixes for e1000e, e1000 and igb (was: Re: [Bugme-new] [Bug 11865] New: WOL for E100 Doesn't Work Anymore)

[Rafael J. Wysocki]

On Tuesday, 4 of November 2008, Rafael J. Wysocki wrote:
[--snip--]
> 
> > Your work is appeciated, I just didn't realize it would break all
> > devices that didn't implement it.
> 
> In fact it doesn't really break them, you can write 'enabled' to
> /sys/devices/.../power/wakeup of the device in question and it will work.
> 
> > I'm open to corrections to my point of view above.
> 
> Well, before those changes WOL didn't work on any of my machines and now
> it works on all of them (except for one that hasn't been tested) - with the
> already merged patches and the two waiting for merging.
> 
> Sorry about the problems caused, I'm willing to fix them ASAP.

BTW, WOL fixes for e1000e, e1000 and igb follow.

Thanks,
Rafael


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

[PATCH 1/3] e1000e: Use device_set_wakeup_enable

[Rafael J. Wysocki]

Author: Rafael J. Wysocki <rjw@sisk.pl>
Subject: e1000e: Use device_set_wakeup_enable

Since dev->power.should_wakeup bit is used by the PCI core to
decide whether the device should wake up the system from sleep
states, set/unset this bit whenever WOL is enabled/disabled using
e1000_set_wol().  Accordingly, use device_can_wakeup() for checking
if wake-up is supported by the device.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/net/e1000e/ethtool.c |    8 ++++++--
 drivers/net/e1000e/netdev.c  |    1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/net/e1000e/ethtool.c
===================================================================
--- linux-2.6.orig/drivers/net/e1000e/ethtool.c
+++ linux-2.6/drivers/net/e1000e/ethtool.c
@@ -1713,7 +1713,8 @@ static void e1000_get_wol(struct net_dev
 	wol->supported = 0;
 	wol->wolopts = 0;
 
-	if (!(adapter->flags & FLAG_HAS_WOL))
+	if (!(adapter->flags & FLAG_HAS_WOL) ||
+	    !device_can_wakeup(&adapter->pdev->dev))
 		return;
 
 	wol->supported = WAKE_UCAST | WAKE_MCAST |
@@ -1751,7 +1752,8 @@ static int e1000_set_wol(struct net_devi
 	if (wol->wolopts & WAKE_MAGICSECURE)
 		return -EOPNOTSUPP;
 
-	if (!(adapter->flags & FLAG_HAS_WOL))
+	if (!(adapter->flags & FLAG_HAS_WOL) ||
+	    !device_can_wakeup(&adapter->pdev->dev))
 		return wol->wolopts ? -EOPNOTSUPP : 0;
 
 	/* these settings will always override what we currently have */
@@ -1770,6 +1772,8 @@ static int e1000_set_wol(struct net_devi
 	if (wol->wolopts & WAKE_ARP)
 		adapter->wol |= E1000_WUFC_ARP;
 
+	device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
+
 	return 0;
 }
 
Index: linux-2.6/drivers/net/e1000e/netdev.c
===================================================================
--- linux-2.6.orig/drivers/net/e1000e/netdev.c
+++ linux-2.6/drivers/net/e1000e/netdev.c
@@ -4970,6 +4970,7 @@ static int __devinit e1000_probe(struct
 
 	/* initialize the wol settings based on the eeprom settings */
 	adapter->wol = adapter->eeprom_wol;
+	device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
 
 	/* reset the hardware with the new settings */
 	e1000e_reset(adapter);


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/