The patch is from Todd Broch <[email protected]>.
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.
I make some modification that let this patch only for RTL8168G and later.
Signed-off-by: Chunhao Lin <[email protected]>
---
drivers/net/ethernet/realtek/r8169.c | 73 ++++++++++++++++++++++++------------
1 file changed, 50 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 0bf7d17..87d3136 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -346,6 +346,7 @@ static const struct pci_device_id rtl8169_pci_tbl[] = {
MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
static int rx_buf_sz = 16383;
+static int aspm_disable = 0;
static int use_dac = -1;
static struct {
u32 msg_enable;
@@ -867,6 +868,8 @@ struct rtl8169_private {
MODULE_AUTHOR("Realtek and the Linux r8169 crew <[email protected]>");
MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver");
+module_param(aspm_disable, int, 0444);
+MODULE_PARM_DESC(aspm_disable, "Disable ASPM completely.");
module_param(use_dac, int, 0);
MODULE_PARM_DESC(use_dac, "Enable PCI DAC. Unsafe on 32 bit PCI slot.");
module_param_named(debug, debug.msg_enable, int, 0);
@@ -5878,6 +5881,20 @@ static void rtl_pcie_state_l2l3_enable(struct rtl8169_private *tp, bool enable)
RTL_W8(Config3, data);
}
+static void rtl_hw_internal_aspm_clkreq_enable(struct rtl8169_private *tp,
+ bool enable)
+{
+ void __iomem *ioaddr = tp->mmio_addr;
+
+ if (enable) {
+ RTL_W8(Config2, RTL_R8(Config2) | ClkReqEn);
+ RTL_W8(Config5, RTL_R8(Config5) | ASPM_en);
+ } else {
+ RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
+ RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
+ }
+}
+
#define R8168_CPCMD_QUIRK_MASK (\
EnableBist | \
Mac_dbgo_oe | \
@@ -6264,7 +6281,6 @@ static void rtl_hw_start_8168g(struct rtl8169_private *tp)
static void rtl_hw_start_8168g_1(struct rtl8169_private *tp)
{
- void __iomem *ioaddr = tp->mmio_addr;
static const struct ephy_info e_info_8168g_1[] = {
{ 0x00, 0x0000, 0x0008 },
{ 0x0c, 0x37d0, 0x0820 },
@@ -6275,14 +6291,14 @@ static void rtl_hw_start_8168g_1(struct rtl8169_private *tp)
rtl_hw_start_8168g(tp);
/* disable aspm and clock request before access ephy */
- RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
- RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
+ rtl_hw_internal_aspm_clkreq_enable(tp, false);
rtl_ephy_init(tp, e_info_8168g_1, ARRAY_SIZE(e_info_8168g_1));
+ if (!aspm_disable)
+ rtl_hw_internal_aspm_clkreq_enable(tp, true);
}
static void rtl_hw_start_8168g_2(struct rtl8169_private *tp)
{
- void __iomem *ioaddr = tp->mmio_addr;
static const struct ephy_info e_info_8168g_2[] = {
{ 0x00, 0x0000, 0x0008 },
{ 0x0c, 0x3df0, 0x0200 },
@@ -6293,14 +6309,14 @@ static void rtl_hw_start_8168g_2(struct rtl8169_private *tp)
rtl_hw_start_8168g(tp);
/* disable aspm and clock request before access ephy */
- RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
- RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
+ rtl_hw_internal_aspm_clkreq_enable(tp, false);
rtl_ephy_init(tp, e_info_8168g_2, ARRAY_SIZE(e_info_8168g_2));
+ if (!aspm_disable)
+ rtl_hw_internal_aspm_clkreq_enable(tp, true);
}
static void rtl_hw_start_8411_2(struct rtl8169_private *tp)
{
- void __iomem *ioaddr = tp->mmio_addr;
static const struct ephy_info e_info_8411_2[] = {
{ 0x00, 0x0000, 0x0008 },
{ 0x0c, 0x3df0, 0x0200 },
@@ -6312,9 +6328,10 @@ static void rtl_hw_start_8411_2(struct rtl8169_private *tp)
rtl_hw_start_8168g(tp);
/* disable aspm and clock request before access ephy */
- RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
- RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
+ rtl_hw_internal_aspm_clkreq_enable(tp, false);
rtl_ephy_init(tp, e_info_8411_2, ARRAY_SIZE(e_info_8411_2));
+ if (!aspm_disable)
+ rtl_hw_internal_aspm_clkreq_enable(tp, true);
}
static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
@@ -6333,8 +6350,7 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
};
/* disable aspm and clock request before access ephy */
- RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
- RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
+ rtl_hw_internal_aspm_clkreq_enable(tp, false);
rtl_ephy_init(tp, e_info_8168h_1, ARRAY_SIZE(e_info_8168h_1));
RTL_W32(TxConfig, RTL_R32(TxConfig) | TXCFG_AUTO_FIFO);
@@ -6413,6 +6429,9 @@ static void rtl_hw_start_8168h_1(struct rtl8169_private *tp)
r8168_mac_ocp_write(tp, 0xe63e, 0x0000);
r8168_mac_ocp_write(tp, 0xc094, 0x0000);
r8168_mac_ocp_write(tp, 0xc09e, 0x0000);
+
+ if (!aspm_disable)
+ rtl_hw_internal_aspm_clkreq_enable(tp, true);
}
static void rtl_hw_start_8168ep(struct rtl8169_private *tp)
@@ -6458,7 +6477,6 @@ static void rtl_hw_start_8168ep(struct rtl8169_private *tp)
static void rtl_hw_start_8168ep_1(struct rtl8169_private *tp)
{
- void __iomem *ioaddr = tp->mmio_addr;
static const struct ephy_info e_info_8168ep_1[] = {
{ 0x00, 0xffff, 0x10ab },
{ 0x06, 0xffff, 0xf030 },
@@ -6468,11 +6486,13 @@ static void rtl_hw_start_8168ep_1(struct rtl8169_private *tp)
};
/* disable aspm and clock request before access ephy */
- RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
- RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
+ rtl_hw_internal_aspm_clkreq_enable(tp, false);
rtl_ephy_init(tp, e_info_8168ep_1, ARRAY_SIZE(e_info_8168ep_1));
rtl_hw_start_8168ep(tp);
+
+ if (!aspm_disable)
+ rtl_hw_internal_aspm_clkreq_enable(tp, true);
}
static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp)
@@ -6485,14 +6505,16 @@ static void rtl_hw_start_8168ep_2(struct rtl8169_private *tp)
};
/* disable aspm and clock request before access ephy */
- RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
- RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
+ rtl_hw_internal_aspm_clkreq_enable(tp, false);
rtl_ephy_init(tp, e_info_8168ep_2, ARRAY_SIZE(e_info_8168ep_2));
rtl_hw_start_8168ep(tp);
RTL_W8(DLLPR, RTL_R8(DLLPR) & ~PFM_EN);
RTL_W8(MISC_1, RTL_R8(MISC_1) & ~PFM_D3COLD_EN);
+
+ if (!aspm_disable)
+ rtl_hw_internal_aspm_clkreq_enable(tp, true);
}
static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
@@ -6507,8 +6529,7 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
};
/* disable aspm and clock request before access ephy */
- RTL_W8(Config2, RTL_R8(Config2) & ~ClkReqEn);
- RTL_W8(Config5, RTL_R8(Config5) & ~ASPM_en);
+ rtl_hw_internal_aspm_clkreq_enable(tp, false);
rtl_ephy_init(tp, e_info_8168ep_3, ARRAY_SIZE(e_info_8168ep_3));
rtl_hw_start_8168ep(tp);
@@ -6528,6 +6549,9 @@ static void rtl_hw_start_8168ep_3(struct rtl8169_private *tp)
data = r8168_mac_ocp_read(tp, 0xe860);
data |= 0x0080;
r8168_mac_ocp_write(tp, 0xe860, data);
+
+ if (!aspm_disable)
+ rtl_hw_internal_aspm_clkreq_enable(tp, true);
}
static void rtl_hw_start_8168(struct net_device *dev)
@@ -8425,11 +8449,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
mii->reg_num_mask = 0x1f;
mii->supports_gmii = !!(cfg->features & RTL_FEATURE_GMII);
- /* 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);
-
/* enable device (incl. PCI PM wakeup and hotplug setup) */
rc = pcim_enable_device(pdev);
if (rc < 0) {
@@ -8476,6 +8495,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
/* Identify chip attached to board */
rtl8169_get_mac_version(tp, dev, cfg->default_ver);
+ /* disable ASPM completely as that cause random device stop working
+ * problems as well as full system hangs for some PCIe devices users */
+ if (aspm_disable || tp->mac_version < RTL_GIGA_MAC_VER_40) {
+ pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |
+ PCIE_LINK_STATE_CLKPM);
+ netif_info(tp, probe, dev, "ASPM disabled\n");
+ }
+
tp->cp_cmd = 0;
if ((sizeof(dma_addr_t) > 4) &&
--
2.7.4
On Wed, Feb 14, 2018 at 05:02:45PM +0800, Chunhao Lin wrote:
> The patch is from Todd Broch <[email protected]>.
> 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.
>
> I make some modification that let this patch only for RTL8168G and later.
>
> Signed-off-by: Chunhao Lin <[email protected]>
> ---
> drivers/net/ethernet/realtek/r8169.c | 73 ++++++++++++++++++++++++------------
> 1 file changed, 50 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 0bf7d17..87d3136 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -346,6 +346,7 @@ static const struct pci_device_id rtl8169_pci_tbl[] = {
> MODULE_DEVICE_TABLE(pci, rtl8169_pci_tbl);
>
> static int rx_buf_sz = 16383;
> +static int aspm_disable = 0;
> static int use_dac = -1;
> static struct {
> u32 msg_enable;
> @@ -867,6 +868,8 @@ struct rtl8169_private {
>
> MODULE_AUTHOR("Realtek and the Linux r8169 crew <[email protected]>");
> MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver");
> +module_param(aspm_disable, int, 0444);
> +MODULE_PARM_DESC(aspm_disable, "Disable ASPM completely.");
Hi Chunhao
linux/drivers$ grep -ir aspm * | grep MODULE_
gpu/drm/amd/amdgpu/amdgpu_drv.c:MODULE_PARM_DESC(aspm, "ASPM support (1 = enable, 0 = disable, -1 = auto)");
gpu/drm/radeon/radeon_drv.c:MODULE_PARM_DESC(aspm, "ASPM support (1 = enable, 0 = disable, -1 = auto)");
infiniband/hw/hfi1/pcie.c:MODULE_PARM_DESC(aspm, "PCIe ASPM: 0: disable, 1: enable, 2: dynamic");
net/wireless/realtek/rtlwifi/rtl8192ee/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
net/wireless/realtek/rtlwifi/rtl8188ee/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
net/wireless/realtek/rtlwifi/rtl8821ae/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
net/wireless/realtek/rtlwifi/rtl8192se/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
net/wireless/realtek/rtlwifi/rtl8192ce/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
net/wireless/realtek/rtlwifi/rtl8723be/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
net/wireless/realtek/rtlwifi/rtl8192de/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
net/wireless/realtek/rtlwifi/rtl8723ae/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
staging/rts5208/rtsx.c:MODULE_PARM_DESC(aspm_l0s_l1_en, "enable device aspm");
staging/rtlwifi/rtl8822be/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
This patch seems to have the exact opposite of everybody else already
does.
Maybe you can follow the AMD example, and default to -1, since you are
proposing to mostly have it enabled, but disabled in one case?
Andrew
From: Andrew Lunn <[email protected]>
Date: Wed, 14 Feb 2018 14:40:40 +0100
> linux/drivers$ grep -ir aspm * | grep MODULE_
> gpu/drm/amd/amdgpu/amdgpu_drv.c:MODULE_PARM_DESC(aspm, "ASPM support (1 = enable, 0 = disable, -1 = auto)");
> gpu/drm/radeon/radeon_drv.c:MODULE_PARM_DESC(aspm, "ASPM support (1 = enable, 0 = disable, -1 = auto)");
> infiniband/hw/hfi1/pcie.c:MODULE_PARM_DESC(aspm, "PCIe ASPM: 0: disable, 1: enable, 2: dynamic");
> net/wireless/realtek/rtlwifi/rtl8192ee/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> net/wireless/realtek/rtlwifi/rtl8188ee/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> net/wireless/realtek/rtlwifi/rtl8821ae/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> net/wireless/realtek/rtlwifi/rtl8192se/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> net/wireless/realtek/rtlwifi/rtl8192ce/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> net/wireless/realtek/rtlwifi/rtl8723be/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> net/wireless/realtek/rtlwifi/rtl8192de/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> net/wireless/realtek/rtlwifi/rtl8723ae/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> staging/rts5208/rtsx.c:MODULE_PARM_DESC(aspm_l0s_l1_en, "enable device aspm");
> staging/rtlwifi/rtl8822be/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
>
> This patch seems to have the exact opposite of everybody else already
> does.
>
> Maybe you can follow the AMD example, and default to -1, since you are
> proposing to mostly have it enabled, but disabled in one case?
This is just another good reminder of why module parameters are a
terrible user experience for just about anything device configuration
related.
"Let's add knob X using a module parameter!"
Then a dozen or so other drivers copy the same thing with subtly, or
not so subtly, different behaviors, defaults, and semantics.
For users, this leads to misery.
On Wed, Feb 14, 2018 at 12:22:02PM -0500, David Miller wrote:
> From: Andrew Lunn <[email protected]>
> Date: Wed, 14 Feb 2018 14:40:40 +0100
>
> > linux/drivers$ grep -ir aspm * | grep MODULE_
> > gpu/drm/amd/amdgpu/amdgpu_drv.c:MODULE_PARM_DESC(aspm, "ASPM support (1 = enable, 0 = disable, -1 = auto)");
> > gpu/drm/radeon/radeon_drv.c:MODULE_PARM_DESC(aspm, "ASPM support (1 = enable, 0 = disable, -1 = auto)");
> > infiniband/hw/hfi1/pcie.c:MODULE_PARM_DESC(aspm, "PCIe ASPM: 0: disable, 1: enable, 2: dynamic");
> > net/wireless/realtek/rtlwifi/rtl8192ee/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> > net/wireless/realtek/rtlwifi/rtl8188ee/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> > net/wireless/realtek/rtlwifi/rtl8821ae/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> > net/wireless/realtek/rtlwifi/rtl8192se/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> > net/wireless/realtek/rtlwifi/rtl8192ce/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> > net/wireless/realtek/rtlwifi/rtl8723be/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> > net/wireless/realtek/rtlwifi/rtl8192de/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> > net/wireless/realtek/rtlwifi/rtl8723ae/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> > staging/rts5208/rtsx.c:MODULE_PARM_DESC(aspm_l0s_l1_en, "enable device aspm");
> > staging/rtlwifi/rtl8822be/sw.c:MODULE_PARM_DESC(aspm, "Set to 1 to enable ASPM (default 1)\n");
> >
> > This patch seems to have the exact opposite of everybody else already
> > does.
> >
> > Maybe you can follow the AMD example, and default to -1, since you are
> > proposing to mostly have it enabled, but disabled in one case?
>
> This is just another good reminder of why module parameters are a
> terrible user experience for just about anything device configuration
> related.
Agreed.
I took a look at see if there was some other way to do this. Maybe the
pcie core had a callback or something. Nope.
> For users, this leads to misery.
At least in this case, the misery is mostly limited to realtek
users. Nearly everybody else seems to get it right and not need such a
hack.
Andrew