On 13.08.2021 12:11, Kai-Heng Feng wrote:

On Fri, Aug 13, 2021 at 3:39 AM Heiner Kallweit [off-list ref] wrote:

quoted

On 12.08.2021 17:53, Kai-Heng Feng wrote:

quoted

The latest vendor driver enables ASPM for more recent r8168 NICs, do the
same here to match the behavior.

In addition, pci_disable_link_state() is only used for RTL8168D/8111D in
vendor driver, also match that.

Signed-off-by: Kai-Heng Feng <redacted>
---
v2:
 - No change

 drivers/net/ethernet/realtek/r8169_main.c | 34 +++++++++++++++++------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 7ab2e841dc69..caa29e72a21a 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c

@@ -623,7 +623,7 @@ struct rtl8169_private {
      } wk;

      unsigned supports_gmii:1;
-     unsigned aspm_manageable:1;
+     unsigned aspm_supported:1;
      unsigned aspm_enabled:1;
      struct delayed_work aspm_toggle;
      struct mutex aspm_mutex;

@@ -2667,8 +2667,11 @@ static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp)

 static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable)
 {
+     if (!tp->aspm_supported)
+             return;
+
      /* Don't enable ASPM in the chip if OS can't control ASPM */
-     if (enable && tp->aspm_manageable) {
+     if (enable) {
              RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en);
              RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn);
      } else {

@@ -5284,6 +5287,21 @@ static void rtl_init_mac_address(struct rtl8169_private *tp)
      rtl_rar_set(tp, mac_addr);
 }

+static int rtl_hw_aspm_supported(struct rtl8169_private *tp)
+{
+     switch (tp->mac_version) {
+     case RTL_GIGA_MAC_VER_32 ... RTL_GIGA_MAC_VER_36:
+     case RTL_GIGA_MAC_VER_38:
+     case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_42:
+     case RTL_GIGA_MAC_VER_44 ... RTL_GIGA_MAC_VER_46:
+     case RTL_GIGA_MAC_VER_49 ... RTL_GIGA_MAC_VER_63:

This shouldn't be needed because ASPM support is announced the
standard PCI way. Max a blacklist should be needed if there are
chip versions that announce ASPM support whilst in reality they
do not support it (or support is completely broken).

So can we also remove aspm_manageable since blacklist will be used?

That's independent. What I mean is replace the whitelist with auto-
detected ASPM support and blacklist just the ones that are where
ASPM is completely unusable.
Retrieving the info about ASPM support may need a smll PCI core
extension. We need something similar to pcie_aspm_enabled(),
just exposing link->aspm_support. link->aspm_enabled may change
at runtime (sysfs link attributes).

Kai-Heng

quoted

quoted

+             return 1;
+
+     default:
+             return 0;
+     }
+}
+
 static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
      struct rtl8169_private *tp;

@@ -5315,12 +5333,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.
-      */
-     rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S |
-                                       PCIE_LINK_STATE_L1);
-     tp->aspm_manageable = !rc;
+     if (tp->mac_version == RTL_GIGA_MAC_VER_25)
+             pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S |
+                                    PCIE_LINK_STATE_L1 |
+                                    PCIE_LINK_STATE_CLKPM);
+
+     tp->aspm_supported = rtl_hw_aspm_supported(tp);

      /* enable device (incl. PCI PM wakeup and hotplug setup) */
      rc = pcim_enable_device(pdev);