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@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. > 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 = { >
