[PATCH] r8169: add module param for control of ASPM disable

[PATCH] r8169: add module param for control of ASPM disable

[Todd Broch]

ASPM has been disabled in this driver by default as its been
implicated in stability issues on at least one platform.  This CL adds
a  module parameter to allow control of ASPM disable.  The default
value is to enable ASPM again as its provides signficant (200mW) power
savings on the platform I tested.

Signed-off-by: Todd Broch <tbroch@chromium.org>
---
 drivers/net/r8169.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 5f838ef..05769fa0 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -671,6 +671,10 @@ struct rtl8169_private {
 #define RTL_FIRMWARE_UNKNOWN	ERR_PTR(-EAGAIN);
 };
 
+static int aspm_disable = 0;
+module_param(aspm_disable, bool, 0444);
+MODULE_PARM_DESC(aspm_disable, "Disable ASPM completely.");
+
 MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>");
 MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver");
 module_param(use_dac, int, 0);
@@ -3291,8 +3295,12 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	/* disable ASPM completely as that cause random device stop working
 	 * problems as well as full system hangs for some PCIe devices users */
-	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |
-				     PCIE_LINK_STATE_CLKPM);
+	if (aspm_disable) {
+		pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S |
+				       PCIE_LINK_STATE_L1 |
+				       PCIE_LINK_STATE_CLKPM);
+		dprintk("ASPM disabled");
+	}
 
 	/* enable device (incl. PCI PM wakeup and hotplug setup) */
 	rc = pci_enable_device(pdev);
-- 
1.7.3.1

Re: [PATCH] r8169: add module param for control of ASPM disable

[Matthew Garrett]

On Fri, Nov 11, 2011 at 03:05:55PM -0800, Todd Broch wrote:
> ASPM has been disabled in this driver by default as its been
> implicated in stability issues on at least one platform.  This CL adds
> a  module parameter to allow control of ASPM disable.  The default
> value is to enable ASPM again as its provides signficant (200mW) power
> savings on the platform I tested.

The default is then to break some hardware. That's not reasonable. 
Instead, we need to identify which chip versions are broken and limit 
the blacklist to them.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] r8169: add module param for control of ASPM disable

[Francois Romieu]

Matthew Garrett <mjg59@srcf.ucam.org> :
> On Fri, Nov 11, 2011 at 03:05:55PM -0800, Todd Broch wrote:
> > ASPM has been disabled in this driver by default as its been
> > implicated in stability issues on at least one platform.  This CL adds
> > a  module parameter to allow control of ASPM disable.  The default
> > value is to enable ASPM again as its provides signficant (200mW) power
> > savings on the platform I tested.
> 
> The default is then to break some hardware. That's not reasonable. 
> Instead, we need to identify which chip versions are broken and limit 
> the blacklist to them.

Yes, assuming it's only a device problem.

Todd, which chipset(s) (XID or such) was it tested with ?

It would be nice to know the kind of use as well (desktop / laptop / server).

Btw, what is a 'CL' ?

-- 
Ueimor

Re: [PATCH] r8169: add module param for control of ASPM disable

[Matthew Garrett]

On Tue, Nov 15, 2011 at 08:27:41AM -0800, Todd Broch wrote:
> On Sat, Nov 12, 2011 at 2:46 AM, Francois Romieu <romieu@fr.zoreil.com>wrote:
> >
> > Re-visiting the original change that disabled ASPM,
> 
> http://www.google.com/url?q=http%3A//git.kernel.org/%3Fp%3Dlinux/kernel/git/torvalds/linux-2.6.git%3Ba%3Dcommit%3Bh%3Dba04c7c93bbcb48ce880cf75b6e9dffcd79d4c7b&usg=AFQjCNFfPARrhwg-nBtW09W_n4qr1hgvdA
> 
> Led me to,
>   https://bugzilla.redhat.com/show_bug.cgi?id=642861#c4
> 
> This comment by tomi.leppikangas@, is later re-canted as a h/w issue in,
>  https://bugzilla.redhat.com/show_bug.cgi?id=642861#c9
>    'I am now pretty sure that my problems were caused by faulty hardware.
> Cpu or
>     motherboard seems to be broken, so pcie_aspm=off  didnt help for me.
> Sorry
>     about misleading info.'

Mike Khusid's issue was fixed by disabling ASPM.

> My assement from above is that ASPM was disabled prematurely and given the
> power
> savings should be re-enabled.

Power savings are great. I'm all in favour of power savings. But not 
when they break otherwise working setups.

> I'd certainly be agreeable to switching the assertion of patch to default
> being disabled.
> Unfortunately I fear that means most will never benefit from the power
> savings.

I'd recommend working with your hardware partners to figure out which 
parts are expected to work and which aren't. There's no problem with 
making this code conditional on product ID or version.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] r8169: add module param for control of ASPM disable

[Francois Romieu]

Todd Broch <tbroch@chromium.org> :
[...]
>  I've tested on a Mobile Sandybridge platform only as I presently don't have
> access to any other systems w/ the r8169 h/w.

Sorry for the lack of specificity : the XID line from the r8169 driver would
be welcome. It should look like:


[    6.315053] r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
[    6.315287] r8169 0000:03:00.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
[    6.315568] r8169 0000:03:00.0: setting latency timer to 64
[    6.315663] r8169 0000:03:00.0: irq 49 for MSI/MSI-X
[    6.315834] r8169 0000:03:00.0: eth0: RTL8168evl/8111evl at 0xffffc90000676000, 00:e0:4c:68:00:1f, XID 0c900800 IRQ 49
                                         ^^^^^^^^^^^^^^^^^^                                           ^^^^^^^^^^^^

Thanks.

-- 
Ueimor

Re: [PATCH] r8169: add module param for control of ASPM disable

[Matthew Garrett]

On Thu, Nov 17, 2011 at 01:20:46PM -0800, Todd Broch wrote:

> Looking a bit more at the driver a lot of configuration / feature choice is
> tied to mac_version.  Would this be a better solution (partial patch ...
> will submit properly later)

I'd be mildly surprised if the mac's the relevant constraint here, 
although it's possible. This patch is almost certainly safe, but if 
you're able to check with the hardware vendor you're working with that 
would be even better.

-- 
Matthew Garrett | mjg59@srcf.ucam.org