| Message ID | 1463991648-85051-1-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote: > When a PCI device is removed through sysfs interface the upstream bridge > (PCIe port) can be runtime suspended if it was the last device on that bus. > Now, if the bridge is in D3 we cannot find devices below the bridge > anymore. For example following fails to find the removed device again: > > # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove > # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan > > Where 0000:00:01.0 is the bridge device. > > In order to be able to rescan devices below the bridge add > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This > should keep bridges powered on while their children devices are being > scanned. > > Reported-by: Peter Wu <peter@lekensteyn.nl> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> This looks like basically the same idea as "ACPI / hotplug / PCI: Runtime resume bridge before rescan". The hotplug_event() path modified by that patch eventually calls pci_scan_bridge(): hotplug_event enable_slot pci_scan_bridge so this patch looks a little more general. Does it make "ACPI / hotplug / PCI: Runtime resume bridge before rescan" unnecessary? Can I just replace that patch with this one? > --- > drivers/pci/probe.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 8004f67c57ec..15e77c92311e 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -16,6 +16,7 @@ > #include <linux/aer.h> > #include <linux/acpi.h> > #include <linux/irqdomain.h> > +#include <linux/pm_runtime.h> > #include "pci.h" > > #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ > @@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > u8 primary, secondary, subordinate; > int broken = 0; > > + /* > + * Make sure the bridge is powered on to be able to access config > + * space of devices below it. > + */ > + pm_runtime_get_sync(&dev->dev); > + > pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses); > primary = buses & 0xFF; > secondary = (buses >> 8) & 0xFF; > @@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > out: > pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl); > > + pm_runtime_put(&dev->dev); > + > return max; > } > EXPORT_SYMBOL(pci_scan_bridge); > -- > 2.8.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[+cc Valdis, Dave] On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote: > On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote: > > When a PCI device is removed through sysfs interface the upstream bridge > > (PCIe port) can be runtime suspended if it was the last device on that bus. > > Now, if the bridge is in D3 we cannot find devices below the bridge > > anymore. For example following fails to find the removed device again: > > > > # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove > > # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan > > > > Where 0000:00:01.0 is the bridge device. > > > > In order to be able to rescan devices below the bridge add > > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This > > should keep bridges powered on while their children devices are being > > scanned. > > > > Reported-by: Peter Wu <peter@lekensteyn.nl> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > This looks like basically the same idea as "ACPI / hotplug / PCI: > Runtime resume bridge before rescan". > > The hotplug_event() path modified by that patch eventually calls > pci_scan_bridge(): > > hotplug_event > enable_slot > pci_scan_bridge > > so this patch looks a little more general. Does it make "ACPI / > hotplug / PCI: Runtime resume bridge before rescan" unnecessary? > Can I just replace that patch with this one? I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge before rescan" with this one and pushed the result to https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm Please take a look, test it, and let me know if I need to add the ACPI patch back. This branch also includes the fix for the lockdep splat reported by Valdis. This is what I hope to get into v4.7-rc1. > > --- > > drivers/pci/probe.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 8004f67c57ec..15e77c92311e 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -16,6 +16,7 @@ > > #include <linux/aer.h> > > #include <linux/acpi.h> > > #include <linux/irqdomain.h> > > +#include <linux/pm_runtime.h> > > #include "pci.h" > > > > #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ > > @@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > > u8 primary, secondary, subordinate; > > int broken = 0; > > > > + /* > > + * Make sure the bridge is powered on to be able to access config > > + * space of devices below it. > > + */ > > + pm_runtime_get_sync(&dev->dev); > > + > > pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses); > > primary = buses & 0xFF; > > secondary = (buses >> 8) & 0xFF; > > @@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > > out: > > pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl); > > > > + pm_runtime_put(&dev->dev); > > + > > return max; > > } > > EXPORT_SYMBOL(pci_scan_bridge); > > -- > > 2.8.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 23, 2016 at 04:50:15PM -0500, Bjorn Helgaas wrote: > [+cc Valdis, Dave] > > On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote: > > On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote: > > > When a PCI device is removed through sysfs interface the upstream bridge > > > (PCIe port) can be runtime suspended if it was the last device on that bus. > > > Now, if the bridge is in D3 we cannot find devices below the bridge > > > anymore. For example following fails to find the removed device again: > > > > > > # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove > > > # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan > > > > > > Where 0000:00:01.0 is the bridge device. > > > > > > In order to be able to rescan devices below the bridge add > > > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This > > > should keep bridges powered on while their children devices are being > > > scanned. > > > > > > Reported-by: Peter Wu <peter@lekensteyn.nl> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > This looks like basically the same idea as "ACPI / hotplug / PCI: > > Runtime resume bridge before rescan". > > > > The hotplug_event() path modified by that patch eventually calls > > pci_scan_bridge(): > > > > hotplug_event > > enable_slot > > pci_scan_bridge > > > > so this patch looks a little more general. Does it make "ACPI / > > hotplug / PCI: Runtime resume bridge before rescan" unnecessary? > > Can I just replace that patch with this one? > > I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge > before rescan" with this one and pushed the result to > > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm > > Please take a look, test it, and let me know if I need to add the ACPI > patch back. > > This branch also includes the fix for the lockdep splat reported by > Valdis. This is what I hope to get into v4.7-rc1. Ping? I'd like to ask Linus to pull this pci/pm branch before v4.7-rc1. It currently has these changes: 8b71f5652eea PCI: Add runtime PM support for PCIe ports af81f0fa638b PCI: Power on bridges before scanning new devices 9741a01c9f55 PCI: Put PCIe ports into D3 during suspend b3a63ff7baf1 PCI: Don't clear d3cold_allowed for PCIe ports I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan" on the assumption that "PCI: Power on bridges before scanning new devices" is sufficient to cover both the ACPI and the generic PCi rescan cases, but I'd like some reassurance about that. Lukas, you tested that acpiphp patch. Would you be able to verify that your test case still works even with that patch replaced by the generic one? FWIW, my pci/pm branch with these updates is included in next-20160524. > > > --- > > > drivers/pci/probe.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > index 8004f67c57ec..15e77c92311e 100644 > > > --- a/drivers/pci/probe.c > > > +++ b/drivers/pci/probe.c > > > @@ -16,6 +16,7 @@ > > > #include <linux/aer.h> > > > #include <linux/acpi.h> > > > #include <linux/irqdomain.h> > > > +#include <linux/pm_runtime.h> > > > #include "pci.h" > > > > > > #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ > > > @@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > > > u8 primary, secondary, subordinate; > > > int broken = 0; > > > > > > + /* > > > + * Make sure the bridge is powered on to be able to access config > > > + * space of devices below it. > > > + */ > > > + pm_runtime_get_sync(&dev->dev); > > > + > > > pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses); > > > primary = buses & 0xFF; > > > secondary = (buses >> 8) & 0xFF; > > > @@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > > > out: > > > pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl); > > > > > > + pm_runtime_put(&dev->dev); > > > + > > > return max; > > > } > > > EXPORT_SYMBOL(pci_scan_bridge); > > > -- > > > 2.8.1 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, On Tue, May 24, 2016 at 07:23:57AM -0500, Bjorn Helgaas wrote: > On Mon, May 23, 2016 at 04:50:15PM -0500, Bjorn Helgaas wrote: > > [+cc Valdis, Dave] > > > > On Mon, May 23, 2016 at 03:00:42PM -0500, Bjorn Helgaas wrote: > > > On Mon, May 23, 2016 at 11:20:48AM +0300, Mika Westerberg wrote: > > > > When a PCI device is removed through sysfs interface the upstream bridge > > > > (PCIe port) can be runtime suspended if it was the last device on that bus. > > > > Now, if the bridge is in D3 we cannot find devices below the bridge > > > > anymore. For example following fails to find the removed device again: > > > > > > > > # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove > > > > # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan > > > > > > > > Where 0000:00:01.0 is the bridge device. > > > > > > > > In order to be able to rescan devices below the bridge add > > > > pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This > > > > should keep bridges powered on while their children devices are being > > > > scanned. > > > > > > > > Reported-by: Peter Wu <peter@lekensteyn.nl> > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > > > This looks like basically the same idea as "ACPI / hotplug / PCI: > > > Runtime resume bridge before rescan". > > > > > > The hotplug_event() path modified by that patch eventually calls > > > pci_scan_bridge(): > > > > > > hotplug_event > > > enable_slot > > > pci_scan_bridge > > > > > > so this patch looks a little more general. Does it make "ACPI / > > > hotplug / PCI: Runtime resume bridge before rescan" unnecessary? > > > Can I just replace that patch with this one? > > > > I speculatively replaced "ACPI / hotplug / PCI: Runtime resume bridge > > before rescan" with this one and pushed the result to > > > > https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/pm > > > > Please take a look, test it, and let me know if I need to add the ACPI > > patch back. > > > > This branch also includes the fix for the lockdep splat reported by > > Valdis. This is what I hope to get into v4.7-rc1. > > Ping? I'd like to ask Linus to pull this pci/pm branch before v4.7-rc1. > It currently has these changes: > > 8b71f5652eea PCI: Add runtime PM support for PCIe ports > af81f0fa638b PCI: Power on bridges before scanning new devices > 9741a01c9f55 PCI: Put PCIe ports into D3 during suspend > b3a63ff7baf1 PCI: Don't clear d3cold_allowed for PCIe ports > > I dropped "ACPI / hotplug / PCI: Runtime resume bridge before rescan" > on the assumption that "PCI: Power on bridges before scanning new > devices" is sufficient to cover both the ACPI and the generic PCi > rescan cases, but I'd like some reassurance about that. > > Lukas, you tested that acpiphp patch. Would you be able to verify > that your test case still works even with that patch replaced by the > generic one? I only have a Mac and Macs don't use ACPI-based hotplug, only native pciehp hotplug. So unfortunately I can't say anything about whether the ACPI patch is still needed. For Dave Airlie's use case the patch seems not to be needed, he wants to suspend root ports to which a GPU is attached, so I guess he doesn't care much about hotplug. The patch could therefore be pulled by Linus separately, if it turns that it's still needed. Thanks, Lukas > > FWIW, my pci/pm branch with these updates is included in > next-20160524. > > > > > --- > > > > drivers/pci/probe.c | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > > index 8004f67c57ec..15e77c92311e 100644 > > > > --- a/drivers/pci/probe.c > > > > +++ b/drivers/pci/probe.c > > > > @@ -16,6 +16,7 @@ > > > > #include <linux/aer.h> > > > > #include <linux/acpi.h> > > > > #include <linux/irqdomain.h> > > > > +#include <linux/pm_runtime.h> > > > > #include "pci.h" > > > > > > > > #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ > > > > @@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > > > > u8 primary, secondary, subordinate; > > > > int broken = 0; > > > > > > > > + /* > > > > + * Make sure the bridge is powered on to be able to access config > > > > + * space of devices below it. > > > > + */ > > > > + pm_runtime_get_sync(&dev->dev); > > > > + > > > > pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses); > > > > primary = buses & 0xFF; > > > > secondary = (buses >> 8) & 0xFF; > > > > @@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) > > > > out: > > > > pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl); > > > > > > > > + pm_runtime_put(&dev->dev); > > > > + > > > > return max; > > > > } > > > > EXPORT_SYMBOL(pci_scan_bridge); > > > > -- > > > > 2.8.1 > > > > > > > > -- > > > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > > > > the body of a message to majordomo@vger.kernel.org > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 8004f67c57ec..15e77c92311e 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -16,6 +16,7 @@ #include <linux/aer.h> #include <linux/acpi.h> #include <linux/irqdomain.h> +#include <linux/pm_runtime.h> #include "pci.h" #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ @@ -832,6 +833,12 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) u8 primary, secondary, subordinate; int broken = 0; + /* + * Make sure the bridge is powered on to be able to access config + * space of devices below it. + */ + pm_runtime_get_sync(&dev->dev); + pci_read_config_dword(dev, PCI_PRIMARY_BUS, &buses); primary = buses & 0xFF; secondary = (buses >> 8) & 0xFF; @@ -1012,6 +1019,8 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max, int pass) out: pci_write_config_word(dev, PCI_BRIDGE_CONTROL, bctl); + pm_runtime_put(&dev->dev); + return max; } EXPORT_SYMBOL(pci_scan_bridge);
When a PCI device is removed through sysfs interface the upstream bridge (PCIe port) can be runtime suspended if it was the last device on that bus. Now, if the bridge is in D3 we cannot find devices below the bridge anymore. For example following fails to find the removed device again: # echo 1 > /sys/bus/pci/devices/0000:00:01.0/0000:01:00.0/remove # echo 1 > /sys/bus/pci/devices/0000:00:01.0/rescan Where 0000:00:01.0 is the bridge device. In order to be able to rescan devices below the bridge add pm_runtime_get_sync()/pm_runtime_put() calls to pci_scan_bridge(). This should keep bridges powered on while their children devices are being scanned. Reported-by: Peter Wu <peter@lekensteyn.nl> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/pci/probe.c | 9 +++++++++ 1 file changed, 9 insertions(+)