| Message ID | 1618955.HVa0YQSOW5@kreacher (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | nvme-pci: Allow PCI bus-level PM to be used if ASPM is disabled | expand |
s|PCI: PCIe: ASPM: Introduce pcie_aspm_enabled()|PCI/ASPM: Add pcie_aspm_enabled()| to match previous history. On Thu, Aug 08, 2019 at 11:55:07PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Add a function checking whether or not PCIe ASPM has been enabled for > a given device. > > It will be used by the NVMe driver to decide how to handle the > device during system suspend. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- > > v2 -> v3: > * Make the new function return bool. > * Change its name back to pcie_aspm_enabled(). > * Fix kerneldoc comment formatting. > > -> v2: > * Move the PCI/PCIe ASPM changes to a separate patch. > * Add the _mask suffix to the new function name. > * Add EXPORT_SYMBOL_GPL() to the new function. > * Avoid adding an unnecessary blank line. > > --- > drivers/pci/pcie/aspm.c | 20 ++++++++++++++++++++ > include/linux/pci.h | 3 +++ > 2 files changed, 23 insertions(+) > > Index: linux-pm/drivers/pci/pcie/aspm.c > =================================================================== > --- linux-pm.orig/drivers/pci/pcie/aspm.c > +++ linux-pm/drivers/pci/pcie/aspm.c > @@ -1170,6 +1170,26 @@ static int pcie_aspm_get_policy(char *bu > module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy, > NULL, 0644); > > +/** > + * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device. > + * @pci_device: Target device. > + */ > +bool pcie_aspm_enabled(struct pci_dev *pci_device) The typical name in this file is "pdev". > +{ > + struct pci_dev *bridge = pci_upstream_bridge(pci_device); > + bool ret; > + > + if (!bridge) > + return false; > + > + mutex_lock(&aspm_lock); > + ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false; > + mutex_unlock(&aspm_lock); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(pcie_aspm_enabled); > + > #ifdef CONFIG_PCIEASPM_DEBUG > static ssize_t link_state_show(struct device *dev, > struct device_attribute *attr, > Index: linux-pm/include/linux/pci.h > =================================================================== > --- linux-pm.orig/include/linux/pci.h > +++ linux-pm/include/linux/pci.h > @@ -1567,8 +1567,11 @@ extern bool pcie_ports_native; > > #ifdef CONFIG_PCIEASPM > bool pcie_aspm_support_enabled(void); > +bool pcie_aspm_enabled(struct pci_dev *pci_device); > #else > static inline bool pcie_aspm_support_enabled(void) { return false; } > +static inline bool pcie_aspm_enabled(struct pci_dev *pci_device) > +{ return false; } > #endif > > #ifdef CONFIG_PCIEAER > > >
On Fri, Aug 9, 2019 at 6:51 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > s|PCI: PCIe: ASPM: Introduce pcie_aspm_enabled()|PCI/ASPM: Add pcie_aspm_enabled()| Will change. > > to match previous history. > > On Thu, Aug 08, 2019 at 11:55:07PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Add a function checking whether or not PCIe ASPM has been enabled for > > a given device. > > > > It will be used by the NVMe driver to decide how to handle the > > device during system suspend. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> Thanks! > > --- > > > > v2 -> v3: > > * Make the new function return bool. > > * Change its name back to pcie_aspm_enabled(). > > * Fix kerneldoc comment formatting. > > > > -> v2: > > * Move the PCI/PCIe ASPM changes to a separate patch. > > * Add the _mask suffix to the new function name. > > * Add EXPORT_SYMBOL_GPL() to the new function. > > * Avoid adding an unnecessary blank line. > > > > --- > > drivers/pci/pcie/aspm.c | 20 ++++++++++++++++++++ > > include/linux/pci.h | 3 +++ > > 2 files changed, 23 insertions(+) > > > > Index: linux-pm/drivers/pci/pcie/aspm.c > > =================================================================== > > --- linux-pm.orig/drivers/pci/pcie/aspm.c > > +++ linux-pm/drivers/pci/pcie/aspm.c > > @@ -1170,6 +1170,26 @@ static int pcie_aspm_get_policy(char *bu > > module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy, > > NULL, 0644); > > > > +/** > > + * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device. > > + * @pci_device: Target device. > > + */ > > +bool pcie_aspm_enabled(struct pci_dev *pci_device) > > The typical name in this file is "pdev". OK, will change. > > +{ > > + struct pci_dev *bridge = pci_upstream_bridge(pci_device); > > + bool ret; > > + > > + if (!bridge) > > + return false; > > + > > + mutex_lock(&aspm_lock); > > + ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false; > > + mutex_unlock(&aspm_lock); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(pcie_aspm_enabled); > > + > > #ifdef CONFIG_PCIEASPM_DEBUG > > static ssize_t link_state_show(struct device *dev, > > struct device_attribute *attr, > > Index: linux-pm/include/linux/pci.h > > =================================================================== > > --- linux-pm.orig/include/linux/pci.h > > +++ linux-pm/include/linux/pci.h > > @@ -1567,8 +1567,11 @@ extern bool pcie_ports_native; > > > > #ifdef CONFIG_PCIEASPM > > bool pcie_aspm_support_enabled(void); > > +bool pcie_aspm_enabled(struct pci_dev *pci_device); > > #else > > static inline bool pcie_aspm_support_enabled(void) { return false; } > > +static inline bool pcie_aspm_enabled(struct pci_dev *pci_device) > > +{ return false; } > > #endif > > > > #ifdef CONFIG_PCIEAER > > > > > >
[+cc Heiner] On Thu, Aug 08, 2019 at 11:55:07PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Add a function checking whether or not PCIe ASPM has been enabled for > a given device. > > It will be used by the NVMe driver to decide how to handle the > device during system suspend. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v2 -> v3: > * Make the new function return bool. > * Change its name back to pcie_aspm_enabled(). > * Fix kerneldoc comment formatting. > > -> v2: > * Move the PCI/PCIe ASPM changes to a separate patch. > * Add the _mask suffix to the new function name. > * Add EXPORT_SYMBOL_GPL() to the new function. > * Avoid adding an unnecessary blank line. > > --- > drivers/pci/pcie/aspm.c | 20 ++++++++++++++++++++ > include/linux/pci.h | 3 +++ > 2 files changed, 23 insertions(+) > > Index: linux-pm/drivers/pci/pcie/aspm.c > =================================================================== > --- linux-pm.orig/drivers/pci/pcie/aspm.c > +++ linux-pm/drivers/pci/pcie/aspm.c > @@ -1170,6 +1170,26 @@ static int pcie_aspm_get_policy(char *bu > module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy, > NULL, 0644); > > +/** > + * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device. > + * @pci_device: Target device. > + */ > +bool pcie_aspm_enabled(struct pci_dev *pci_device) > +{ > + struct pci_dev *bridge = pci_upstream_bridge(pci_device); > + bool ret; > + > + if (!bridge) > + return false; > + > + mutex_lock(&aspm_lock); > + ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false; > + mutex_unlock(&aspm_lock); Why do we need to acquire aspm_lock here? We aren't modifying anything, and I don't think we're preventing a race. If this races with another thread that changes aspm_enabled, we'll return either the old state or the new one, and I think that's still the case even if we don't acquire aspm_lock. > + return ret; > +} > +EXPORT_SYMBOL_GPL(pcie_aspm_enabled); > + > #ifdef CONFIG_PCIEASPM_DEBUG > static ssize_t link_state_show(struct device *dev, > struct device_attribute *attr, > Index: linux-pm/include/linux/pci.h > =================================================================== > --- linux-pm.orig/include/linux/pci.h > +++ linux-pm/include/linux/pci.h > @@ -1567,8 +1567,11 @@ extern bool pcie_ports_native; > > #ifdef CONFIG_PCIEASPM > bool pcie_aspm_support_enabled(void); > +bool pcie_aspm_enabled(struct pci_dev *pci_device); > #else > static inline bool pcie_aspm_support_enabled(void) { return false; } > +static inline bool pcie_aspm_enabled(struct pci_dev *pci_device) > +{ return false; } > #endif > > #ifdef CONFIG_PCIEAER > > >
On Tue, Oct 8, 2019 at 12:34 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Heiner] > > On Thu, Aug 08, 2019 at 11:55:07PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Add a function checking whether or not PCIe ASPM has been enabled for > > a given device. > > > > It will be used by the NVMe driver to decide how to handle the > > device during system suspend. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > > > v2 -> v3: > > * Make the new function return bool. > > * Change its name back to pcie_aspm_enabled(). > > * Fix kerneldoc comment formatting. > > > > -> v2: > > * Move the PCI/PCIe ASPM changes to a separate patch. > > * Add the _mask suffix to the new function name. > > * Add EXPORT_SYMBOL_GPL() to the new function. > > * Avoid adding an unnecessary blank line. > > > > --- > > drivers/pci/pcie/aspm.c | 20 ++++++++++++++++++++ > > include/linux/pci.h | 3 +++ > > 2 files changed, 23 insertions(+) > > > > Index: linux-pm/drivers/pci/pcie/aspm.c > > =================================================================== > > --- linux-pm.orig/drivers/pci/pcie/aspm.c > > +++ linux-pm/drivers/pci/pcie/aspm.c > > @@ -1170,6 +1170,26 @@ static int pcie_aspm_get_policy(char *bu > > module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy, > > NULL, 0644); > > > > +/** > > + * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device. > > + * @pci_device: Target device. > > + */ > > +bool pcie_aspm_enabled(struct pci_dev *pci_device) > > +{ > > + struct pci_dev *bridge = pci_upstream_bridge(pci_device); > > + bool ret; > > + > > + if (!bridge) > > + return false; > > + > > + mutex_lock(&aspm_lock); > > + ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false; > > + mutex_unlock(&aspm_lock); > > Why do we need to acquire aspm_lock here? We aren't modifying > anything, and I don't think we're preventing a race. If this races > with another thread that changes aspm_enabled, we'll return either the > old state or the new one, and I think that's still the case even if we > don't acquire aspm_lock. Well, if we can guarantee that pci_remove_bus_device() will never be called in parallel with this helper, then I agree, but can we guarantee that?
On Tue, Oct 08, 2019 at 11:27:51AM +0200, Rafael J. Wysocki wrote: > On Tue, Oct 8, 2019 at 12:34 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Aug 08, 2019 at 11:55:07PM +0200, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Add a function checking whether or not PCIe ASPM has been enabled for > > > a given device. > > > > > > It will be used by the NVMe driver to decide how to handle the > > > device during system suspend. > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > > > > v2 -> v3: > > > * Make the new function return bool. > > > * Change its name back to pcie_aspm_enabled(). > > > * Fix kerneldoc comment formatting. > > > > > > -> v2: > > > * Move the PCI/PCIe ASPM changes to a separate patch. > > > * Add the _mask suffix to the new function name. > > > * Add EXPORT_SYMBOL_GPL() to the new function. > > > * Avoid adding an unnecessary blank line. > > > > > > --- > > > drivers/pci/pcie/aspm.c | 20 ++++++++++++++++++++ > > > include/linux/pci.h | 3 +++ > > > 2 files changed, 23 insertions(+) > > > > > > Index: linux-pm/drivers/pci/pcie/aspm.c > > > =================================================================== > > > --- linux-pm.orig/drivers/pci/pcie/aspm.c > > > +++ linux-pm/drivers/pci/pcie/aspm.c > > > @@ -1170,6 +1170,26 @@ static int pcie_aspm_get_policy(char *bu > > > module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy, > > > NULL, 0644); > > > > > > +/** > > > + * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device. > > > + * @pci_device: Target device. > > > + */ > > > +bool pcie_aspm_enabled(struct pci_dev *pci_device) > > > +{ > > > + struct pci_dev *bridge = pci_upstream_bridge(pci_device); > > > + bool ret; > > > + > > > + if (!bridge) > > > + return false; > > > + > > > + mutex_lock(&aspm_lock); > > > + ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false; > > > + mutex_unlock(&aspm_lock); > > > > Why do we need to acquire aspm_lock here? We aren't modifying > > anything, and I don't think we're preventing a race. If this races > > with another thread that changes aspm_enabled, we'll return either the > > old state or the new one, and I think that's still the case even if we > > don't acquire aspm_lock. > > Well, if we can guarantee that pci_remove_bus_device() will never be > called in parallel with this helper, then I agree, but can we > guarantee that? Hmm, yeah, I guess that's the question. It's not a race with another thread changing aspm_enabled; the potential race is with another thread removing the last child of "bridge", which will free the link_state and set bridge->link_state = NULL. I think it should be safe to call device-related PCI interfaces if you're holding a reference to the device, e.g., from a driver bound to the device or a sysfs accessor. Since we call pcie_aspm_enabled(dev) from a driver bound to "dev", another thread should not be able to remove "dev" while we're using it. I know that's a little hand-wavey, but if it weren't true, I think we'd have a lot more locking sprinkled everywhere in the PCI core than we do. This has implications for Heiner's ASPM sysfs patches because we're currently doing this in sysfs accessors: static ssize_t aspm_attr_show_common(struct device *dev, ...) { ... link = pcie_aspm_get_link(pdev); mutex_lock(&aspm_lock); enabled = link->aspm_enabled & state; mutex_unlock(&aspm_lock); ... } I assume sysfs must be holding a reference that guarantees "dev" is valid througout this code, and therefore we should not need to hold aspm_lock. Bjorn
On Tue, Oct 8, 2019 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Oct 08, 2019 at 11:27:51AM +0200, Rafael J. Wysocki wrote: > > On Tue, Oct 8, 2019 at 12:34 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Thu, Aug 08, 2019 at 11:55:07PM +0200, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > Add a function checking whether or not PCIe ASPM has been enabled for > > > > a given device. > > > > > > > > It will be used by the NVMe driver to decide how to handle the > > > > device during system suspend. > > > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > --- > > > > > > > > v2 -> v3: > > > > * Make the new function return bool. > > > > * Change its name back to pcie_aspm_enabled(). > > > > * Fix kerneldoc comment formatting. > > > > > > > > -> v2: > > > > * Move the PCI/PCIe ASPM changes to a separate patch. > > > > * Add the _mask suffix to the new function name. > > > > * Add EXPORT_SYMBOL_GPL() to the new function. > > > > * Avoid adding an unnecessary blank line. > > > > > > > > --- > > > > drivers/pci/pcie/aspm.c | 20 ++++++++++++++++++++ > > > > include/linux/pci.h | 3 +++ > > > > 2 files changed, 23 insertions(+) > > > > > > > > Index: linux-pm/drivers/pci/pcie/aspm.c > > > > =================================================================== > > > > --- linux-pm.orig/drivers/pci/pcie/aspm.c > > > > +++ linux-pm/drivers/pci/pcie/aspm.c > > > > @@ -1170,6 +1170,26 @@ static int pcie_aspm_get_policy(char *bu > > > > module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy, > > > > NULL, 0644); > > > > > > > > +/** > > > > + * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device. > > > > + * @pci_device: Target device. > > > > + */ > > > > +bool pcie_aspm_enabled(struct pci_dev *pci_device) > > > > +{ > > > > + struct pci_dev *bridge = pci_upstream_bridge(pci_device); > > > > + bool ret; > > > > + > > > > + if (!bridge) > > > > + return false; > > > > + > > > > + mutex_lock(&aspm_lock); > > > > + ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false; > > > > + mutex_unlock(&aspm_lock); > > > > > > Why do we need to acquire aspm_lock here? We aren't modifying > > > anything, and I don't think we're preventing a race. If this races > > > with another thread that changes aspm_enabled, we'll return either the > > > old state or the new one, and I think that's still the case even if we > > > don't acquire aspm_lock. > > > > Well, if we can guarantee that pci_remove_bus_device() will never be > > called in parallel with this helper, then I agree, but can we > > guarantee that? > > Hmm, yeah, I guess that's the question. It's not a race with another > thread changing aspm_enabled; the potential race is with another > thread removing the last child of "bridge", which will free the > link_state and set bridge->link_state = NULL. > > I think it should be safe to call device-related PCI interfaces if > you're holding a reference to the device, e.g., from a driver bound to > the device or a sysfs accessor. Since we call pcie_aspm_enabled(dev) > from a driver bound to "dev", another thread should not be able to > remove "dev" while we're using it. > > I know that's a little hand-wavey, but if it weren't true, I think > we'd have a lot more locking sprinkled everywhere in the PCI core than > we do. > > This has implications for Heiner's ASPM sysfs patches because we're > currently doing this in sysfs accessors: > > static ssize_t aspm_attr_show_common(struct device *dev, ...) > { > ... > link = pcie_aspm_get_link(pdev); > > mutex_lock(&aspm_lock); > enabled = link->aspm_enabled & state; > mutex_unlock(&aspm_lock); > ... > } > > I assume sysfs must be holding a reference that guarantees "dev" is > valid througout this code, and therefore we should not need to hold > aspm_lock. In principle, pcie_aspm_enabled() need not be called via sysfs. In the particular NVMe use case, it is called from the driver's own PM callback, so it would be safe without the locking AFAICS. I guess it is safe to drop the locking from there, but then it would be good to mention in the kerneldoc that calling it is only safe under the assumption that the link_state object cannot go away while it is running.
On Wed, Oct 09, 2019 at 12:54:37AM +0200, Rafael J. Wysocki wrote: > On Tue, Oct 8, 2019 at 11:16 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Tue, Oct 08, 2019 at 11:27:51AM +0200, Rafael J. Wysocki wrote: > > > On Tue, Oct 8, 2019 at 12:34 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Thu, Aug 08, 2019 at 11:55:07PM +0200, Rafael J. Wysocki wrote: > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > Add a function checking whether or not PCIe ASPM has been enabled for > > > > > a given device. > > > > > > > > > > It will be used by the NVMe driver to decide how to handle the > > > > > device during system suspend. > > > > > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > --- > > > > > > > > > > v2 -> v3: > > > > > * Make the new function return bool. > > > > > * Change its name back to pcie_aspm_enabled(). > > > > > * Fix kerneldoc comment formatting. > > > > > > > > > > -> v2: > > > > > * Move the PCI/PCIe ASPM changes to a separate patch. > > > > > * Add the _mask suffix to the new function name. > > > > > * Add EXPORT_SYMBOL_GPL() to the new function. > > > > > * Avoid adding an unnecessary blank line. > > > > > > > > > > --- > > > > > drivers/pci/pcie/aspm.c | 20 ++++++++++++++++++++ > > > > > include/linux/pci.h | 3 +++ > > > > > 2 files changed, 23 insertions(+) > > > > > > > > > > Index: linux-pm/drivers/pci/pcie/aspm.c > > > > > =================================================================== > > > > > --- linux-pm.orig/drivers/pci/pcie/aspm.c > > > > > +++ linux-pm/drivers/pci/pcie/aspm.c > > > > > @@ -1170,6 +1170,26 @@ static int pcie_aspm_get_policy(char *bu > > > > > module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy, > > > > > NULL, 0644); > > > > > > > > > > +/** > > > > > + * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device. > > > > > + * @pci_device: Target device. > > > > > + */ > > > > > +bool pcie_aspm_enabled(struct pci_dev *pci_device) > > > > > +{ > > > > > + struct pci_dev *bridge = pci_upstream_bridge(pci_device); > > > > > + bool ret; > > > > > + > > > > > + if (!bridge) > > > > > + return false; > > > > > + > > > > > + mutex_lock(&aspm_lock); > > > > > + ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false; > > > > > + mutex_unlock(&aspm_lock); > > > > > > > > Why do we need to acquire aspm_lock here? We aren't modifying > > > > anything, and I don't think we're preventing a race. If this races > > > > with another thread that changes aspm_enabled, we'll return either the > > > > old state or the new one, and I think that's still the case even if we > > > > don't acquire aspm_lock. > > > > > > Well, if we can guarantee that pci_remove_bus_device() will never be > > > called in parallel with this helper, then I agree, but can we > > > guarantee that? > > > > Hmm, yeah, I guess that's the question. It's not a race with another > > thread changing aspm_enabled; the potential race is with another > > thread removing the last child of "bridge", which will free the > > link_state and set bridge->link_state = NULL. > > > > I think it should be safe to call device-related PCI interfaces if > > you're holding a reference to the device, e.g., from a driver bound to > > the device or a sysfs accessor. Since we call pcie_aspm_enabled(dev) > > from a driver bound to "dev", another thread should not be able to > > remove "dev" while we're using it. > > > > I know that's a little hand-wavey, but if it weren't true, I think > > we'd have a lot more locking sprinkled everywhere in the PCI core than > > we do. > > > > This has implications for Heiner's ASPM sysfs patches because we're > > currently doing this in sysfs accessors: > > > > static ssize_t aspm_attr_show_common(struct device *dev, ...) > > { > > ... > > link = pcie_aspm_get_link(pdev); > > > > mutex_lock(&aspm_lock); > > enabled = link->aspm_enabled & state; > > mutex_unlock(&aspm_lock); > > ... > > } > > > > I assume sysfs must be holding a reference that guarantees "dev" is > > valid througout this code, and therefore we should not need to hold > > aspm_lock. > > In principle, pcie_aspm_enabled() need not be called via sysfs. > > In the particular NVMe use case, it is called from the driver's own PM > callback, so it would be safe without the locking AFAICS. Right, pcie_aspm_enabled() is only used by drivers (actually only by the nvme driver so far). And aspm_attr_show_common() is only used via new sysfs code being added by Heiner. > I guess it is safe to drop the locking from there, but then it would > be good to mention in the kerneldoc that calling it is only safe under > the assumption that the link_state object cannot go away while it is > running. I'll post a patch to that effect. Thanks! Bjorn
Index: linux-pm/drivers/pci/pcie/aspm.c =================================================================== --- linux-pm.orig/drivers/pci/pcie/aspm.c +++ linux-pm/drivers/pci/pcie/aspm.c @@ -1170,6 +1170,26 @@ static int pcie_aspm_get_policy(char *bu module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy, NULL, 0644); +/** + * pcie_aspm_enabled - Check if PCIe ASPM has been enabled for a device. + * @pci_device: Target device. + */ +bool pcie_aspm_enabled(struct pci_dev *pci_device) +{ + struct pci_dev *bridge = pci_upstream_bridge(pci_device); + bool ret; + + if (!bridge) + return false; + + mutex_lock(&aspm_lock); + ret = bridge->link_state ? !!bridge->link_state->aspm_enabled : false; + mutex_unlock(&aspm_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(pcie_aspm_enabled); + #ifdef CONFIG_PCIEASPM_DEBUG static ssize_t link_state_show(struct device *dev, struct device_attribute *attr, Index: linux-pm/include/linux/pci.h =================================================================== --- linux-pm.orig/include/linux/pci.h +++ linux-pm/include/linux/pci.h @@ -1567,8 +1567,11 @@ extern bool pcie_ports_native; #ifdef CONFIG_PCIEASPM bool pcie_aspm_support_enabled(void); +bool pcie_aspm_enabled(struct pci_dev *pci_device); #else static inline bool pcie_aspm_support_enabled(void) { return false; } +static inline bool pcie_aspm_enabled(struct pci_dev *pci_device) +{ return false; } #endif #ifdef CONFIG_PCIEAER