| Message ID | 20220517124319.47125-1-yangyicong@hisilicon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RESEND,v5] PCI: Make sure the bus bridge powered on when scanning bus | expand |
On 2022/5/17 20:43, Yicong Yang wrote: > When the bus bridge is runtime suspended, we'll fail to rescan > the devices through sysfs as we cannot access the configuration > space correctly when the bridge is in D3hot. > It can be reproduced like: > > $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/0000:81:00.1/remove > $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/pci_bus/0000:81/rescan > > 0000:80:00.0 is a Root Port and it is runtime-suspended, so > 0000:81:00.1 is unreachable after a rescan. > > Power up the bridge when scanning the child bus and allow it to > suspend again by adding pm_runtime_get_sync()/pm_runtime_put() > in pci_scan_child_bus_extend(). > > Cc: Rafael J. Wysocki <rafael@kernel.org> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- Attach the change log here. Sorry for my forgetness.. Change since v4: - rephrase the commit suggested by Rafael Link: https://lore.kernel.org/lkml/20220422080404.27724-1-yangyicong@hisilicon.com/ Change since v3: - retain the pm_runtime_*() calls in pci_scan_bridge_extend() as Rafael points out that it's necessary when the brigde is in D3cold Link: https://lore.kernel.org/linux-pci/20220414123736.34150-1-yangyicong@hisilicon.com/ Change since v2: - just rebase it on v5.18-rc2 Link: https://lore.kernel.org/linux-pci/1601029386-4928-1-git-send-email-yangyicong@hisilicon.com/ Change since v1: - use an intermediate variable *bridge as suggested - remove the pm_runtime_*() calls in pci_scan_bridge_extend() Link: https://lore.kernel.org/linux-pci/1596022223-4765-1-git-send-email-yangyicong@hisilicon.com/ > drivers/pci/probe.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 17a969942d37..b108e72b6586 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2859,11 +2859,20 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0; > unsigned int start = bus->busn_res.start; > unsigned int devfn, fn, cmax, max = start; > + struct pci_dev *bridge = bus->self; > struct pci_dev *dev; > int nr_devs; > > dev_dbg(&bus->dev, "scanning bus\n"); > > + /* > + * Make sure the bus bridge is powered on, otherwise we may not be > + * able to scan the devices as we may fail to access the configuration > + * space of subordinates. > + */ > + if (bridge) > + pm_runtime_get_sync(&bridge->dev); > + > /* Go find them, Rover! */ > for (devfn = 0; devfn < 256; devfn += 8) { > nr_devs = pci_scan_slot(bus, devfn); > @@ -2976,6 +2985,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > } > } > > + if (bridge) > + pm_runtime_put(&bridge->dev); > + > /* > * We've scanned the bus and so we know all about what's on > * the other side of any bridges that may be on this bus plus >
On Tue, May 17, 2022 at 08:43:19PM +0800, Yicong Yang wrote: > When the bus bridge is runtime suspended, we'll fail to rescan > the devices through sysfs as we cannot access the configuration > space correctly when the bridge is in D3hot. Is the "D3hot" above a typo? I think devices are supposed to respond to config accesses when in D3hot. PCIe r6.0, sec 5.3.1.4.1: Configuration and Message requests are the only TLPs accepted by a Function in the D3Hot state. ... Functions that are in D3Hot are permitted to be transitioned by software (writing to their PMCSR PowerState field) to the D0active state or the D0uninitialized state. Functions that are in D3Hot must respond to Configuration Space accesses as long as power and clock are supplied so that they can be returned to D0 by software. > It can be reproduced like: > > $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/0000:81:00.1/remove > $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/pci_bus/0000:81/rescan > > 0000:80:00.0 is a Root Port and it is runtime-suspended, so > 0000:81:00.1 is unreachable after a rescan. > > Power up the bridge when scanning the child bus and allow it to > suspend again by adding pm_runtime_get_sync()/pm_runtime_put() > in pci_scan_child_bus_extend(). > > Cc: Rafael J. Wysocki <rafael@kernel.org> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/pci/probe.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 17a969942d37..b108e72b6586 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2859,11 +2859,20 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0; > unsigned int start = bus->busn_res.start; > unsigned int devfn, fn, cmax, max = start; > + struct pci_dev *bridge = bus->self; > struct pci_dev *dev; > int nr_devs; > > dev_dbg(&bus->dev, "scanning bus\n"); > > + /* > + * Make sure the bus bridge is powered on, otherwise we may not be > + * able to scan the devices as we may fail to access the configuration > + * space of subordinates. > + */ > + if (bridge) > + pm_runtime_get_sync(&bridge->dev); > + > /* Go find them, Rover! */ > for (devfn = 0; devfn < 256; devfn += 8) { > nr_devs = pci_scan_slot(bus, devfn); > @@ -2976,6 +2985,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > } > } > > + if (bridge) > + pm_runtime_put(&bridge->dev); > + > /* > * We've scanned the bus and so we know all about what's on > * the other side of any bridges that may be on this bus plus > -- > 2.24.0 >
On Tue, May 24, 2022 at 10:58 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, May 17, 2022 at 08:43:19PM +0800, Yicong Yang wrote: > > When the bus bridge is runtime suspended, we'll fail to rescan > > the devices through sysfs as we cannot access the configuration > > space correctly when the bridge is in D3hot. > > Is the "D3hot" above a typo? I think devices are supposed to respond > to config accesses when in D3hot. PCIe r6.0, sec 5.3.1.4.1: > > Configuration and Message requests are the only TLPs accepted by a > Function in the D3Hot state. ... > > Functions that are in D3Hot are permitted to be transitioned by > software (writing to their PMCSR PowerState field) to the D0active > state or the D0uninitialized state. Functions that are in D3Hot must > respond to Configuration Space accesses as long as power and clock > are supplied so that they can be returned to D0 by software. That applies to the device itself, though, and not to the bus under it. In general, a bridge in D3hot causes the bus segment on the other side of it to be inaccessible even for config space accesses AFAICS. > > It can be reproduced like: > > > > $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/0000:81:00.1/remove > > $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/pci_bus/0000:81/rescan > > > > 0000:80:00.0 is a Root Port and it is runtime-suspended, so > > 0000:81:00.1 is unreachable after a rescan. > > > > Power up the bridge when scanning the child bus and allow it to > > suspend again by adding pm_runtime_get_sync()/pm_runtime_put() > > in pci_scan_child_bus_extend(). > > > > Cc: Rafael J. Wysocki <rafael@kernel.org> > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/pci/probe.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 17a969942d37..b108e72b6586 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -2859,11 +2859,20 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > > unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0; > > unsigned int start = bus->busn_res.start; > > unsigned int devfn, fn, cmax, max = start; > > + struct pci_dev *bridge = bus->self; > > struct pci_dev *dev; > > int nr_devs; > > > > dev_dbg(&bus->dev, "scanning bus\n"); > > > > + /* > > + * Make sure the bus bridge is powered on, otherwise we may not be > > + * able to scan the devices as we may fail to access the configuration > > + * space of subordinates. > > + */ > > + if (bridge) > > + pm_runtime_get_sync(&bridge->dev); > > + > > /* Go find them, Rover! */ > > for (devfn = 0; devfn < 256; devfn += 8) { > > nr_devs = pci_scan_slot(bus, devfn); > > @@ -2976,6 +2985,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > > } > > } > > > > + if (bridge) > > + pm_runtime_put(&bridge->dev); > > + > > /* > > * We've scanned the bus and so we know all about what's on > > * the other side of any bridges that may be on this bus plus > > -- > > 2.24.0 > >
On Wed, May 25, 2022 at 01:27:21PM +0200, Rafael J. Wysocki wrote: > On Tue, May 24, 2022 at 10:58 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Tue, May 17, 2022 at 08:43:19PM +0800, Yicong Yang wrote: > > > When the bus bridge is runtime suspended, we'll fail to rescan > > > the devices through sysfs as we cannot access the configuration > > > space correctly when the bridge is in D3hot. > > > > Is the "D3hot" above a typo? I think devices are supposed to respond > > to config accesses when in D3hot. PCIe r6.0, sec 5.3.1.4.1: > > > > Configuration and Message requests are the only TLPs accepted by a > > Function in the D3Hot state. ... > > > > Functions that are in D3Hot are permitted to be transitioned by > > software (writing to their PMCSR PowerState field) to the D0active > > state or the D0uninitialized state. Functions that are in D3Hot must > > respond to Configuration Space accesses as long as power and clock > > are supplied so that they can be returned to D0 by software. > > That applies to the device itself, though, and not to the bus under it. > > In general, a bridge in D3hot causes the bus segment on the other side > of it to be inaccessible even for config space accesses AFAICS. Oh, thank you! That was the piece I was missing. I'll tweak the commit log to say something like: A bridge in a non-D0 power state does not forward config accesses to its secondary side (PCIe r6.0, sec 5.3.1). Make sure the bridge is in D0 while we enumerate devices below it. > > > It can be reproduced like: > > > > > > $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/0000:81:00.1/remove > > > $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/pci_bus/0000:81/rescan > > > > > > 0000:80:00.0 is a Root Port and it is runtime-suspended, so > > > 0000:81:00.1 is unreachable after a rescan. > > > > > > Power up the bridge when scanning the child bus and allow it to > > > suspend again by adding pm_runtime_get_sync()/pm_runtime_put() > > > in pci_scan_child_bus_extend(). > > > > > > Cc: Rafael J. Wysocki <rafael@kernel.org> > > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > --- > > > drivers/pci/probe.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > > index 17a969942d37..b108e72b6586 100644 > > > --- a/drivers/pci/probe.c > > > +++ b/drivers/pci/probe.c > > > @@ -2859,11 +2859,20 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > > > unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0; > > > unsigned int start = bus->busn_res.start; > > > unsigned int devfn, fn, cmax, max = start; > > > + struct pci_dev *bridge = bus->self; > > > struct pci_dev *dev; > > > int nr_devs; > > > > > > dev_dbg(&bus->dev, "scanning bus\n"); > > > > > > + /* > > > + * Make sure the bus bridge is powered on, otherwise we may not be > > > + * able to scan the devices as we may fail to access the configuration > > > + * space of subordinates. > > > + */ > > > + if (bridge) > > > + pm_runtime_get_sync(&bridge->dev); > > > + > > > /* Go find them, Rover! */ > > > for (devfn = 0; devfn < 256; devfn += 8) { > > > nr_devs = pci_scan_slot(bus, devfn); > > > @@ -2976,6 +2985,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, > > > } > > > } > > > > > > + if (bridge) > > > + pm_runtime_put(&bridge->dev); > > > + > > > /* > > > * We've scanned the bus and so we know all about what's on > > > * the other side of any bridges that may be on this bus plus > > > -- > > > 2.24.0 > > >
在 2022/5/25 4:58, Bjorn Helgaas 写道: > On Tue, May 17, 2022 at 08:43:19PM +0800, Yicong Yang wrote: >> When the bus bridge is runtime suspended, we'll fail to rescan >> the devices through sysfs as we cannot access the configuration >> space correctly when the bridge is in D3hot. > Is the "D3hot" above a typo? I think devices are supposed to respond > to config accesses when in D3hot. PCIe r6.0, sec 5.3.1.4.1: > > Configuration and Message requests are the only TLPs accepted by a > Function in the D3Hot state. ... It's right that we can access the bridge's configuration space when it's in D3hot. But we need to access the configuration space of downstream devices in a rescan, which is inaccessible in the upstream bridge's D3hot state. So the "D3hot" state in the commit is qualified to the *bridge*. > > Functions that are in D3Hot are permitted to be transitioned by > software (writing to their PMCSR PowerState field) to the D0active > state or the D0uninitialized state. Functions that are in D3Hot must > respond to Configuration Space accesses as long as power and clock > are supplied so that they can be returned to D0 by software. > >> It can be reproduced like: >> >> $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/0000:81:00.1/remove >> $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/pci_bus/0000:81/rescan >> >> 0000:80:00.0 is a Root Port and it is runtime-suspended, so >> 0000:81:00.1 is unreachable after a rescan. >> >> Power up the bridge when scanning the child bus and allow it to >> suspend again by adding pm_runtime_get_sync()/pm_runtime_put() >> in pci_scan_child_bus_extend(). >> >> Cc: Rafael J. Wysocki <rafael@kernel.org> >> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> --- >> drivers/pci/probe.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index 17a969942d37..b108e72b6586 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -2859,11 +2859,20 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, >> unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0; >> unsigned int start = bus->busn_res.start; >> unsigned int devfn, fn, cmax, max = start; >> + struct pci_dev *bridge = bus->self; >> struct pci_dev *dev; >> int nr_devs; >> >> dev_dbg(&bus->dev, "scanning bus\n"); >> >> + /* >> + * Make sure the bus bridge is powered on, otherwise we may not be >> + * able to scan the devices as we may fail to access the configuration >> + * space of subordinates. >> + */ >> + if (bridge) >> + pm_runtime_get_sync(&bridge->dev); >> + >> /* Go find them, Rover! */ >> for (devfn = 0; devfn < 256; devfn += 8) { >> nr_devs = pci_scan_slot(bus, devfn); >> @@ -2976,6 +2985,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, >> } >> } >> >> + if (bridge) >> + pm_runtime_put(&bridge->dev); >> + >> /* >> * We've scanned the bus and so we know all about what's on >> * the other side of any bridges that may be on this bus plus >> -- >> 2.24.0 >>
在 2022/5/25 22:19, Bjorn Helgaas 写道: > On Wed, May 25, 2022 at 01:27:21PM +0200, Rafael J. Wysocki wrote: >> On Tue, May 24, 2022 at 10:58 PM Bjorn Helgaas <helgaas@kernel.org> wrote: >>> On Tue, May 17, 2022 at 08:43:19PM +0800, Yicong Yang wrote: >>>> When the bus bridge is runtime suspended, we'll fail to rescan >>>> the devices through sysfs as we cannot access the configuration >>>> space correctly when the bridge is in D3hot. >>> Is the "D3hot" above a typo? I think devices are supposed to respond >>> to config accesses when in D3hot. PCIe r6.0, sec 5.3.1.4.1: >>> >>> Configuration and Message requests are the only TLPs accepted by a >>> Function in the D3Hot state. ... >>> >>> Functions that are in D3Hot are permitted to be transitioned by >>> software (writing to their PMCSR PowerState field) to the D0active >>> state or the D0uninitialized state. Functions that are in D3Hot must >>> respond to Configuration Space accesses as long as power and clock >>> are supplied so that they can be returned to D0 by software. >> That applies to the device itself, though, and not to the bus under it. >> >> In general, a bridge in D3hot causes the bus segment on the other side >> of it to be inaccessible even for config space accesses AFAICS. > Oh, thank you! That was the piece I was missing. I'll tweak the > commit log to say something like: > > A bridge in a non-D0 power state does not forward config accesses to > its secondary side (PCIe r6.0, sec 5.3.1). Make sure the bridge is > in D0 while we enumerate devices below it. Thanks for the illustration and tweaking of this. I should have qualified the "devices" in the commit with downstream or secondary to make it less ambiguous. >>>> It can be reproduced like: >>>> >>>> $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/0000:81:00.1/remove >>>> $ echo 1 > /sys/bus/pci/devices/0000:80:00.0/pci_bus/0000:81/rescan >>>> >>>> 0000:80:00.0 is a Root Port and it is runtime-suspended, so >>>> 0000:81:00.1 is unreachable after a rescan. >>>> >>>> Power up the bridge when scanning the child bus and allow it to >>>> suspend again by adding pm_runtime_get_sync()/pm_runtime_put() >>>> in pci_scan_child_bus_extend(). >>>> >>>> Cc: Rafael J. Wysocki <rafael@kernel.org> >>>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> >>>> Cc: Bjorn Helgaas <bhelgaas@google.com> >>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >>>> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>> --- >>>> drivers/pci/probe.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >>>> index 17a969942d37..b108e72b6586 100644 >>>> --- a/drivers/pci/probe.c >>>> +++ b/drivers/pci/probe.c >>>> @@ -2859,11 +2859,20 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, >>>> unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0; >>>> unsigned int start = bus->busn_res.start; >>>> unsigned int devfn, fn, cmax, max = start; >>>> + struct pci_dev *bridge = bus->self; >>>> struct pci_dev *dev; >>>> int nr_devs; >>>> >>>> dev_dbg(&bus->dev, "scanning bus\n"); >>>> >>>> + /* >>>> + * Make sure the bus bridge is powered on, otherwise we may not be >>>> + * able to scan the devices as we may fail to access the configuration >>>> + * space of subordinates. >>>> + */ >>>> + if (bridge) >>>> + pm_runtime_get_sync(&bridge->dev); >>>> + >>>> /* Go find them, Rover! */ >>>> for (devfn = 0; devfn < 256; devfn += 8) { >>>> nr_devs = pci_scan_slot(bus, devfn); >>>> @@ -2976,6 +2985,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, >>>> } >>>> } >>>> >>>> + if (bridge) >>>> + pm_runtime_put(&bridge->dev); >>>> + >>>> /* >>>> * We've scanned the bus and so we know all about what's on >>>> * the other side of any bridges that may be on this bus plus >>>> -- >>>> 2.24.0 >>>>
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 17a969942d37..b108e72b6586 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2859,11 +2859,20 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, unsigned int used_buses, normal_bridges = 0, hotplug_bridges = 0; unsigned int start = bus->busn_res.start; unsigned int devfn, fn, cmax, max = start; + struct pci_dev *bridge = bus->self; struct pci_dev *dev; int nr_devs; dev_dbg(&bus->dev, "scanning bus\n"); + /* + * Make sure the bus bridge is powered on, otherwise we may not be + * able to scan the devices as we may fail to access the configuration + * space of subordinates. + */ + if (bridge) + pm_runtime_get_sync(&bridge->dev); + /* Go find them, Rover! */ for (devfn = 0; devfn < 256; devfn += 8) { nr_devs = pci_scan_slot(bus, devfn); @@ -2976,6 +2985,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, } } + if (bridge) + pm_runtime_put(&bridge->dev); + /* * We've scanned the bus and so we know all about what's on * the other side of any bridges that may be on this bus plus