On Tue, Oct 08, 2019 at 05:10:40PM -0500, Bjorn Helgaas wrote:
> On Sat, Oct 05, 2019 at 02:02:29PM +0200, Heiner Kallweit wrote:
> > 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 attributes users can control which ASPM
> > link-states are disabled.
> >
> > v2:
> > - use a dedicated sysfs attribute per link state
> > - allow separate control of ASPM and PCI PM L1 sub-states
> >
> > v3:
> > - patch 3: statically allocate the attribute group
> > - patch 3: replace snprintf with printf
> > - add patch 4
> >
> > v4:
> > - patch 3: add call to sysfs_update_group because is_visible callback
> > returns false always at file creation time
> > - patch 3: simplify code a little
> >
> > v5:
> > - rebased to latest pci/next
> >
> > v6:
> > - patch 3: consider several review comments from Bjorn
> > - patch 4: add discussion link to commit message
> >
> > v7:
> > - Move adding pcie_aspm_get_link() to separate patch 3
> > - patch 4: change group name from aspm to link_pm
> > - patch 4: control visibility of attributes individually
> >
> > Heiner Kallweit (5):
> > PCI/ASPM: add L1 sub-state support to pci_disable_link_state
> > PCI/ASPM: allow to re-enable Clock PM
> > PCI/ASPM: Add and use helper pcie_aspm_get_link
> > PCI/ASPM: Add sysfs attributes for controlling ASPM link states
> > PCI/ASPM: Remove Kconfig option PCIEASPM_DEBUG and related code
> >
> > Documentation/ABI/testing/sysfs-bus-pci | 14 ++
> > drivers/pci/pci-sysfs.c | 6 +-
> > drivers/pci/pci.h | 12 +-
> > drivers/pci/pcie/Kconfig | 7 -
> > drivers/pci/pcie/aspm.c | 252 ++++++++++++++++--------
> > include/linux/pci.h | 10 +-
> > 6 files changed, 199 insertions(+), 102 deletions(-)
>
> I applied these to pci/aspm for v5.5. Thank you very much for all the
> work you put into this!
>
> There are a couple questions that are still open, but I have no
> problem if we want to make minor tweaks before the merge window opens.
To resolve these open questions, I propose the diff below, which:
- Makes pcie_aspm_get_link() work only when called for an Upstream
Port (Endpoint, Switch Upstream Port, or other component at the
downstream end of a Link). I don't think there's any caller that
needs to supply the upstream end.
- Makes pcie_aspm_get_link() check that both ends are PCIe devices.
This might be overkill, but we can't rely on the PCI topology
being "correct", e.g., we have to deal gracefully with a
virtualization or similar scenario where a bridge is PCI and the
child is PCIe. In that case, we shouldn't try to manage ASPM, so
we don't need a link_state, but I couldn't quite convince myself
that pcie_aspm_init_link_state() handles these cases.
- Removes the aspm_lock from the sysfs show functions. Per the
discussion with Rafael, I don't think it's necessary there:
https://lore.kernel.org/r/20191007223428.GA72605@xxxxxxxxxx
I didn't remove it from the store functions because they do ASPM
reconfiguration and I didn't try to figure out the locking there.
Let me know what you think about this. If it looks right, I'll just
squash these changes into the relevant patches.
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 200ec994299d..83a169a196f5 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1078,24 +1078,22 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev)
static struct pcie_link_state *pcie_aspm_get_link(struct pci_dev *pdev)
{
- struct pci_dev *upstream;
+ struct pci_dev *bridge;
- if (pcie_downstream_port(pdev))
- upstream = pdev;
- else
- upstream = pci_upstream_bridge(pdev);
+ if (!pci_is_pcie(pdev))
+ return NULL;
+
+ bridge = pci_upstream_bridge(pdev);
+ if (!bridge || !pci_is_pcie(bridge))
+ return NULL;
- return upstream ? upstream->link_state : NULL;
+ return bridge->link_state;
}
static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
{
- struct pcie_link_state *link;
-
- if (!pci_is_pcie(pdev))
- return 0;
+ struct pcie_link_state *link = pcie_aspm_get_link(pdev);
- link = pcie_aspm_get_link(pdev);
if (!link)
return -EINVAL;
/*
@@ -1225,16 +1223,9 @@ static ssize_t aspm_attr_show_common(struct device *dev,
char *buf, u8 state)
{
struct pci_dev *pdev = to_pci_dev(dev);
- struct pcie_link_state *link;
- bool enabled;
-
- link = pcie_aspm_get_link(pdev);
-
- mutex_lock(&aspm_lock);
- enabled = link->aspm_enabled & state;
- mutex_unlock(&aspm_lock);
+ struct pcie_link_state *link = pcie_aspm_get_link(pdev);
- return sprintf(buf, "%d\n", enabled ? 1 : 0);
+ return sprintf(buf, "%d\n", (link->aspm_enabled & state) ? 1 : 0);
}
static ssize_t aspm_attr_store_common(struct device *dev,
@@ -1242,11 +1233,9 @@ static ssize_t aspm_attr_store_common(struct device *dev,
const char *buf, size_t len, u8 state)
{
struct pci_dev *pdev = to_pci_dev(dev);
- struct pcie_link_state *link;
+ struct pcie_link_state *link = pcie_aspm_get_link(pdev);
bool state_enable;
- link = pcie_aspm_get_link(pdev);
-
if (strtobool(buf, &state_enable) < 0)
return -EINVAL;
@@ -1291,16 +1280,9 @@ static ssize_t clkpm_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct pci_dev *pdev = to_pci_dev(dev);
- struct pcie_link_state *link;
- int val;
-
- link = pcie_aspm_get_link(pdev);
-
- mutex_lock(&aspm_lock);
- val = link->clkpm_enabled;
- mutex_unlock(&aspm_lock);
+ struct pcie_link_state *link = pcie_aspm_get_link(pdev);
- return sprintf(buf, "%d\n", val);
+ return sprintf(buf, "%d\n", link->clkpm_enabled);
}
static ssize_t clkpm_store(struct device *dev,
@@ -1308,11 +1290,9 @@ static ssize_t clkpm_store(struct device *dev,
const char *buf, size_t len)
{
struct pci_dev *pdev = to_pci_dev(dev);
- struct pcie_link_state *link;
+ struct pcie_link_state *link = pcie_aspm_get_link(pdev);
bool state_enable;
- link = pcie_aspm_get_link(pdev);
-
if (strtobool(buf, &state_enable) < 0)
return -EINVAL;
@@ -1352,7 +1332,7 @@ static umode_t aspm_ctrl_attrs_are_visible(struct kobject *kobj,
{
struct device *dev = kobj_to_dev(kobj);
struct pci_dev *pdev = to_pci_dev(dev);
- struct pcie_link_state *link = NULL;
+ struct pcie_link_state *link = pcie_aspm_get_link(pdev);
static const u8 aspm_state_map[] = {
ASPM_STATE_L0S,
ASPM_STATE_L1,
@@ -1362,19 +1342,13 @@ static umode_t aspm_ctrl_attrs_are_visible(struct kobject *kobj,
ASPM_STATE_L1_2_PCIPM,
};
- if (aspm_disabled)
- return 0;
-
- if (pci_is_pcie(pdev))
- link = pcie_aspm_get_link(pdev);
-
- if (!link)
+ if (aspm_disabled || !link)
return 0;
- if (n)
- return link->aspm_capable & aspm_state_map[n - 1] ? a->mode : 0;
- else
+ if (n == 0)
return link->clkpm_capable ? a->mode : 0;
+
+ return link->aspm_capable & aspm_state_map[n - 1] ? a->mode : 0;
}
const struct attribute_group aspm_ctrl_attr_group = {
