Re: [PATCH] PCI: Power on bridges before scanning new devices

[Bjorn Helgaas <helgaas@kernel.org>]

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