Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] PCI: Make sure the bus bridge powered on when scanning bus
@ 2020-07-29 11:30 Yicong Yang
  2020-08-21  9:54 ` Yicong Yang
  2020-09-17 21:07 ` Bjorn Helgaas
  0 siblings, 2 replies; 6+ messages in thread
From: Yicong Yang @ 2020-07-29 11:30 UTC (permalink / raw)
  To: bhelgaas, linux-pci; +Cc: linuxarm, yangyicong

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 root port and is runtime suspended and we cannot
get 0000:81:00.1 after rescan.

Make bridge powered on when scanning the child bus, by adding
pm_runtime_get_sync()/pm_runtime_put() in pci_scan_child_bus_extend().

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/pci/probe.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2f66988..5bb502b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2795,6 +2795,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 
 	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 (bus->self)
+		pm_runtime_get_sync(&bus->self->dev);
+
 	/* Go find them, Rover! */
 	for (devfn = 0; devfn < 256; devfn += 8) {
 		nr_devs = pci_scan_slot(bus, devfn);
@@ -2907,6 +2915,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 		}
 	}
 
+	if (bus->self)
+		pm_runtime_put(&bus->self->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.8.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI: Make sure the bus bridge powered on when scanning bus
  2020-07-29 11:30 [PATCH] PCI: Make sure the bus bridge powered on when scanning bus Yicong Yang
@ 2020-08-21  9:54 ` Yicong Yang
  2020-09-17 21:07 ` Bjorn Helgaas
  1 sibling, 0 replies; 6+ messages in thread
From: Yicong Yang @ 2020-08-21  9:54 UTC (permalink / raw)
  To: bhelgaas, linux-pci, Bjorn Helgaas; +Cc: linuxarm

gentle ping ...

Any comments on this or is it possible to be merged?


On 2020/7/29 19:30, 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 root port and is runtime suspended and we cannot
> get 0000:81:00.1 after rescan.
>
> Make bridge powered on when scanning the child bus, by adding
> pm_runtime_get_sync()/pm_runtime_put() in pci_scan_child_bus_extend().
>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/pci/probe.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 2f66988..5bb502b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2795,6 +2795,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  
>  	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 (bus->self)
> +		pm_runtime_get_sync(&bus->self->dev);
> +
>  	/* Go find them, Rover! */
>  	for (devfn = 0; devfn < 256; devfn += 8) {
>  		nr_devs = pci_scan_slot(bus, devfn);
> @@ -2907,6 +2915,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  		}
>  	}
>  
> +	if (bus->self)
> +		pm_runtime_put(&bus->self->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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI: Make sure the bus bridge powered on when scanning bus
  2020-07-29 11:30 [PATCH] PCI: Make sure the bus bridge powered on when scanning bus Yicong Yang
  2020-08-21  9:54 ` Yicong Yang
@ 2020-09-17 21:07 ` Bjorn Helgaas
  2020-09-18  9:31   ` Yicong Yang
  1 sibling, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2020-09-17 21:07 UTC (permalink / raw)
  To: Yicong Yang
  Cc: bhelgaas, linux-pci, linuxarm, Mika Westerberg,
	Rafael J. Wysocki, Peter Wu

[+cc Mika, Rafael, Peter]

On Wed, Jul 29, 2020 at 07:30:23PM +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.
> 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 root port and is runtime suspended and we cannot
> get 0000:81:00.1 after rescan.
> 
> Make bridge powered on when scanning the child bus, by adding
> pm_runtime_get_sync()/pm_runtime_put() in pci_scan_child_bus_extend().
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> ---
>  drivers/pci/probe.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 2f66988..5bb502b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2795,6 +2795,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  
>  	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 (bus->self)
> +		pm_runtime_get_sync(&bus->self->dev);

I think if we do this, we should be able to remove the call from
pci_scan_bridge() added by d963f6512e15 ("PCI: Power on bridges before
scanning new devices"), right?

The reason we need it here is because there are two paths to
pci_scan_child_bus_extend() and only one of them calls
pm_runtime_get_sync():

  pci_scan_bridge_extend
    pm_runtime_get_sync
    pci_scan_child_bus_extend

  pci_scan_child_bus
    pci_scan_child_bus_extend

If we move the pm_runtime_get_sync() from pci_scan_bridge_extend() to
pci_scan_child_bus_extend(), both paths should be safe.

>  	/* Go find them, Rover! */
>  	for (devfn = 0; devfn < 256; devfn += 8) {
>  		nr_devs = pci_scan_slot(bus, devfn);
> @@ -2907,6 +2915,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  		}
>  	}
>  
> +	if (bus->self)
> +		pm_runtime_put(&bus->self->dev);

I would probably do this:

  struct pci_dev *bridge = bus->self;

  if (bridge)
    pm_runtime_get_sync(&bridge->dev);
  ...
  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.8.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI: Make sure the bus bridge powered on when scanning bus
  2020-09-17 21:07 ` Bjorn Helgaas
@ 2020-09-18  9:31   ` Yicong Yang
  2020-09-18 16:17     ` Bjorn Helgaas
  0 siblings, 1 reply; 6+ messages in thread
From: Yicong Yang @ 2020-09-18  9:31 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: bhelgaas, linux-pci, linuxarm, Mika Westerberg,
	Rafael J. Wysocki, Peter Wu

On 2020/9/18 5:07, Bjorn Helgaas wrote:
> [+cc Mika, Rafael, Peter]
>
> On Wed, Jul 29, 2020 at 07:30:23PM +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.
>> 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 root port and is runtime suspended and we cannot
>> get 0000:81:00.1 after rescan.
>>
>> Make bridge powered on when scanning the child bus, by adding
>> pm_runtime_get_sync()/pm_runtime_put() in pci_scan_child_bus_extend().
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>> ---
>>  drivers/pci/probe.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 2f66988..5bb502b 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2795,6 +2795,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>>  
>>  	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 (bus->self)
>> +		pm_runtime_get_sync(&bus->self->dev);
> I think if we do this, we should be able to remove the call from
> pci_scan_bridge() added by d963f6512e15 ("PCI: Power on bridges before
> scanning new devices"), right?
>
> The reason we need it here is because there are two paths to
> pci_scan_child_bus_extend() and only one of them calls
> pm_runtime_get_sync():
>
>   pci_scan_bridge_extend
>     pm_runtime_get_sync
>     pci_scan_child_bus_extend
>
>   pci_scan_child_bus
>     pci_scan_child_bus_extend
>
> If we move the pm_runtime_get_sync() from pci_scan_bridge_extend() to
> pci_scan_child_bus_extend(), both paths should be safe.

A bit different, I think. The issue I met is a bit different from Mika, as
we go through different sysfs files. Think about rescanning device under a 
root port,

when echo 1 > /sysfs/bus/pci/devices/${RootPort}/rescan:

rescan_restore()
  pci_rescan_bus(pdev->bus) /* we will rescan the root bus */
    pci_rescan_child_bus()
      pci_scan_child_bus_extend()  /* we cannot wake up the bus bridge here as is on the root bus */
        pci_scan_bridge_extend() /* we have to wake up the root port here */

when echo 1 > /sysfs/bus/pci/devices/${RootPort}/pci_bus/${PciBus}/rescan:

rescan_restore()
  pci_rescan_bus(bus) /* we will rescan the bus of the root port */
    pci_rescan_child_bus()
      pci_scan_child_bus_extend() /* we can wake up the bus bridge - root port here */

As different bus is rescanned, so it'll have problem without patch d963f6512e15.


>
>>  	/* Go find them, Rover! */
>>  	for (devfn = 0; devfn < 256; devfn += 8) {
>>  		nr_devs = pci_scan_slot(bus, devfn);
>> @@ -2907,6 +2915,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>>  		}
>>  	}
>>  
>> +	if (bus->self)
>> +		pm_runtime_put(&bus->self->dev);
> I would probably do this:
>
>   struct pci_dev *bridge = bus->self;
>
>   if (bridge)
>     pm_runtime_get_sync(&bridge->dev);
>   ...
>   if (bridge)
>     pm_runtime_put(&bridge->dev);

Sure.

Regards,
Yicong


>
>>  	/*
>>  	 * 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.8.1
>>
> .
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI: Make sure the bus bridge powered on when scanning bus
  2020-09-18  9:31   ` Yicong Yang
@ 2020-09-18 16:17     ` Bjorn Helgaas
  2020-09-19 10:22       ` Yicong Yang
  0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2020-09-18 16:17 UTC (permalink / raw)
  To: Yicong Yang
  Cc: bhelgaas, linux-pci, linuxarm, Mika Westerberg,
	Rafael J. Wysocki, Peter Wu

On Fri, Sep 18, 2020 at 05:31:54PM +0800, Yicong Yang wrote:
> On 2020/9/18 5:07, Bjorn Helgaas wrote:
> > On Wed, Jul 29, 2020 at 07:30:23PM +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.
> >> 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 root port and is runtime suspended and we cannot
> >> get 0000:81:00.1 after rescan.
> >>
> >> Make bridge powered on when scanning the child bus, by adding
> >> pm_runtime_get_sync()/pm_runtime_put() in pci_scan_child_bus_extend().
> >>
> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> >> ---
> >>  drivers/pci/probe.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> >> index 2f66988..5bb502b 100644
> >> --- a/drivers/pci/probe.c
> >> +++ b/drivers/pci/probe.c
> >> @@ -2795,6 +2795,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
> >>  
> >>  	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 (bus->self)
> >> +		pm_runtime_get_sync(&bus->self->dev);
> >
> > I think if we do this, we should be able to remove the call from
> > pci_scan_bridge() added by d963f6512e15 ("PCI: Power on bridges before
> > scanning new devices"), right?
> >
> > The reason we need it here is because there are two paths to
> > pci_scan_child_bus_extend() and only one of them calls
> > pm_runtime_get_sync():
> >
> >   pci_scan_bridge_extend
> >     pm_runtime_get_sync
> >     pci_scan_child_bus_extend
> >
> >   pci_scan_child_bus
> >     pci_scan_child_bus_extend
> >
> > If we move the pm_runtime_get_sync() from pci_scan_bridge_extend() to
> > pci_scan_child_bus_extend(), both paths should be safe.
> 
> A bit different, I think. The issue I met is a bit different from
> Mika, as we go through different sysfs files. Think about rescanning
> device under a root port,
> 
> when echo 1 > /sysfs/bus/pci/devices/${RootPort}/rescan:
> 
> rescan_store()
>   pci_rescan_bus(pdev->bus) /* we will rescan the root bus */
>     pci_rescan_child_bus()
>       pci_scan_child_bus_extend()  /* we cannot wake up the bus bridge here as is on the root bus */
>         pci_scan_bridge_extend() /* we have to wake up the root port here */
> 
> when echo 1 > /sysfs/bus/pci/devices/${RootPort}/pci_bus/${PciBus}/rescan:
> 
> rescan_store()
>   pci_rescan_bus(bus) /* we will rescan the bus of the root port */
>     pci_rescan_child_bus()
>       pci_scan_child_bus_extend() /* we can wake up the bus bridge - root port here */
> 
> As different bus is rescanned, so it'll have problem without patch
> d963f6512e15.

Sorry, I didn't quite follow the above.

The problem here is about scanning a bridge's secondary bus when the
bridge may be runtime-suspended.  The bridge may be in D0, D1, D2, or
D3hot.  It is not in D3cold.  pm_runtime_get_sync() brings a device
that may have been runtime-suspended back to D0.

All PCI devices respond to config accesses when they are in D0, D1,
D2, or D3hot [1], so we don't need pm_runtime_get_sync() to access a
bridge's config space.

But when a bridge is not in D0, it does not initiate transactions on
its secondary bus [2], so we do need pm_runtime_get_sync() before we
attempt config accesses for any children.

pci_scan_bridge_extend() does not directly do anything with the
secondary bus, which is why I'm not sure it needs
pm_runtime_get_sync().

The accesses to the secondary bus are in pci_scan_slot(), so the
pm_runtime_get_sync() you added immediately before calling
pci_scan_slot() makes sense to me.  Although possibly it could go in
pci_scan_slot() itself, since there are several other callers.

[1] PCIe r5.0, sec 5.3.1.4.1
[2] PCIe r5.0, sec 5.3.1 implementation note

> >>  	/* Go find them, Rover! */
> >>  	for (devfn = 0; devfn < 256; devfn += 8) {
> >>  		nr_devs = pci_scan_slot(bus, devfn);
> >> @@ -2907,6 +2915,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
> >>  		}
> >>  	}
> >>  
> >> +	if (bus->self)
> >> +		pm_runtime_put(&bus->self->dev);
> > I would probably do this:
> >
> >   struct pci_dev *bridge = bus->self;
> >
> >   if (bridge)
> >     pm_runtime_get_sync(&bridge->dev);
> >   ...
> >   if (bridge)
> >     pm_runtime_put(&bridge->dev);
> 
> Sure.
> 
> Regards,
> Yicong
> 
> 
> >
> >>  	/*
> >>  	 * 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.8.1
> >>
> > .
> >
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] PCI: Make sure the bus bridge powered on when scanning bus
  2020-09-18 16:17     ` Bjorn Helgaas
@ 2020-09-19 10:22       ` Yicong Yang
  0 siblings, 0 replies; 6+ messages in thread
From: Yicong Yang @ 2020-09-19 10:22 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: bhelgaas, linux-pci, linuxarm, Mika Westerberg,
	Rafael J. Wysocki, Peter Wu

On 2020/9/19 0:17, Bjorn Helgaas wrote:

> On Fri, Sep 18, 2020 at 05:31:54PM +0800, Yicong Yang wrote:
>> On 2020/9/18 5:07, Bjorn Helgaas wrote:
>>> On Wed, Jul 29, 2020 at 07:30:23PM +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.
>>>> 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 root port and is runtime suspended and we cannot
>>>> get 0000:81:00.1 after rescan.
>>>>
>>>> Make bridge powered on when scanning the child bus, by adding
>>>> pm_runtime_get_sync()/pm_runtime_put() in pci_scan_child_bus_extend().
>>>>
>>>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
>>>> ---
>>>>  drivers/pci/probe.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>> index 2f66988..5bb502b 100644
>>>> --- a/drivers/pci/probe.c
>>>> +++ b/drivers/pci/probe.c
>>>> @@ -2795,6 +2795,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>>>>  
>>>>  	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 (bus->self)
>>>> +		pm_runtime_get_sync(&bus->self->dev);
>>> I think if we do this, we should be able to remove the call from
>>> pci_scan_bridge() added by d963f6512e15 ("PCI: Power on bridges before
>>> scanning new devices"), right?
>>>
>>> The reason we need it here is because there are two paths to
>>> pci_scan_child_bus_extend() and only one of them calls
>>> pm_runtime_get_sync():
>>>
>>>   pci_scan_bridge_extend
>>>     pm_runtime_get_sync
>>>     pci_scan_child_bus_extend
>>>
>>>   pci_scan_child_bus
>>>     pci_scan_child_bus_extend
>>>
>>> If we move the pm_runtime_get_sync() from pci_scan_bridge_extend() to
>>> pci_scan_child_bus_extend(), both paths should be safe.
>> A bit different, I think. The issue I met is a bit different from
>> Mika, as we go through different sysfs files. Think about rescanning
>> device under a root port,
>>
>> when echo 1 > /sysfs/bus/pci/devices/${RootPort}/rescan:
>>
>> rescan_store()
>>   pci_rescan_bus(pdev->bus) /* we will rescan the root bus */
>>     pci_rescan_child_bus()
>>       pci_scan_child_bus_extend()  /* we cannot wake up the bus bridge here as is on the root bus */
>>         pci_scan_bridge_extend() /* we have to wake up the root port here */
>>
>> when echo 1 > /sysfs/bus/pci/devices/${RootPort}/pci_bus/${PciBus}/rescan:
>>
>> rescan_store()
>>   pci_rescan_bus(bus) /* we will rescan the bus of the root port */
>>     pci_rescan_child_bus()
>>       pci_scan_child_bus_extend() /* we can wake up the bus bridge - root port here */
>>
>> As different bus is rescanned, so it'll have problem without patch
>> d963f6512e15.
> Sorry, I didn't quite follow the above.
>
> The problem here is about scanning a bridge's secondary bus when the
> bridge may be runtime-suspended.  The bridge may be in D0, D1, D2, or
> D3hot.  It is not in D3cold.  pm_runtime_get_sync() brings a device
> that may have been runtime-suspended back to D0.
>
> All PCI devices respond to config accesses when they are in D0, D1,
> D2, or D3hot [1], so we don't need pm_runtime_get_sync() to access a
> bridge's config space.
>
> But when a bridge is not in D0, it does not initiate transactions on
> its secondary bus [2], so we do need pm_runtime_get_sync() before we
> attempt config accesses for any children.
>
> pci_scan_bridge_extend() does not directly do anything with the
> secondary bus, which is why I'm not sure it needs
> pm_runtime_get_sync().
>
> The accesses to the secondary bus are in pci_scan_slot(), so the
> pm_runtime_get_sync() you added immediately before calling
> pci_scan_slot() makes sense to me.  Although possibly it could go in
> pci_scan_slot() itself, since there are several other callers.
>
> [1] PCIe r5.0, sec 5.3.1.4.1
> [2] PCIe r5.0, sec 5.3.1 implementation note

yes it's right. with this patch and patch d963f6512e15 removed, the rescan
works well and I've tested it. I didn't understand it correctly in my last
reply, sorry. I'll remove d963f6512e15 in v2 patch.

If we put the pm_runtime_get/put in the pci_scan_slot(), we'll get/put the
bridge many times. so maybe put it immediately before pci_scan_slot() is
better?

Thanks,
Yicong


>
>>>>  	/* Go find them, Rover! */
>>>>  	for (devfn = 0; devfn < 256; devfn += 8) {
>>>>  		nr_devs = pci_scan_slot(bus, devfn);
>>>> @@ -2907,6 +2915,9 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>>>>  		}
>>>>  	}
>>>>  
>>>> +	if (bus->self)
>>>> +		pm_runtime_put(&bus->self->dev);
>>> I would probably do this:
>>>
>>>   struct pci_dev *bridge = bus->self;
>>>
>>>   if (bridge)
>>>     pm_runtime_get_sync(&bridge->dev);
>>>   ...
>>>   if (bridge)
>>>     pm_runtime_put(&bridge->dev);
>> Sure.
>>
>> Regards,
>> Yicong
>>
>>
>>>>  	/*
>>>>  	 * 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.8.1
>>>>
>>> .
>>>
> .
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-29 11:30 [PATCH] PCI: Make sure the bus bridge powered on when scanning bus Yicong Yang
2020-08-21  9:54 ` Yicong Yang
2020-09-17 21:07 ` Bjorn Helgaas
2020-09-18  9:31   ` Yicong Yang
2020-09-18 16:17     ` Bjorn Helgaas
2020-09-19 10:22       ` Yicong Yang

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git