[PATCH] r8169: add enable_aspm parameter

[PATCH] r8169: add enable_aspm parameter

[AceLan Kao]

We have many commits in the driver which enable and then disable ASPM
function over and over again.
   commit b75bb8a5b755 ("r8169: disable ASPM again")
   commit 0866cd15029b ("r8169: enable ASPM on RTL8106E")
   commit 94235460f9ea ("r8169: Align ASPM/CLKREQ setting function with vendor driver")
   commit aa1e7d2c31ef ("r8169: enable ASPM on RTL8168E-VL")
   commit f37658da21aa ("r8169: align ASPM entry latency setting with vendor driver")
   commit a99790bf5c7f ("r8169: Reinstate ASPM Support")
   commit 671646c151d4 ("r8169: Don't disable ASPM in the driver")
   commit 4521e1a94279 ("Revert "r8169: enable internal ASPM and clock request settings".")
   commit d64ec841517a ("r8169: enable internal ASPM and clock request settings")

This function is very important for production, and if we can't come out
a solution to make both happy, I'd suggest we add a parameter in the
driver to toggle it.

Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/net/ethernet/realtek/r8169.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index d06a61f00e78..f557cb36e2c6 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -702,10 +702,13 @@ struct rtl8169_private {
 
 typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
 
+static int enable_aspm;
 MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>");
 MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver");
 module_param_named(debug, debug.msg_enable, int, 0);
 MODULE_PARM_DESC(debug, "Debug verbosity level (0=none, ..., 16=all)");
+module_param(enable_aspm, int, 0);
+MODULE_PARM_DESC(enable_aspm, "Enable ASPM support (0 = disable, 1 = enable");
 MODULE_SOFTDEP("pre: realtek");
 MODULE_LICENSE("GPL");
 MODULE_FIRMWARE(FIRMWARE_8168D_1);
@@ -7163,10 +7166,12 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;
 
-	/* 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);
+	if (!enable_aspm) {
+		/* 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);
+	}
 
 	/* enable device (incl. PCI PM wakeup and hotplug setup) */
 	rc = pcim_enable_device(pdev);
-- 
2.17.1

Re: [PATCH] r8169: add enable_aspm parameter

[Yanjun Zhu]

On 2019/7/8 14:37, AceLan Kao wrote:
> We have many commits in the driver which enable and then disable ASPM
> function over and over again.
>     commit b75bb8a5b755 ("r8169: disable ASPM again")
>     commit 0866cd15029b ("r8169: enable ASPM on RTL8106E")
>     commit 94235460f9ea ("r8169: Align ASPM/CLKREQ setting function with vendor driver")
>     commit aa1e7d2c31ef ("r8169: enable ASPM on RTL8168E-VL")
>     commit f37658da21aa ("r8169: align ASPM entry latency setting with vendor driver")
>     commit a99790bf5c7f ("r8169: Reinstate ASPM Support")
>     commit 671646c151d4 ("r8169: Don't disable ASPM in the driver")
>     commit 4521e1a94279 ("Revert "r8169: enable internal ASPM and clock request settings".")
>     commit d64ec841517a ("r8169: enable internal ASPM and clock request settings")
>
> This function is very important for production, and if we can't come out
> a solution to make both happy, I'd suggest we add a parameter in the
> driver to toggle it.


Perhaps sysctl is better?


>
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> ---
>   drivers/net/ethernet/realtek/r8169.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index d06a61f00e78..f557cb36e2c6 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -702,10 +702,13 @@ struct rtl8169_private {
>   
>   typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
>   
> +static int enable_aspm;
>   MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>");
>   MODULE_DESCRIPTION("RealTek RTL-8169 Gigabit Ethernet driver");
>   module_param_named(debug, debug.msg_enable, int, 0);
>   MODULE_PARM_DESC(debug, "Debug verbosity level (0=none, ..., 16=all)");
> +module_param(enable_aspm, int, 0);
> +MODULE_PARM_DESC(enable_aspm, "Enable ASPM support (0 = disable, 1 = enable");
>   MODULE_SOFTDEP("pre: realtek");
>   MODULE_LICENSE("GPL");
>   MODULE_FIRMWARE(FIRMWARE_8168D_1);
> @@ -7163,10 +7166,12 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	if (rc)
>   		return rc;
>   
> -	/* 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);
> +	if (!enable_aspm) {
> +		/* 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);
> +	}
>   
>   	/* enable device (incl. PCI PM wakeup and hotplug setup) */
>   	rc = pcim_enable_device(pdev);

Re: [PATCH] r8169: add enable_aspm parameter

[Heiner Kallweit]

On 08.07.2019 08:37, AceLan Kao wrote:
> We have many commits in the driver which enable and then disable ASPM
> function over and over again.
>    commit b75bb8a5b755 ("r8169: disable ASPM again")
>    commit 0866cd15029b ("r8169: enable ASPM on RTL8106E")
>    commit 94235460f9ea ("r8169: Align ASPM/CLKREQ setting function with vendor driver")
>    commit aa1e7d2c31ef ("r8169: enable ASPM on RTL8168E-VL")
>    commit f37658da21aa ("r8169: align ASPM entry latency setting with vendor driver")
>    commit a99790bf5c7f ("r8169: Reinstate ASPM Support")
>    commit 671646c151d4 ("r8169: Don't disable ASPM in the driver")
>    commit 4521e1a94279 ("Revert "r8169: enable internal ASPM and clock request settings".")
>    commit d64ec841517a ("r8169: enable internal ASPM and clock request settings")
> 
> This function is very important for production, and if we can't come out
> a solution to make both happy, I'd suggest we add a parameter in the
> driver to toggle it.
> 
The usage of a module parameter to control ASPM is discouraged.
There have been more such attempts in the past that have been declined.

Pending with the PCI maintainers is a series adding ASPM control
via sysfs, see here: https://www.spinics.net/lists/linux-pci/msg83228.html

Also more details than just stating "it's important for production"
would have been appreciated in the commit message, e.g. which
power-savings you can achieve with ASPM on which systems.

Re: [PATCH] r8169: add enable_aspm parameter

[AceLan Kao]

Heiner Kallweit <hkallweit1@gmail.com> 於 2019年7月9日 週二 上午2:27寫道：
>
> On 08.07.2019 08:37, AceLan Kao wrote:
> > We have many commits in the driver which enable and then disable ASPM
> > function over and over again.
> >    commit b75bb8a5b755 ("r8169: disable ASPM again")
> >    commit 0866cd15029b ("r8169: enable ASPM on RTL8106E")
> >    commit 94235460f9ea ("r8169: Align ASPM/CLKREQ setting function with vendor driver")
> >    commit aa1e7d2c31ef ("r8169: enable ASPM on RTL8168E-VL")
> >    commit f37658da21aa ("r8169: align ASPM entry latency setting with vendor driver")
> >    commit a99790bf5c7f ("r8169: Reinstate ASPM Support")
> >    commit 671646c151d4 ("r8169: Don't disable ASPM in the driver")
> >    commit 4521e1a94279 ("Revert "r8169: enable internal ASPM and clock request settings".")
> >    commit d64ec841517a ("r8169: enable internal ASPM and clock request settings")
> >
> > This function is very important for production, and if we can't come out
> > a solution to make both happy, I'd suggest we add a parameter in the
> > driver to toggle it.
> >
> The usage of a module parameter to control ASPM is discouraged.
> There have been more such attempts in the past that have been declined.
>
> Pending with the PCI maintainers is a series adding ASPM control
> via sysfs, see here: https://www.spinics.net/lists/linux-pci/msg83228.html
Cool, I'll try your patches and reply on that thread.

>
> Also more details than just stating "it's important for production"
> would have been appreciated in the commit message, e.g. which
> power-savings you can achieve with ASPM on which systems.
I should use more specific wordings rather than "important for
production", thanks.

Re: [PATCH] r8169: add enable_aspm parameter

[AceLan Kao]

Hi Heiner,

I've tried and verified your PCI ASPM patches and it works well.
I've replied the patch thread and hope this can make it get some progress.

BTW, do you think we can revert commit b75bb8a5b755 ("r8169: disable
ASPM again") once the PCI ASPM patches get merged?

Best regards,
AceLan Kao.

AceLan Kao <acelan.kao@canonical.com> 於 2019年7月9日 週二 上午11:19寫道：
>
> Heiner Kallweit <hkallweit1@gmail.com> 於 2019年7月9日 週二 上午2:27寫道：
> >
> > On 08.07.2019 08:37, AceLan Kao wrote:
> > > We have many commits in the driver which enable and then disable ASPM
> > > function over and over again.
> > >    commit b75bb8a5b755 ("r8169: disable ASPM again")
> > >    commit 0866cd15029b ("r8169: enable ASPM on RTL8106E")
> > >    commit 94235460f9ea ("r8169: Align ASPM/CLKREQ setting function with vendor driver")
> > >    commit aa1e7d2c31ef ("r8169: enable ASPM on RTL8168E-VL")
> > >    commit f37658da21aa ("r8169: align ASPM entry latency setting with vendor driver")
> > >    commit a99790bf5c7f ("r8169: Reinstate ASPM Support")
> > >    commit 671646c151d4 ("r8169: Don't disable ASPM in the driver")
> > >    commit 4521e1a94279 ("Revert "r8169: enable internal ASPM and clock request settings".")
> > >    commit d64ec841517a ("r8169: enable internal ASPM and clock request settings")
> > >
> > > This function is very important for production, and if we can't come out
> > > a solution to make both happy, I'd suggest we add a parameter in the
> > > driver to toggle it.
> > >
> > The usage of a module parameter to control ASPM is discouraged.
> > There have been more such attempts in the past that have been declined.
> >
> > Pending with the PCI maintainers is a series adding ASPM control
> > via sysfs, see here: https://www.spinics.net/lists/linux-pci/msg83228.html
> Cool, I'll try your patches and reply on that thread.
>
> >
> > Also more details than just stating "it's important for production"
> > would have been appreciated in the commit message, e.g. which
> > power-savings you can achieve with ASPM on which systems.
> I should use more specific wordings rather than "important for
> production", thanks.

Re: [PATCH] r8169: add enable_aspm parameter

[Heiner Kallweit]

On 10.07.2019 09:05, AceLan Kao wrote:
> Hi Heiner,
> 
> I've tried and verified your PCI ASPM patches and it works well.
> I've replied the patch thread and hope this can make it get some progress.
> 
Thanks for the feedback!

> BTW, do you think we can revert commit b75bb8a5b755 ("r8169: disable
> ASPM again") once the PCI ASPM patches get merged?
> 
Default should remain "ASPM off" as quite a few BIOS / chip version
combinations have problems with ASPM. Interested users then can use
the new sysctl interface to switch on ASPM completely or just selected
states (e.g. L0 only).

> Best regards,
> AceLan Kao.
> 
Heiner

> AceLan Kao <acelan.kao@canonical.com> 於 2019年7月9日 週二 上午11:19寫道：
>>
>> Heiner Kallweit <hkallweit1@gmail.com> 於 2019年7月9日 週二 上午2:27寫道：
>>>
>>> On 08.07.2019 08:37, AceLan Kao wrote:
>>>> We have many commits in the driver which enable and then disable ASPM
>>>> function over and over again.
>>>>    commit b75bb8a5b755 ("r8169: disable ASPM again")
>>>>    commit 0866cd15029b ("r8169: enable ASPM on RTL8106E")
>>>>    commit 94235460f9ea ("r8169: Align ASPM/CLKREQ setting function with vendor driver")
>>>>    commit aa1e7d2c31ef ("r8169: enable ASPM on RTL8168E-VL")
>>>>    commit f37658da21aa ("r8169: align ASPM entry latency setting with vendor driver")
>>>>    commit a99790bf5c7f ("r8169: Reinstate ASPM Support")
>>>>    commit 671646c151d4 ("r8169: Don't disable ASPM in the driver")
>>>>    commit 4521e1a94279 ("Revert "r8169: enable internal ASPM and clock request settings".")
>>>>    commit d64ec841517a ("r8169: enable internal ASPM and clock request settings")
>>>>
>>>> This function is very important for production, and if we can't come out
>>>> a solution to make both happy, I'd suggest we add a parameter in the
>>>> driver to toggle it.
>>>>
>>> The usage of a module parameter to control ASPM is discouraged.
>>> There have been more such attempts in the past that have been declined.
>>>
>>> Pending with the PCI maintainers is a series adding ASPM control
>>> via sysfs, see here: https://www.spinics.net/lists/linux-pci/msg83228.html
>> Cool, I'll try your patches and reply on that thread.
>>
>>>
>>> Also more details than just stating "it's important for production"
>>> would have been appreciated in the commit message, e.g. which
>>> power-savings you can achieve with ASPM on which systems.
>> I should use more specific wordings rather than "important for
>> production", thanks.
>