* [RESEND PATCH net-next] r8169: add module param for control of ASPM disable.
@ 2018-02-14 9:02 Chunhao Lin
2018-02-14 13:40 ` Andrew Lunn
0 siblings, 1 reply; 4+ messages in thread
From: Chunhao Lin @ 2018-02-14 9:02 UTC (permalink / raw)
To: netdev; +Cc: nic_swsd, linux-kernel, Chunhao Lin
The patch is from Todd Broch <tbroch@chromium.org>.
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 <hau@realtek.com>
---
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 <netdev@vger.kernel.org>");
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH net-next] r8169: add module param for control of ASPM disable.
2018-02-14 9:02 [RESEND PATCH net-next] r8169: add module param for control of ASPM disable Chunhao Lin
@ 2018-02-14 13:40 ` Andrew Lunn
2018-02-14 17:22 ` David Miller
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Lunn @ 2018-02-14 13:40 UTC (permalink / raw)
To: Chunhao Lin; +Cc: netdev, nic_swsd, linux-kernel
On Wed, Feb 14, 2018 at 05:02:45PM +0800, Chunhao Lin wrote:
> The patch is from Todd Broch <tbroch@chromium.org>.
> 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 <hau@realtek.com>
> ---
> 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 <netdev@vger.kernel.org>");
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH net-next] r8169: add module param for control of ASPM disable.
2018-02-14 13:40 ` Andrew Lunn
@ 2018-02-14 17:22 ` David Miller
2018-02-14 17:34 ` Andrew Lunn
0 siblings, 1 reply; 4+ messages in thread
From: David Miller @ 2018-02-14 17:22 UTC (permalink / raw)
To: andrew; +Cc: hau, netdev, nic_swsd, linux-kernel
From: Andrew Lunn <andrew@lunn.ch>
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RESEND PATCH net-next] r8169: add module param for control of ASPM disable.
2018-02-14 17:22 ` David Miller
@ 2018-02-14 17:34 ` Andrew Lunn
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Lunn @ 2018-02-14 17:34 UTC (permalink / raw)
To: David Miller; +Cc: hau, netdev, nic_swsd, linux-kernel
On Wed, Feb 14, 2018 at 12:22:02PM -0500, David Miller wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-14 17:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-14 9:02 [RESEND PATCH net-next] r8169: add module param for control of ASPM disable Chunhao Lin
2018-02-14 13:40 ` Andrew Lunn
2018-02-14 17:22 ` David Miller
2018-02-14 17:34 ` Andrew Lunn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.