[REGRESSION][PATCH] Fix an old sky2 WOL regression

Sky2 Wake on LAN is broken since February 2010 on a number of systems.
Yes. More than two years.

We know about the problem and the cause since October 2010
(Bugzilla bug #19492). It´s commit 87b09f1f25cd1e01d7c50bf423c7fe33027d7511.

Stephen, David: You signed off that commit.

Andrew: You called it a regression in October 2010.

It has been proposed to revert the commit that caused the problem.
Nothing happened.

I proposed to re-establish the old code for dmi_match()ed systems.
Without success.

Now it is proposed to re-establish the old code as a configuration option.
If nothing happens again I will propose a module parameter ;-)

Stephen, I don´t want to be a pain in the neck, and it is not my intention
to offend you by my "attitude". But I simply cannot understand why this
know regression is not fixed. The bit we talk about is documented,
and in fact it was set for a number of kernel versions unconditionally.
Nobody complained about ruined hardware or minor problems.

The systems affected are old enough that no manufacturer cares about
them, but they are still quite usable for a lot of jobs (kernel 3.3 compile
time here is below 15 minutes).

If there is a problem in the kernel and if we do know an easy solution,
that solution should be commited to the kernel, no matter what is written
in some random documentation, no matter if we could blame some
BIOS authors. That´s the way Linux works - at least I thought so.

cu,
Knut

On Wed, 21 Mar 2012 12:40:30 +0100
Knut Petersen [off-list ref] wrote:

Sky2 Wake on LAN is broken since February 2010 on a number of systems.
Yes. More than two years.

We know about the problem and the cause since October 2010
(Bugzilla bug #19492). It´s commit 87b09f1f25cd1e01d7c50bf423c7fe33027d7511.

Stephen, David: You signed off that commit.

Andrew: You called it a regression in October 2010.

It has been proposed to revert the commit that caused the problem.
Nothing happened.

I proposed to re-establish the old code for dmi_match()ed systems.
Without success.

Now it is proposed to re-establish the old code as a configuration option.
If nothing happens again I will propose a module parameter ;-)

Stephen, I don´t want to be a pain in the neck, and it is not my intention
to offend you by my "attitude". But I simply cannot understand why this
know regression is not fixed. The bit we talk about is documented,
and in fact it was set for a number of kernel versions unconditionally.
Nobody complained about ruined hardware or minor problems.

The systems affected are old enough that no manufacturer cares about
them, but they are still quite usable for a lot of jobs (kernel 3.3 compile
time here is below 15 minutes).

If there is a problem in the kernel and if we do know an easy solution,
that solution should be commited to the kernel, no matter what is written
in some random documentation, no matter if we could blame some
BIOS authors. That´s the way Linux works - at least I thought so.

cu,
Knut

Config options don't work for distro's.

Some BIOS's don't setup power management correctly (what else is
new) and don't allow use of PCI Express power control. Add a special
exception module parameter to allow working around this issue.
Based on slightly different patch by Knut Petersen.

Reported-by: Arkadiusz Miskiewicz <redacted>
Signed-off-by: Stephen Hemminger <redacted>
---
Patch against -net (ie. 3.3.0)

--- a/drivers/net/ethernet/marvell/sky2.c	2012-01-10 10:56:56.855156017 -0800
+++ b/drivers/net/ethernet/marvell/sky2.c	2012-03-21 08:25:52.400929532 -0700

@@ -95,6 +95,10 @@ static int disable_msi = 0;
 module_param(disable_msi, int, 0);
 MODULE_PARM_DESC(disable_msi, "Disable Message Signaled Interrupt (MSI)");
 
+static int legacy_pme = 0;
+module_param(legacy_pme, int, 0);
+MODULE_PARM_DESC(legacy_pme, "Legacy power management");
+
 static DEFINE_PCI_DEVICE_TABLE(sky2_id_table) = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_SYSKONNECT, 0x9000) }, /* SK-9Sxx */
 	{ PCI_DEVICE(PCI_VENDOR_ID_SYSKONNECT, 0x9E00) }, /* SK-9Exx */

@@ -867,6 +871,13 @@ static void sky2_wol_init(struct sky2_po
 	/* Disable PiG firmware */
 	sky2_write16(hw, B0_CTST, Y2_HW_WOL_OFF);
 
+	/* Needed by some broken BIOSes, use PCI rather than PCI-e for WOL */
+	if (legacy_pme) {
+		u32 reg1 = sky2_pci_read32(hw, PCI_DEV_REG1);
+		reg1 |= PCI_Y2_PME_LEGACY;
+		sky2_pci_write32(hw, PCI_DEV_REG1, reg1);
+	}
+
 	/* block receiver */
 	sky2_write8(hw, SK_REG(port, RX_GMF_CTRL_T), GMF_RST_SET);
 	sky2_read32(hw, B0_CTST);

Thanks a lot!

Some BIOS's don't setup power management correctly (what else is
new) and don't allow use of PCI Express power control. Add a special
exception module parameter to allow working around this issue.
Based on slightly different patch by Knut Petersen.

Reported-by: Arkadiusz Miskiewicz<redacted>
Signed-off-by: Stephen Hemminger<redacted>

On Wed, Mar 21, 2012 at 9:32 AM, Stephen Hemminger
[off-list ref] wrote:

Some BIOS's don't setup power management correctly (what else is
new) and don't allow use of PCI Express power control. Add a special
exception module parameter to allow working around this issue.
Based on slightly different patch by Knut Petersen.

Reported-by: Arkadiusz Miskiewicz <redacted>
Signed-off-by: Stephen Hemminger <redacted>

Is there a problem report URL you can include here?

It looks like this requires a user to figure out that he might be
suffering from this problem, then use this module parameter to work
around it.  How would a user figure that out?  Can we do it
automatically to save him the trouble?

On Wed, 21 Mar 2012 14:22:01 -0600
Bjorn Helgaas [off-list ref] wrote:

On Wed, Mar 21, 2012 at 9:32 AM, Stephen Hemminger
[off-list ref] wrote:

quoted

Some BIOS's don't setup power management correctly (what else is
new) and don't allow use of PCI Express power control. Add a special
exception module parameter to allow working around this issue.
Based on slightly different patch by Knut Petersen.

Reported-by: Arkadiusz Miskiewicz <redacted>
Signed-off-by: Stephen Hemminger <redacted>

Is there a problem report URL you can include here?

It looks like this requires a user to figure out that he might be
suffering from this problem, then use this module parameter to work
around it.  How would a user figure that out?  Can we do it
automatically to save him the trouble?

I am not a power management expert. Looks like a BIOS issue where the
BIOS has configured the device to disable power management but the
user wants to override that value. There is no method to determine
when the BIOS is broken versus when the BIOS setting is correct and
we should follow what it says.

On Wed, Mar 21, 2012 at 2:47 PM, Stephen Hemminger
[off-list ref] wrote:

On Wed, 21 Mar 2012 14:22:01 -0600
Bjorn Helgaas [off-list ref] wrote:

quoted

On Wed, Mar 21, 2012 at 9:32 AM, Stephen Hemminger
[off-list ref] wrote:

quoted

Some BIOS's don't setup power management correctly (what else is
new) and don't allow use of PCI Express power control. Add a special
exception module parameter to allow working around this issue.
Based on slightly different patch by Knut Petersen.

Reported-by: Arkadiusz Miskiewicz <redacted>
Signed-off-by: Stephen Hemminger <redacted>

Is there a problem report URL you can include here?

It looks like this requires a user to figure out that he might be
suffering from this problem, then use this module parameter to work
around it.  How would a user figure that out?  Can we do it
automatically to save him the trouble?

I am not a power management expert. Looks like a BIOS issue where the
BIOS has configured the device to disable power management but the
user wants to override that value. There is no method to determine
when the BIOS is broken versus when the BIOS setting is correct and
we should follow what it says.

I'm not a power management expert either.  I was just wondering
whether the known broken BIOSes could be encoded in a blacklist or
something, because it looks like a case where a user might have to
request help or debug the problem again before discovering this flag.

Bjorn

From: Stephen Hemminger <redacted>
Date: Wed, 21 Mar 2012 08:32:05 -0700

Some BIOS's don't setup power management correctly (what else is
new) and don't allow use of PCI Express power control. Add a special
exception module parameter to allow working around this issue.
Based on slightly different patch by Knut Petersen.

Reported-by: Arkadiusz Miskiewicz <redacted>
Signed-off-by: Stephen Hemminger <redacted>

Applied and queued up for -stable.

Am 21.03.2012 21:22, schrieb Bjorn Helgaas:

It looks like this requires a user to figure out that he might be suffering from this problem, then use this module parameter to work around it. How would a user figure that out? Can we do it automatically to save him the trouble?

It´s easy to dmi_match() known broken systems - I have dmidecode outputs of four systems that definitely need the patch.
Two ASUSTek P5* mainboards with AMI BIOSes, two AOpen i915G* mainboards with Award/Phoenix BIOSes.

cu,
  Knut

Knut Petersen wrote, a few months ago:

It´s easy to dmi_match() known broken systems - I have dmidecode
outputs of four systems that definitely need the patch.

Two ASUSTek P5* mainboards with AMI BIOSes, two AOpen i915G*
mainboards with Award/Phoenix BIOSes.

Yes, please.  Could you attach those to [1]?

Thanks,
Jonathan

[1] https://bugzilla.kernel.org/show_bug.cgi?id=19492