| Message ID | e1c28e25-df18-16e1-3e9f-933f613ea858@gmail.com |
|---|---|
| Headers | show |
| Series | PCI/ASPM: Add sysfs attributes for controlling ASPM | expand |
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. Bjorn
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@google.com 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 = {
On 10.10.2019 15:22, Bjorn Helgaas wrote: > 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@google.com > > 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. > Looks good to me. Thanks, Heiner > 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 = { >
On Thu, Oct 10, 2019 at 10:45:52PM +0200, Heiner Kallweit wrote: > On 10.10.2019 15:22, Bjorn Helgaas wrote: > > 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@google.com > > > > 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. > > > Looks good to me. Thanks, Heiner Thanks, I squashed these in and updated pci/aspm.
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(-)