[PATCH 0/4][SRU][OEM-OSP1-B] enable realtek ethernet device ASPM function

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/4][SRU][OEM-OSP1-B] enable realtek ethernet device ASPM function

AceLan Kao
[Impact]
The PC state stays at PC3 if realtek ethernet doesn't enable ASPM.

[Fix]
Discussed with upstream and they would like to use sysfs to toggle the
ASPM link states, but the patches didn't get merged yet, so we merger
them as SAUCE patches.

[Test]
Verified on machines with realtek ethernet device, the ethernet works
well and the system can enter PC10.

[Regression Potential]
High. From upstream maintainer, enable realtek ethernet ASPM may lead to
some serious issue, so regression is expected. Those regression should
come from old realtek chips, we'll make sure all new platforms with
realtek NIC have no any issues.

AceLan Kao (1):
  UBUNTU: SAUCE: Revert "r8169: disable ASPM again"

Heiner Kallweit (3):
  UBUNTU: SAUCE: PCI/ASPM: add L1 sub-state support to
    pci_disable_link_state
  UBUNTU: SAUCE: PCI/ASPM: allow to re-enable Clock PM
  UBUNTU: SAUCE: PCI/ASPM: add sysfs attribute for controlling ASPM

 Documentation/ABI/testing/sysfs-bus-pci |  13 ++
 drivers/net/ethernet/realtek/r8169.c    |   6 -
 drivers/pci/pci.h                       |   8 +-
 drivers/pci/pcie/aspm.c                 | 211 ++++++++++++++++++++++--
 include/linux/pci-aspm.h                |   8 +-
 5 files changed, 219 insertions(+), 27 deletions(-)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/4][SRU][OEM-OSP1-B] UBUNTU: SAUCE: PCI/ASPM: add L1 sub-state support to pci_disable_link_state

AceLan Kao
From: Heiner Kallweit <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1836030

Link: https://www.spinics.net/lists/linux-pci/msg83229.html

Add support for disabling states L1.1 and L1.2 to pci_disable_link_state.

Signed-off-by: Heiner Kallweit <[hidden email]>
Signed-off-by: AceLan Kao <[hidden email]>
---
 drivers/pci/pcie/aspm.c  | 13 ++++++++++---
 include/linux/pci-aspm.h |  8 +++++---
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 727e3c1ef9a4..a90318a69ce5 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -35,9 +35,9 @@
 #define ASPM_STATE_L1_1_PCIPM (0x20) /* PCI PM L1.1 state */
 #define ASPM_STATE_L1_2_PCIPM (0x40) /* PCI PM L1.2 state */
 #define ASPM_STATE_L1_SS_PCIPM (ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1_2_PCIPM)
+#define ASPM_STATE_L1_1_MASK (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM)
 #define ASPM_STATE_L1_2_MASK (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM)
-#define ASPM_STATE_L1SS (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\
- ASPM_STATE_L1_2_MASK)
+#define ASPM_STATE_L1SS (ASPM_STATE_L1_1_MASK | ASPM_STATE_L1_2_MASK)
 #define ASPM_STATE_L0S (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW)
 #define ASPM_STATE_ALL (ASPM_STATE_L0S | ASPM_STATE_L1 | \
  ASPM_STATE_L1SS)
@@ -1079,8 +1079,15 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
  link = parent->link_state;
  if (state & PCIE_LINK_STATE_L0S)
  link->aspm_disable |= ASPM_STATE_L0S;
- if (state & PCIE_LINK_STATE_L1)
+ if (state & PCIE_LINK_STATE_L1) {
  link->aspm_disable |= ASPM_STATE_L1;
+ /* sub-states require L1 */
+ link->aspm_disable |= ASPM_STATE_L1SS;
+ }
+ if (state & PCIE_LINK_STATE_L1_1)
+ link->aspm_disable |= ASPM_STATE_L1_1_MASK;
+ if (state & PCIE_LINK_STATE_L1_2)
+ link->aspm_disable |= ASPM_STATE_L1_2_MASK;
  pcie_config_aspm_link(link, policy_to_aspm_state(link));
 
  if (state & PCIE_LINK_STATE_CLKPM) {
diff --git a/include/linux/pci-aspm.h b/include/linux/pci-aspm.h
index df28af5cef21..e66c3e3d8a93 100644
--- a/include/linux/pci-aspm.h
+++ b/include/linux/pci-aspm.h
@@ -19,9 +19,11 @@
 
 #include <linux/pci.h>
 
-#define PCIE_LINK_STATE_L0S 1
-#define PCIE_LINK_STATE_L1 2
-#define PCIE_LINK_STATE_CLKPM 4
+#define PCIE_LINK_STATE_L0S BIT(0)
+#define PCIE_LINK_STATE_L1 BIT(1)
+#define PCIE_LINK_STATE_CLKPM BIT(2)
+#define PCIE_LINK_STATE_L1_1 BIT(3)
+#define PCIE_LINK_STATE_L1_2 BIT(4)
 
 #ifdef CONFIG_PCIEASPM
 void pci_disable_link_state(struct pci_dev *pdev, int state);
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/4][SRU][OEM-OSP1-B] UBUNTU: SAUCE: PCI/ASPM: allow to re-enable Clock PM

AceLan Kao
In reply to this post by AceLan Kao
From: Heiner Kallweit <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1836030

Link: https://www.spinics.net/lists/linux-pci/msg83230.html

So far Clock PM can't be re-enabled once it has been disabled with a
call to pci_disable_link_state(). Reason is that clkpm_capable is
reset. Change this by adding a clkpm_disable field similar to
aspm_disable.

Signed-off-by: Heiner Kallweit <[hidden email]>
Signed-off-by: AceLan Kao <[hidden email]>
---
 drivers/pci/pcie/aspm.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a90318a69ce5..93f581dc47df 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -65,6 +65,7 @@ struct pcie_link_state {
  u32 clkpm_capable:1; /* Clock PM capable? */
  u32 clkpm_enabled:1; /* Current Clock PM state */
  u32 clkpm_default:1; /* Default Clock PM state by BIOS */
+ u32 clkpm_disable:1; /* Clock PM disabled */
 
  /* Exit latencies */
  struct aspm_latency latency_up; /* Upstream direction exit latency */
@@ -162,8 +163,11 @@ static void pcie_set_clkpm_nocheck(struct pcie_link_state *link, int enable)
 
 static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
 {
- /* Don't enable Clock PM if the link is not Clock PM capable */
- if (!link->clkpm_capable)
+ /*
+ * Don't enable Clock PM if the link is not Clock PM capable
+ * or Clock PM is disabled
+ */
+ if (!link->clkpm_capable || link->clkpm_disable)
  enable = 0;
  /* Need nothing if the specified equals to current state */
  if (link->clkpm_enabled == enable)
@@ -193,7 +197,8 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
  }
  link->clkpm_enabled = enabled;
  link->clkpm_default = enabled;
- link->clkpm_capable = (blacklist) ? 0 : capable;
+ link->clkpm_capable = capable;
+ link->clkpm_disable = blacklist ? 1 : 0;
 }
 
 /*
@@ -1090,10 +1095,9 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
  link->aspm_disable |= ASPM_STATE_L1_2_MASK;
  pcie_config_aspm_link(link, policy_to_aspm_state(link));
 
- if (state & PCIE_LINK_STATE_CLKPM) {
- link->clkpm_capable = 0;
- pcie_set_clkpm(link, 0);
- }
+ if (state & PCIE_LINK_STATE_CLKPM)
+ link->clkpm_disable = 1;
+ pcie_set_clkpm(link, policy_to_clkpm_state(link));
  mutex_unlock(&aspm_lock);
  if (sem)
  up_read(&pci_bus_sem);
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/4][SRU][OEM-OSP1-B] UBUNTU: SAUCE: PCI/ASPM: add sysfs attribute for controlling ASPM

AceLan Kao
In reply to this post by AceLan Kao
From: Heiner Kallweit <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1836030

Link: https://www.spinics.net/lists/linux-pci/msg83231.html

Background of this extension is a problem with the r8169 network driver.
Several combinations of board chipsets and network chip versions have
problems if ASPM is enabled, therefore we have to disable ASPM per default.
However especially on notebooks ASPM can provide significant power-saving,
therefore we want to give users the option to enable ASPM. With the new sysfs
attribute users can control which ASPM link-states are enabled/disabled.

Signed-off-by: Heiner Kallweit <[hidden email]>
Signed-off-by: AceLan Kao <[hidden email]>
---
 Documentation/ABI/testing/sysfs-bus-pci |  13 ++
 drivers/pci/pci.h                       |   8 +-
 drivers/pci/pcie/aspm.c                 | 180 +++++++++++++++++++++++-
 3 files changed, 193 insertions(+), 8 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 8bfee557e50e..38fe358dee12 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -347,3 +347,16 @@ Description:
  If the device has any Peer-to-Peer memory registered, this
         file contains a '1' if the memory has been published for
  use outside the driver that owns the device.
+
+What: /sys/bus/pci/devices/.../power/aspm_link_states
+Date: May 2019
+Contact: Heiner Kallweit <[hidden email]>
+Description:
+ If ASPM is supported for an endpoint, then this file can be
+ used to enable / disable link states. A link state
+ displayed in brackets is enabled, otherwise it's disabled.
+ To control link states (case insensitive):
+ +state : enables a supported state
+ -state : disables a state
+ none : disables all link states
+ all : enables all supported link states
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 224d88634115..4093b1154006 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -498,17 +498,13 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
 void pcie_aspm_exit_link_state(struct pci_dev *pdev);
 void pcie_aspm_pm_state_change(struct pci_dev *pdev);
 void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
+void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
+void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
 #else
 static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
 static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
 static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
 static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
-#endif
-
-#ifdef CONFIG_PCIEASPM_DEBUG
-void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
-void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
-#else
 static inline void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev) { }
 static inline void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev) { }
 #endif
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 93f581dc47df..210a34f75e0d 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -42,6 +42,8 @@
 #define ASPM_STATE_ALL (ASPM_STATE_L0S | ASPM_STATE_L1 | \
  ASPM_STATE_L1SS)
 
+static const char power_group[] = "power";
+
 struct aspm_latency {
  u32 l0s; /* L0s latency (nsec) */
  u32 l1; /* L1 latency (nsec) */
@@ -1236,38 +1238,212 @@ static ssize_t clk_ctl_store(struct device *dev,
 
 static DEVICE_ATTR_RW(link_state);
 static DEVICE_ATTR_RW(clk_ctl);
+#endif
+
+struct aspm_sysfs_state {
+ const char *name;
+ int mask;
+};
+
+static const struct aspm_sysfs_state aspm_sysfs_states[] = {
+ { "L0S", ASPM_STATE_L0S },
+ { "L1", ASPM_STATE_L1 },
+ { "L1.1", ASPM_STATE_L1_1_MASK },
+ { "L1.2", ASPM_STATE_L1_2_MASK },
+};
+
+static struct pcie_link_state *aspm_get_parent_link(struct pci_dev *pdev)
+{
+ struct pci_dev *parent = pdev->bus->self;
+
+ if (pdev->has_secondary_link)
+ parent = pdev;
+
+ return parent ? parent->link_state : NULL;
+}
+
+static bool pcie_check_aspm_endpoint(struct pci_dev *pdev)
+{
+ struct pcie_link_state *link;
+
+ if (pci_pcie_type(pdev) != PCI_EXP_TYPE_ENDPOINT)
+ return false;
+
+ link = aspm_get_parent_link(pdev);
+
+ return link && link->aspm_support;
+}
+
+static ssize_t aspm_link_states_show(struct device *dev,
+     struct device_attribute *attr, char *buf)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ struct pcie_link_state *link;
+ int len = 0, i;
+
+ link = aspm_get_parent_link(pdev);
+ if (!link)
+ return -EOPNOTSUPP;
+
+ mutex_lock(&aspm_lock);
+
+ for (i = 0; i < ARRAY_SIZE(aspm_sysfs_states); i++) {
+ const struct aspm_sysfs_state *st = aspm_sysfs_states + i;
+
+ if (link->aspm_enabled & st->mask)
+ len += scnprintf(buf + len, PAGE_SIZE - len, "[%s] ",
+ st->name);
+ else
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%s ",
+ st->name);
+ }
+
+ if (link->clkpm_enabled)
+ len += scnprintf(buf + len, PAGE_SIZE - len, "[CLKPM] ");
+ else
+ len += scnprintf(buf + len, PAGE_SIZE - len, "CLKPM ");
+
+ mutex_unlock(&aspm_lock);
+
+ len += scnprintf(buf + len, PAGE_SIZE - len, "\n");
+
+ return len;
+}
+
+static ssize_t aspm_link_states_store(struct device *dev,
+      struct device_attribute *attr,
+      const char *buf, size_t len)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ struct pcie_link_state *link;
+ char *buftmp = (char *)buf, *tok;
+ unsigned int disable_aspm, disable_clkpm;
+ bool first = true, add;
+ int err = 0, i;
+
+ if (aspm_disabled)
+ return -EPERM;
+
+ link = aspm_get_parent_link(pdev);
+ if (!link)
+ return -EOPNOTSUPP;
+
+ down_read(&pci_bus_sem);
+ mutex_lock(&aspm_lock);
+
+ disable_aspm = link->aspm_disable;
+ disable_clkpm = link->clkpm_disable;
+
+ while ((tok = strsep(&buftmp, " \n")) != NULL) {
+ bool found = false;
+
+ if (!*tok)
+ continue;
+
+ if (first) {
+ if (!strcasecmp(tok, "none")) {
+ disable_aspm = ASPM_STATE_ALL;
+ disable_clkpm = 1;
+ break;
+ }
+ if (!strcasecmp(tok, "all")) {
+ disable_aspm = 0;
+ disable_clkpm = 0;
+ break;
+ }
+ first = false;
+ }
+
+ if (*tok != '+' && *tok != '-') {
+ err = -EINVAL;
+ goto out;
+ }
+
+ add = *tok++ == '+';
+
+ for (i = 0; i < ARRAY_SIZE(aspm_sysfs_states); i++) {
+ const struct aspm_sysfs_state *st =
+ aspm_sysfs_states + i;
+
+ if (!strcasecmp(tok, st->name)) {
+ if (add)
+ disable_aspm &= ~st->mask;
+ else
+ disable_aspm |= st->mask;
+ found = true;
+ break;
+ }
+ }
+
+ if (!found && !strcasecmp(tok, "clkpm")) {
+ disable_clkpm = add ? 0 : 1;
+ found = true;
+ }
+
+ if (!found) {
+ err = -EINVAL;
+ goto out;
+ }
+ }
+
+ if (disable_aspm & ASPM_STATE_L1)
+ disable_aspm |= ASPM_STATE_L1SS;
+
+ link->aspm_disable = disable_aspm;
+ link->clkpm_disable = disable_clkpm;
+
+ pcie_config_aspm_link(link, policy_to_aspm_state(link));
+ pcie_set_clkpm(link, policy_to_clkpm_state(link));
+out:
+ mutex_unlock(&aspm_lock);
+ up_read(&pci_bus_sem);
+
+ return err ?: len;
+}
+
+static DEVICE_ATTR_RW(aspm_link_states);
 
-static char power_group[] = "power";
 void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev)
 {
  struct pcie_link_state *link_state = pdev->link_state;
 
+ if (pcie_check_aspm_endpoint(pdev))
+ sysfs_add_file_to_group(&pdev->dev.kobj,
+ &dev_attr_aspm_link_states.attr, power_group);
+
  if (!link_state)
  return;
 
+#ifdef CONFIG_PCIEASPM_DEBUG
  if (link_state->aspm_support)
  sysfs_add_file_to_group(&pdev->dev.kobj,
  &dev_attr_link_state.attr, power_group);
  if (link_state->clkpm_capable)
  sysfs_add_file_to_group(&pdev->dev.kobj,
  &dev_attr_clk_ctl.attr, power_group);
+#endif
 }
 
 void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev)
 {
  struct pcie_link_state *link_state = pdev->link_state;
 
+ if (pcie_check_aspm_endpoint(pdev))
+ sysfs_remove_file_from_group(&pdev->dev.kobj,
+ &dev_attr_aspm_link_states.attr, power_group);
+
  if (!link_state)
  return;
 
+#ifdef CONFIG_PCIEASPM_DEBUG
  if (link_state->aspm_support)
  sysfs_remove_file_from_group(&pdev->dev.kobj,
  &dev_attr_link_state.attr, power_group);
  if (link_state->clkpm_capable)
  sysfs_remove_file_from_group(&pdev->dev.kobj,
  &dev_attr_clk_ctl.attr, power_group);
-}
 #endif
+}
 
 static int __init pcie_aspm_disable(char *str)
 {
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 4/4][SRU][OEM-OSP1-B] UBUNTU: SAUCE: Revert "r8169: disable ASPM again"

AceLan Kao
In reply to this post by AceLan Kao
BugLink: https://bugs.launchpad.net/bugs/1836030

This reverts commit 8b60fe9c63d2b1dc55412bfbc7e4a899063f61c3.

PCI/ASPM have been merged, so we rely on the PCI ASPM setting from BIOS,
and don't need to enable/disable ASPM function on r8169 driver.

Signed-off-by: AceLan Kao <[hidden email]>
---
 drivers/net/ethernet/realtek/r8169.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 365cddbfc684..f294c5c9c206 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -28,7 +28,6 @@
 #include <linux/pm_runtime.h>
 #include <linux/firmware.h>
 #include <linux/prefetch.h>
-#include <linux/pci-aspm.h>
 #include <linux/ipv6.h>
 #include <net/ip6_checksum.h>
 
@@ -7225,11 +7224,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
  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);
-
  /* enable device (incl. PCI PM wakeup and hotplug setup) */
  rc = pcim_enable_device(pdev);
  if (rc < 0) {
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED Re: [PATCH 0/4][SRU][OEM-OSP1-B] enable realtek ethernet device ASPM function

Timo Aaltonen-6
In reply to this post by AceLan Kao
On 10.7.2019 12.40, AceLan Kao wrote:
> [Impact]
> The PC state stays at PC3 if realtek ethernet doesn't enable ASPM.
>
> [Fix]
> Discussed with upstream and they would like to use sysfs to toggle the
> ASPM link states, but the patches didn't get merged yet, so we merger
> them as SAUCE patches.

applied to osp1 oem-next, thanks


--
t

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team