diff mbox

[v5,4/9] ACPI / hotplug / PCI: Do not scan all bridges when native PCIe hotplug is used

Message ID 20180416103453.46232-5-mika.westerberg@linux.intel.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Mika Westerberg April 16, 2018, 10:34 a.m. UTC
When a system is using native PCIe hotplug for Thunderbolt and the
controller is not enabled for full RTD3 (runtime D3) it will be only
present in the system when there is a device connected. This pretty much
follows the BIOS assisted hotplug behaviour.

Thunderbolt host router integrated PCIe switch has two additional PCIe
downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
(USB 3 host controller) respectively. These downstream bridges are not
marked being hotplug capable. Reason for that is to preserve resources.
Otherwise the OS would distribute remaining resources between all
downstream bridges making these two bridges consume precious resources
of the actual hotplug bridges.

Now, because these two bridges are not marked being hotplug capable the OS
will not enable hotplug interrupt for them either and will not receive
interrupt when devices behind them are hot-added. Solution to this is
that the BIOS sends ACPI Notify() to the root port let the OS know it
needs to rescan for added and/or removed devices.

Here is how the mechanism is supposed to work when a Thunderbolt
endpoint is connected to one of the ports. In case of a standard USB-C
device only the xHCI is hot-added otherwise steps are the same.

1. Initially there is only the PCIe root port that is controlled by
   the pciehp driver

  00:1b.0 (Hotplug+) --

2. Then we get native PCIe hotplug interrupt and once it is handled the
   topology looks as following

  00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 --

3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
   they don't have anything behind them currently. Bridge 02:01.0 is
   hotplug capable and used for extending the topology. At this point
   the required PCIe devices are enabled and ACPI Notify() is sent to
   the root port. The resulting topology is expected to look like

  00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 -- xHCI host controller

However, the current ACPI hotplug implementation scans the whole 00:1b.0
hotplug slot and everything behind it regardless whether native PCIe is
used or not, and it expects that the BIOS has configured bridge
resources upfront. If that's not the case it assigns resources using
minimal allocation (everything currently found just barely fit)
preventing future extension. In addition to that, if there is another
native PCIe hotplug going on we may find the new PCIe switch only
partially ready (all links are not fully trained yet) confusing pciehp
when it finally starts to enumerate for new devices.

To make this work better with the native PCIe hotplug driver (pciehp),
we let it handle all slot management and resource allocation for hotplug
bridges and restrict ACPI hotplug to non-hotplug bridges.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/pci/hotplug/acpiphp.h      |  1 +
 drivers/pci/hotplug/acpiphp_glue.c | 73 ++++++++++++++++++++++++++++++--------
 2 files changed, 59 insertions(+), 15 deletions(-)

Comments

Bjorn Helgaas May 2, 2018, 8:49 p.m. UTC | #1
On Mon, Apr 16, 2018 at 01:34:48PM +0300, Mika Westerberg wrote:
> When a system is using native PCIe hotplug for Thunderbolt and the
> controller is not enabled for full RTD3 (runtime D3) it will be only
> present in the system when there is a device connected. This pretty much
> follows the BIOS assisted hotplug behaviour.

This is a tangent, but what exactly does "controller is not enabled
for full RTD3 (runtime D3)" mean?  The way that's worded sounds like
it *could* be a setting in a PCI config register, but I suspect it's
really something in Linux, e.g., some bit in struct pci_dev or device?

> Thunderbolt host router integrated PCIe switch has two additional PCIe
> downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
> (USB 3 host controller) respectively. These downstream bridges are not
> marked being hotplug capable. Reason for that is to preserve resources.
> Otherwise the OS would distribute remaining resources between all
> downstream bridges making these two bridges consume precious resources
> of the actual hotplug bridges.
> 
> Now, because these two bridges are not marked being hotplug capable the OS
> will not enable hotplug interrupt for them either and will not receive
> interrupt when devices behind them are hot-added. Solution to this is
> that the BIOS sends ACPI Notify() to the root port let the OS know it
> needs to rescan for added and/or removed devices.

We're building stuff based on "is_hotplug_bridge", but I'm not sure
that bit is really useful.

set_pcie_hotplug_bridge() sets it for downstream ports with
PCI_EXP_SLTCAP_HPC, which is easy enough to understand.

In acpiphp, check_hotplug_bridge() sets in some scenario that I can't
quite figure out.  I assume it's based on something in the namespace.

But it seems like the whole point of this patch is that even if a
bridge doesn't have "is_hotplug_bridge" set, ACPI hotplug can hot-add
devices below it.

So I'm not sure what "is_hotplug_bridge" is really telling us.  Is it
just a hint about how many resources we want to assign to the bridge,
i.e., we assign only a few when it's not set and more if it is set?

> Here is how the mechanism is supposed to work when a Thunderbolt
> endpoint is connected to one of the ports. In case of a standard USB-C
> device only the xHCI is hot-added otherwise steps are the same.
> 
> 1. Initially there is only the PCIe root port that is controlled by
>    the pciehp driver
> 
>   00:1b.0 (Hotplug+) --
> 
> 2. Then we get native PCIe hotplug interrupt and once it is handled the
>    topology looks as following
> 
>   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
>                                   +- 02:01.0 (HotPlug+)
>                                   \- 02:02.0 --
> 
> 3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and

By "not marked as hotplug capable", I guess you mean
PCI_EXP_SLTCAP_HPC is not set, right?

>    they don't have anything behind them currently. Bridge 02:01.0 is
>    hotplug capable and used for extending the topology. At this point
>    the required PCIe devices are enabled and ACPI Notify() is sent to
>    the root port. The resulting topology is expected to look like
> 
>   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
>                                   +- 02:01.0 (HotPlug+)
>                                   \- 02:02.0 -- xHCI host controller
> 
> However, the current ACPI hotplug implementation scans the whole 00:1b.0
> hotplug slot and everything behind it regardless whether native PCIe is
> used or not, and it expects that the BIOS has configured bridge
> resources upfront. If that's not the case it assigns resources using
> minimal allocation (everything currently found just barely fit)
> preventing future extension. In addition to that, if there is another
> native PCIe hotplug going on we may find the new PCIe switch only
> partially ready (all links are not fully trained yet) confusing pciehp
> when it finally starts to enumerate for new devices.
> 
> To make this work better with the native PCIe hotplug driver (pciehp),
> we let it handle all slot management and resource allocation for hotplug
> bridges and restrict ACPI hotplug to non-hotplug bridges.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/hotplug/acpiphp.h      |  1 +
>  drivers/pci/hotplug/acpiphp_glue.c | 73 ++++++++++++++++++++++++++++++--------
>  2 files changed, 59 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
> index e438a2d734f2..8dcfd623ef1d 100644
> --- a/drivers/pci/hotplug/acpiphp.h
> +++ b/drivers/pci/hotplug/acpiphp.h
> @@ -158,6 +158,7 @@ struct acpiphp_attention_info
>  
>  #define SLOT_ENABLED		(0x00000001)
>  #define SLOT_IS_GOING_AWAY	(0x00000002)
> +#define SLOT_IS_NATIVE		(0x00000004)
>  
>  /* function flags */
>  
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index b45b375c0e6c..5efa21cdddc9 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -282,6 +282,9 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
>  	slot->device = device;
>  	INIT_LIST_HEAD(&slot->funcs);
>  
> +	if (pdev && pciehp_is_native(pdev))
> +		slot->flags |= SLOT_IS_NATIVE;

If I understand correctly, pciehp_is_native() checks
root->osc_control_set, so for everything under the same host bridge
(PNP0A03 device), it gives the same answer.

If so, the name "SLOT_IS_NATIVE" seems a little misleading because
there are very often several slots under the same host bridge, and
they will be either all native or all non-native.

But I must be missing something because it seems like the whole point
of this patch is to treat part of the Thunderbolt host router as
native and another part as non-native.  Help, I'm confused :)

>  	list_add_tail(&slot->node, &bridge->slots);
>  
>  	/*
> @@ -291,7 +294,7 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
>  	 * expose slots to user space in those cases.
>  	 */
>  	if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
> -	    && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
> +	    && !(slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge)) {
>  		unsigned long long sun;
>  		int retval;
>  
> @@ -430,6 +433,29 @@ static int acpiphp_rescan_slot(struct acpiphp_slot *slot)
>  	return pci_scan_slot(slot->bus, PCI_DEVFN(slot->device, 0));
>  }
>  
> +static void acpiphp_native_scan_bridge(struct pci_dev *bridge)
> +{
> +	struct pci_bus *bus = bridge->subordinate;
> +	struct pci_dev *dev;
> +	int max;
> +
> +	if (!bus)
> +		return;
> +
> +	max = bus->busn_res.start;
> +	/* Scan already configured non-hotplug bridges */
> +	for_each_pci_bridge(dev, bus) {
> +		if (!dev->is_hotplug_bridge)
> +			max = pci_scan_bridge(bus, dev, max, 0);
> +	}
> +
> +	/* Scan non-hotplug bridges that need to be reconfigured */
> +	for_each_pci_bridge(dev, bus) {
> +		if (!dev->is_hotplug_bridge)
> +			max = pci_scan_bridge(bus, dev, max, 1);
> +	}
> +}
> +
>  /**
>   * enable_slot - enable, configure a slot
>   * @slot: slot to be enabled
> @@ -442,25 +468,42 @@ static void enable_slot(struct acpiphp_slot *slot)
>  	struct pci_dev *dev;
>  	struct pci_bus *bus = slot->bus;
>  	struct acpiphp_func *func;
> -	int max, pass;
> -	LIST_HEAD(add_list);
>  
> -	acpiphp_rescan_slot(slot);
> -	max = acpiphp_max_busnr(bus);
> -	for (pass = 0; pass < 2; pass++) {
> +	if (slot->flags & SLOT_IS_NATIVE) {
> +		/*
> +		 * If native PCIe hotplug is used, it will take care of
> +		 * hotplug slot management and resource allocation for
> +		 * hotplug bridges. However, ACPI hotplug may still be
> +		 * used for non-hotplug bridges to bring in additional
> +		 * devices such as Thunderbolt host controller.
> +		 */
>  		for_each_pci_bridge(dev, bus) {
> -			if (PCI_SLOT(dev->devfn) != slot->device)
> -				continue;
> -
> -			max = pci_scan_bridge(bus, dev, max, pass);
> -			if (pass && dev->subordinate) {
> -				check_hotplug_bridge(slot, dev);
> -				pcibios_resource_survey_bus(dev->subordinate);
> -				__pci_bus_size_bridges(dev->subordinate, &add_list);
> +			if (PCI_SLOT(dev->devfn) == slot->device)
> +				acpiphp_native_scan_bridge(dev);
> +		}
> +		pci_assign_unassigned_bridge_resources(bus->self);
> +	} else {
> +		LIST_HEAD(add_list);
> +		int max, pass;
> +
> +		acpiphp_rescan_slot(slot);
> +		max = acpiphp_max_busnr(bus);
> +		for (pass = 0; pass < 2; pass++) {
> +			for_each_pci_bridge(dev, bus) {
> +				if (PCI_SLOT(dev->devfn) != slot->device)
> +					continue;
> +
> +				max = pci_scan_bridge(bus, dev, max, pass);
> +				if (pass && dev->subordinate) {
> +					check_hotplug_bridge(slot, dev);
> +					pcibios_resource_survey_bus(dev->subordinate);
> +					__pci_bus_size_bridges(dev->subordinate,
> +							       &add_list);
> +				}
>  			}
>  		}
> +		__pci_bus_assign_resources(bus, &add_list, NULL);
>  	}
> -	__pci_bus_assign_resources(bus, &add_list, NULL);
>  
>  	acpiphp_sanitize_bus(bus);
>  	pcie_bus_configure_settings(bus);
> -- 
> 2.16.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg May 3, 2018, 10:22 a.m. UTC | #2
On Wed, May 02, 2018 at 03:49:32PM -0500, Bjorn Helgaas wrote:
> On Mon, Apr 16, 2018 at 01:34:48PM +0300, Mika Westerberg wrote:
> > When a system is using native PCIe hotplug for Thunderbolt and the
> > controller is not enabled for full RTD3 (runtime D3) it will be only
> > present in the system when there is a device connected. This pretty much
> > follows the BIOS assisted hotplug behaviour.
> 
> This is a tangent, but what exactly does "controller is not enabled
> for full RTD3 (runtime D3)" mean?  The way that's worded sounds like
> it *could* be a setting in a PCI config register, but I suspect it's
> really something in Linux, e.g., some bit in struct pci_dev or device?

It means that the controller can to into D3 runtime. With BIOS assisted
mode and native PCIe mode, the controller is hot-removed when there is
nothing connected. In RTD3 mode it is there always and the OS expected
to put it into D3 when idle using standard PCI PM registers.

> > Thunderbolt host router integrated PCIe switch has two additional PCIe
> > downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
> > (USB 3 host controller) respectively. These downstream bridges are not
> > marked being hotplug capable. Reason for that is to preserve resources.
> > Otherwise the OS would distribute remaining resources between all
> > downstream bridges making these two bridges consume precious resources
> > of the actual hotplug bridges.
> > 
> > Now, because these two bridges are not marked being hotplug capable the OS
> > will not enable hotplug interrupt for them either and will not receive
> > interrupt when devices behind them are hot-added. Solution to this is
> > that the BIOS sends ACPI Notify() to the root port let the OS know it
> > needs to rescan for added and/or removed devices.
> 
> We're building stuff based on "is_hotplug_bridge", but I'm not sure
> that bit is really useful.
> 
> set_pcie_hotplug_bridge() sets it for downstream ports with
> PCI_EXP_SLTCAP_HPC, which is easy enough to understand.
> 
> In acpiphp, check_hotplug_bridge() sets in some scenario that I can't
> quite figure out.  I assume it's based on something in the namespace.
> 
> But it seems like the whole point of this patch is that even if a
> bridge doesn't have "is_hotplug_bridge" set, ACPI hotplug can hot-add
> devices below it.

Correct.

> So I'm not sure what "is_hotplug_bridge" is really telling us.  Is it
> just a hint about how many resources we want to assign to the bridge,
> i.e., we assign only a few when it's not set and more if it is set?

Good question. I did not invent the flag so hard to say. I've been using
it because IIRC you prefer it.

> > Here is how the mechanism is supposed to work when a Thunderbolt
> > endpoint is connected to one of the ports. In case of a standard USB-C
> > device only the xHCI is hot-added otherwise steps are the same.
> > 
> > 1. Initially there is only the PCIe root port that is controlled by
> >    the pciehp driver
> > 
> >   00:1b.0 (Hotplug+) --
> > 
> > 2. Then we get native PCIe hotplug interrupt and once it is handled the
> >    topology looks as following
> > 
> >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
> >                                   +- 02:01.0 (HotPlug+)
> >                                   \- 02:02.0 --
> > 
> > 3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
> 
> By "not marked as hotplug capable", I guess you mean
> PCI_EXP_SLTCAP_HPC is not set, right?

Right.

> >    they don't have anything behind them currently. Bridge 02:01.0 is
> >    hotplug capable and used for extending the topology. At this point
> >    the required PCIe devices are enabled and ACPI Notify() is sent to
> >    the root port. The resulting topology is expected to look like
> > 
> >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
> >                                   +- 02:01.0 (HotPlug+)
> >                                   \- 02:02.0 -- xHCI host controller
> > 
> > However, the current ACPI hotplug implementation scans the whole 00:1b.0
> > hotplug slot and everything behind it regardless whether native PCIe is
> > used or not, and it expects that the BIOS has configured bridge
> > resources upfront. If that's not the case it assigns resources using
> > minimal allocation (everything currently found just barely fit)
> > preventing future extension. In addition to that, if there is another
> > native PCIe hotplug going on we may find the new PCIe switch only
> > partially ready (all links are not fully trained yet) confusing pciehp
> > when it finally starts to enumerate for new devices.
> > 
> > To make this work better with the native PCIe hotplug driver (pciehp),
> > we let it handle all slot management and resource allocation for hotplug
> > bridges and restrict ACPI hotplug to non-hotplug bridges.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/pci/hotplug/acpiphp.h      |  1 +
> >  drivers/pci/hotplug/acpiphp_glue.c | 73 ++++++++++++++++++++++++++++++--------
> >  2 files changed, 59 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
> > index e438a2d734f2..8dcfd623ef1d 100644
> > --- a/drivers/pci/hotplug/acpiphp.h
> > +++ b/drivers/pci/hotplug/acpiphp.h
> > @@ -158,6 +158,7 @@ struct acpiphp_attention_info
> >  
> >  #define SLOT_ENABLED		(0x00000001)
> >  #define SLOT_IS_GOING_AWAY	(0x00000002)
> > +#define SLOT_IS_NATIVE		(0x00000004)
> >  
> >  /* function flags */
> >  
> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > index b45b375c0e6c..5efa21cdddc9 100644
> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -282,6 +282,9 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
> >  	slot->device = device;
> >  	INIT_LIST_HEAD(&slot->funcs);
> >  
> > +	if (pdev && pciehp_is_native(pdev))
> > +		slot->flags |= SLOT_IS_NATIVE;
> 
> If I understand correctly, pciehp_is_native() checks
> root->osc_control_set, so for everything under the same host bridge
> (PNP0A03 device), it gives the same answer.

Correct.

> If so, the name "SLOT_IS_NATIVE" seems a little misleading because
> there are very often several slots under the same host bridge, and
> they will be either all native or all non-native.

I used it because it works with pciehp_is_native(). I can rename it if
you have a better name.

> But I must be missing something because it seems like the whole point
> of this patch is to treat part of the Thunderbolt host router as
> native and another part as non-native.  Help, I'm confused :)

No, you are not confused. It is exactly what we are trying to fix with
this patch. These are systems (already out there in the wild) that are
in "native" mode. As I tried to explain in the changelog hotplugging
PCIe devices to hotplug PCIe downstream ports is done using native PCIe
hotplug. However, xHCI (USB 3) controller and NHI (Thunderbolt
controller) are hot-added using ACPI Notify(). Before you ask, no it is
not violating any spec because these two downstream ports are not under
control of pciehp (they don't have hotplug slot capability set):

  00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 -- xHCI host controller

Hope this clarifies.
Bjorn Helgaas May 5, 2018, 12:04 a.m. UTC | #3
On Thu, May 03, 2018 at 01:22:41PM +0300, Mika Westerberg wrote:
> On Wed, May 02, 2018 at 03:49:32PM -0500, Bjorn Helgaas wrote:
> > On Mon, Apr 16, 2018 at 01:34:48PM +0300, Mika Westerberg wrote:
> > > When a system is using native PCIe hotplug for Thunderbolt and the
> > > controller is not enabled for full RTD3 (runtime D3) it will be only
> > > present in the system when there is a device connected. This pretty much
> > > follows the BIOS assisted hotplug behaviour.
> > 
> > This is a tangent, but what exactly does "controller is not enabled
> > for full RTD3 (runtime D3)" mean?  The way that's worded sounds like
> > it *could* be a setting in a PCI config register, but I suspect it's
> > really something in Linux, e.g., some bit in struct pci_dev or device?
> 
> It means that the controller can to into D3 runtime. With BIOS assisted
> mode and native PCIe mode, the controller is hot-removed when there is
> nothing connected. In RTD3 mode it is there always and the OS expected
> to put it into D3 when idle using standard PCI PM registers.

How do I tell if a controller is enabled for runtime D3?

I'm still struggling to figure out exactly how this runtime D3 part is
relevant to this patch.  It might be behavior that happens in this
particular scenario, but I'm not sure yet that it is a required part
of the picture.

I think you're trying to separate enumeration handled by pciehp from
enumeration handled by acpiphp, and runtime D3 sounds like an
orthogonal issue.

> > > Thunderbolt host router integrated PCIe switch has two additional PCIe
> > > downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
> > > (USB 3 host controller) respectively. These downstream bridges are not
> > > marked being hotplug capable. Reason for that is to preserve resources.
> > > Otherwise the OS would distribute remaining resources between all
> > > downstream bridges making these two bridges consume precious resources
> > > of the actual hotplug bridges.
> > > 
> > > Now, because these two bridges are not marked being hotplug capable the OS
> > > will not enable hotplug interrupt for them either and will not receive
> > > interrupt when devices behind them are hot-added. Solution to this is
> > > that the BIOS sends ACPI Notify() to the root port let the OS know it
> > > needs to rescan for added and/or removed devices.
> > 
> > We're building stuff based on "is_hotplug_bridge", but I'm not sure
> > that bit is really useful.
> > 
> > set_pcie_hotplug_bridge() sets it for downstream ports with
> > PCI_EXP_SLTCAP_HPC, which is easy enough to understand.
> > 
> > In acpiphp, check_hotplug_bridge() sets in some scenario that I can't
> > quite figure out.  I assume it's based on something in the namespace.
> > 
> > But it seems like the whole point of this patch is that even if a
> > bridge doesn't have "is_hotplug_bridge" set, ACPI hotplug can hot-add
> > devices below it.
> 
> Correct.
> 
> > So I'm not sure what "is_hotplug_bridge" is really telling us.  Is it
> > just a hint about how many resources we want to assign to the bridge,
> > i.e., we assign only a few when it's not set and more if it is set?
> 
> Good question. I did not invent the flag so hard to say. I've been using
> it because IIRC you prefer it.

It's not a question of whether I prefer it.  It's only useful if it
means something specific and we agree on what that is.

So what do you think it means?

In this code:

> > > @@ -291,7 +294,7 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
> > >          * expose slots to user space in those cases.
> > >          */
> > >         if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
> > > -           && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
> > > +           && !(slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge)) {

I *think* this part:

  slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge

means "this bridge supports hotplug but it is handled by pciehp, and
acpiphp should not expose the slot to user space".

If that's the case, maybe we should rework pciehp_is_native(bridge) so
instead of answering the question "does the OS have permission to
control PCIe hotplug in the hierarchy containing <bridge>?", it could
answer the specific question "does pciehp handle hotplug for
<bridge>?", e.g., something along these lines:

  bool pciehp_is_native(struct pci_dev *bridge)
  {
  #ifdef CONFIG_HOTPLUG_PCI_PCIE
    u32 slot_cap;
    struct pci_host_bridge *host;

    if (!pci_is_pcie(bridge))
      return false;

    pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
    if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
      return false;

    if (pcie_ports_native)
      return true;

    host = pci_find_host_bridge(bridge->bus);
    return host->native_hotplug;
  #else
    return false;
  #endif
  }

Then your test for whether acpiphp should expose the slot to user
space could be:

  -   && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
  +   && !pciehp_is_native(pdev)) {
 
We could also use pciehp_is_native() directly in
get_port_device_capability(), which is essentially the condition that
allows pciehp to claim the device.

> > > Here is how the mechanism is supposed to work when a Thunderbolt
> > > endpoint is connected to one of the ports. In case of a standard USB-C
> > > device only the xHCI is hot-added otherwise steps are the same.
> > > 
> > > 1. Initially there is only the PCIe root port that is controlled by
> > >    the pciehp driver
> > > 
> > >   00:1b.0 (Hotplug+) --
> > > 
> > > 2. Then we get native PCIe hotplug interrupt and once it is handled the
> > >    topology looks as following
> > > 
> > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
> > >                                   +- 02:01.0 (HotPlug+)
> > >                                   \- 02:02.0 --
> > > 
> > > 3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
> > 
> > By "not marked as hotplug capable", I guess you mean
> > PCI_EXP_SLTCAP_HPC is not set, right?
> 
> Right.
> 
> > >    they don't have anything behind them currently. Bridge 02:01.0 is
> > >    hotplug capable and used for extending the topology. At this point
> > >    the required PCIe devices are enabled and ACPI Notify() is sent to
> > >    the root port. The resulting topology is expected to look like
> > > 
> > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
> > >                                   +- 02:01.0 (HotPlug+)
> > >                                   \- 02:02.0 -- xHCI host controller
> > > 
> > > However, the current ACPI hotplug implementation scans the whole 00:1b.0
> > > hotplug slot and everything behind it regardless whether native PCIe is
> > > used or not, and it expects that the BIOS has configured bridge
> > > resources upfront. If that's not the case it assigns resources using
> > > minimal allocation (everything currently found just barely fit)
> > > preventing future extension. In addition to that, if there is another
> > > native PCIe hotplug going on we may find the new PCIe switch only
> > > partially ready (all links are not fully trained yet) confusing pciehp
> > > when it finally starts to enumerate for new devices.
> > > 
> > > To make this work better with the native PCIe hotplug driver (pciehp),
> > > we let it handle all slot management and resource allocation for hotplug
> > > bridges and restrict ACPI hotplug to non-hotplug bridges.

I *think* the point of this patch is that:

  If X is a bridge, and pciehp manages hotplug on X, i.e., X has
  PCI_EXP_SLTCAP_HPC set and the OS owns PCIe hotplug in this
  hierarchy per _OSC, acpiphp should not enumerate devices on X's
  secondary bus.

  Furthermore, acpiphp should avoid this enumeration no matter where X
  is in the hierarchy.  So if the Notify() goes to a bridge Y far
  above X, acpiphp should look at every existing bridge in the
  hierarchy below Y.  If the bridge is managed by pciehp, acpiphp
  should ignore it.  Otherwise, acpiphp should scan for new devices
  below it.

This enumeration by acpiphp is recursive, and it has to avoid the
pciehp-managed bridges at every level.  Your changes to enable_slot()
check for SLOT_IS_NATIVE at the top level, but I don't see how they
avoid pciehp-managed bridges at lower levels that may be scanned by
pci_scan_bridge().

Bjorn
Mika Westerberg May 7, 2018, 11:34 a.m. UTC | #4
On Fri, May 04, 2018 at 07:04:20PM -0500, Bjorn Helgaas wrote:
> On Thu, May 03, 2018 at 01:22:41PM +0300, Mika Westerberg wrote:
> > On Wed, May 02, 2018 at 03:49:32PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Apr 16, 2018 at 01:34:48PM +0300, Mika Westerberg wrote:
> > > > When a system is using native PCIe hotplug for Thunderbolt and the
> > > > controller is not enabled for full RTD3 (runtime D3) it will be only
> > > > present in the system when there is a device connected. This pretty much
> > > > follows the BIOS assisted hotplug behaviour.
> > > 
> > > This is a tangent, but what exactly does "controller is not enabled
> > > for full RTD3 (runtime D3)" mean?  The way that's worded sounds like
> > > it *could* be a setting in a PCI config register, but I suspect it's
> > > really something in Linux, e.g., some bit in struct pci_dev or device?
> > 
> > It means that the controller can to into D3 runtime. With BIOS assisted
> > mode and native PCIe mode, the controller is hot-removed when there is
> > nothing connected. In RTD3 mode it is there always and the OS expected
> > to put it into D3 when idle using standard PCI PM registers.
> 
> How do I tell if a controller is enabled for runtime D3?
> 
> I'm still struggling to figure out exactly how this runtime D3 part is
> relevant to this patch.  It might be behavior that happens in this
> particular scenario, but I'm not sure yet that it is a required part
> of the picture.
> 
> I think you're trying to separate enumeration handled by pciehp from
> enumeration handled by acpiphp, and runtime D3 sounds like an
> orthogonal issue.

It is not an issue at all. I just thought it is good to mention where
this all comes from in the changelog. In can reword it to:

  When a system is using native PCIe hotplug for Thunderbolt it will be
  only present in the system when there is a device connected. This pretty
  much follows the BIOS assisted hotplug behaviour.

if that is more understandable.

> > > > Thunderbolt host router integrated PCIe switch has two additional PCIe
> > > > downstream bridges that lead to NHI (Thunderbolt host controller) and xHCI
> > > > (USB 3 host controller) respectively. These downstream bridges are not
> > > > marked being hotplug capable. Reason for that is to preserve resources.
> > > > Otherwise the OS would distribute remaining resources between all
> > > > downstream bridges making these two bridges consume precious resources
> > > > of the actual hotplug bridges.
> > > > 
> > > > Now, because these two bridges are not marked being hotplug capable the OS
> > > > will not enable hotplug interrupt for them either and will not receive
> > > > interrupt when devices behind them are hot-added. Solution to this is
> > > > that the BIOS sends ACPI Notify() to the root port let the OS know it
> > > > needs to rescan for added and/or removed devices.
> > > 
> > > We're building stuff based on "is_hotplug_bridge", but I'm not sure
> > > that bit is really useful.
> > > 
> > > set_pcie_hotplug_bridge() sets it for downstream ports with
> > > PCI_EXP_SLTCAP_HPC, which is easy enough to understand.
> > > 
> > > In acpiphp, check_hotplug_bridge() sets in some scenario that I can't
> > > quite figure out.  I assume it's based on something in the namespace.
> > > 
> > > But it seems like the whole point of this patch is that even if a
> > > bridge doesn't have "is_hotplug_bridge" set, ACPI hotplug can hot-add
> > > devices below it.
> > 
> > Correct.
> > 
> > > So I'm not sure what "is_hotplug_bridge" is really telling us.  Is it
> > > just a hint about how many resources we want to assign to the bridge,
> > > i.e., we assign only a few when it's not set and more if it is set?
> > 
> > Good question. I did not invent the flag so hard to say. I've been using
> > it because IIRC you prefer it.
> 
> It's not a question of whether I prefer it.  It's only useful if it
> means something specific and we agree on what that is.
> 
> So what do you think it means?

I think it currently means a bridge that may get new devices behind it
as a result of hot-add.

In case of ACPI based hotplug, it is not always possible to determine
and set it upfront so it would make more sense to me if is_hotplug_bridge 
means that this bridge is used only as part of native PCIe hotplug.

> In this code:
> 
> > > > @@ -291,7 +294,7 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
> > > >          * expose slots to user space in those cases.
> > > >          */
> > > >         if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
> > > > -           && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
> > > > +           && !(slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge)) {
> 
> I *think* this part:
> 
>   slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge
> 
> means "this bridge supports hotplug but it is handled by pciehp, and
> acpiphp should not expose the slot to user space".

Yes, I specifically added SLOT_IS_NATIVE flag there so we can get rid of
one condition. Note that:

  pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev)

and

  slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge

are equivalent.

There are no functional changes in that hunk.

> If that's the case, maybe we should rework pciehp_is_native(bridge) so
> instead of answering the question "does the OS have permission to
> control PCIe hotplug in the hierarchy containing <bridge>?", it could
> answer the specific question "does pciehp handle hotplug for
> <bridge>?", e.g., something along these lines:
> 
>   bool pciehp_is_native(struct pci_dev *bridge)
>   {
>   #ifdef CONFIG_HOTPLUG_PCI_PCIE
>     u32 slot_cap;
>     struct pci_host_bridge *host;
> 
>     if (!pci_is_pcie(bridge))
>       return false;
> 
>     pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
>     if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
>       return false;
> 
>     if (pcie_ports_native)
>       return true;
> 
>     host = pci_find_host_bridge(bridge->bus);
>     return host->native_hotplug;
>   #else
>     return false;
>   #endif
>   }
> 
> Then your test for whether acpiphp should expose the slot to user
> space could be:

[That test was there already before this patch.]

>   -   && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
>   +   && !pciehp_is_native(pdev)) {
>  
> We could also use pciehp_is_native() directly in
> get_port_device_capability(), which is essentially the condition that
> allows pciehp to claim the device.

Fine, I agree it makes sense.

However, I intended *this* patch to be *fix* and thus did not want to
start reworking things too much. Especially code that is not related to
the issue at all (pciehp_is_native()).

> > > > Here is how the mechanism is supposed to work when a Thunderbolt
> > > > endpoint is connected to one of the ports. In case of a standard USB-C
> > > > device only the xHCI is hot-added otherwise steps are the same.
> > > > 
> > > > 1. Initially there is only the PCIe root port that is controlled by
> > > >    the pciehp driver
> > > > 
> > > >   00:1b.0 (Hotplug+) --
> > > > 
> > > > 2. Then we get native PCIe hotplug interrupt and once it is handled the
> > > >    topology looks as following
> > > > 
> > > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
> > > >                                   +- 02:01.0 (HotPlug+)
> > > >                                   \- 02:02.0 --
> > > > 
> > > > 3. Bridges 02:00.0 and 02:02.0 are not marked as hotplug capable and
> > > 
> > > By "not marked as hotplug capable", I guess you mean
> > > PCI_EXP_SLTCAP_HPC is not set, right?
> > 
> > Right.
> > 
> > > >    they don't have anything behind them currently. Bridge 02:01.0 is
> > > >    hotplug capable and used for extending the topology. At this point
> > > >    the required PCIe devices are enabled and ACPI Notify() is sent to
> > > >    the root port. The resulting topology is expected to look like
> > > > 
> > > >   00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller
> > > >                                   +- 02:01.0 (HotPlug+)
> > > >                                   \- 02:02.0 -- xHCI host controller
> > > > 
> > > > However, the current ACPI hotplug implementation scans the whole 00:1b.0
> > > > hotplug slot and everything behind it regardless whether native PCIe is
> > > > used or not, and it expects that the BIOS has configured bridge
> > > > resources upfront. If that's not the case it assigns resources using
> > > > minimal allocation (everything currently found just barely fit)
> > > > preventing future extension. In addition to that, if there is another
> > > > native PCIe hotplug going on we may find the new PCIe switch only
> > > > partially ready (all links are not fully trained yet) confusing pciehp
> > > > when it finally starts to enumerate for new devices.
> > > > 
> > > > To make this work better with the native PCIe hotplug driver (pciehp),
> > > > we let it handle all slot management and resource allocation for hotplug
> > > > bridges and restrict ACPI hotplug to non-hotplug bridges.
> 
> I *think* the point of this patch is that:
> 
>   If X is a bridge, and pciehp manages hotplug on X, i.e., X has
>   PCI_EXP_SLTCAP_HPC set and the OS owns PCIe hotplug in this
>   hierarchy per _OSC, acpiphp should not enumerate devices on X's
>   secondary bus.
> 
>   Furthermore, acpiphp should avoid this enumeration no matter where X
>   is in the hierarchy.  So if the Notify() goes to a bridge Y far
>   above X, acpiphp should look at every existing bridge in the
>   hierarchy below Y.  If the bridge is managed by pciehp, acpiphp
>   should ignore it.  Otherwise, acpiphp should scan for new devices
>   below it.
> 
> This enumeration by acpiphp is recursive, and it has to avoid the
> pciehp-managed bridges at every level.  Your changes to enable_slot()
> check for SLOT_IS_NATIVE at the top level, but I don't see how they
> avoid pciehp-managed bridges at lower levels that may be scanned by
> pci_scan_bridge().

This is pretty special case. For example below the code scans only the
two bridges (02:00.0 and 02:02.0) and they only have one bus reserved.
There won't be any further bridges downstream. The hotplug bridge
(02:01.0) is left for pciehp:

  00:1b.0 (Hotplug+) -- 01:00.0 --+- 02:00.0 --
                                  +- 02:01.0 (HotPlug+)
                                  \- 02:02.0 --

In theory of course it is possible that there will be another hotplug
bridge added behind those two bridges but in practice this is only used
for this special case to bring in the two devices (NHI, xHCI). I don't
think it makes much sense to complicate pci_scan_bridge() and friends
just to make more "generic" support for a case that in practice never
happens.
Bjorn Helgaas May 7, 2018, 8:37 p.m. UTC | #5
On Mon, May 07, 2018 at 02:34:49PM +0300, Mika Westerberg wrote:
> On Fri, May 04, 2018 at 07:04:20PM -0500, Bjorn Helgaas wrote:
> ...

> > In this code:
> > 
> > > > > @@ -291,7 +294,7 @@ static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
> > > > >          * expose slots to user space in those cases.
> > > > >          */
> > > > >         if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
> > > > > -           && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
> > > > > +           && !(slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge)) {
> > 
> > I *think* this part:
> > 
> >   slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge
> > 
> > means "this bridge supports hotplug but it is handled by pciehp, and
> > acpiphp should not expose the slot to user space".
> 
> Yes, I specifically added SLOT_IS_NATIVE flag there so we can get rid of
> one condition. Note that:
> 
>   pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev)
> 
> and
> 
>   slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge
> 
> are equivalent.
> 
> There are no functional changes in that hunk.
> 
> > If that's the case, maybe we should rework pciehp_is_native(bridge) so
> > instead of answering the question "does the OS have permission to
> > control PCIe hotplug in the hierarchy containing <bridge>?", it could
> > answer the specific question "does pciehp handle hotplug for
> > <bridge>?", e.g., something along these lines:
> > 
> >   bool pciehp_is_native(struct pci_dev *bridge)
> >   {
> >   #ifdef CONFIG_HOTPLUG_PCI_PCIE
> >     u32 slot_cap;
> >     struct pci_host_bridge *host;
> > 
> >     if (!pci_is_pcie(bridge))
> >       return false;
> > 
> >     pcie_capability_read_dword(bridge, PCI_EXP_SLTCAP, &slot_cap);
> >     if (!(slot_cap & PCI_EXP_SLTCAP_HPC))
> >       return false;
> > 
> >     if (pcie_ports_native)
> >       return true;
> > 
> >     host = pci_find_host_bridge(bridge->bus);
> >     return host->native_hotplug;
> >   #else
> >     return false;
> >   #endif
> >   }
> > 
> > Then your test for whether acpiphp should expose the slot to user
> > space could be:
> 
> [That test was there already before this patch.]
> 
> >   -   && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
> >   +   && !pciehp_is_native(pdev)) {
> >  
> > We could also use pciehp_is_native() directly in
> > get_port_device_capability(), which is essentially the condition that
> > allows pciehp to claim the device.
> 
> Fine, I agree it makes sense.
> 
> However, I intended *this* patch to be *fix* and thus did not want to
> start reworking things too much. Especially code that is not related to
> the issue at all (pciehp_is_native()).

I think this pciehp_is_native() issue is definitely related.  You're
using it to set SLOT_IS_NATIVE.  But it's more complicated than I
first thought and there are several things that look wrong here.

  - We request OSC_PCI_EXPRESS_NATIVE_HP_CONTROL unconditionally, even
    if CONFIG_HOTPLUG_PCI_PCIE isn't set, which seems wrong.  If we
    request control, we should be prepared to handle hotplug events.

  - I think we should make CONFIG_HOTPLUG_PCI_SHPC a bool instead of a
    tristate, like we did for pciehp.  acpiphp is a bool and I don't
    think it can coordinate correctly with pciehp and SHPC unless
    they're all bools.

  - We probably should split "host->native_hotplug" into
    "native_pcie_hotplug" and "native_shpc_hotplug".

  - We should request OSC_PCI_SHPC_NATIVE_HP_CONTROL similarly to how
    we request OSC_PCI_EXPRESS_NATIVE_HP_CONTROL and adapt the
    is_shpc_capable() / acpi_get_hp_hw_control_from_firmware() path to
    use "host->native_shpc_hotplug".

  - The acpiphp_add_context() path probably should treat SHPC and
    pciehp the same.

I know this probably sounds like a tangent to you, but to me it seems
like we're building on a poor foundation and we should make the
foundation solid before extending it.

Here's an example of what I mean about the foundation being poor: it
seems fairly clear that the *intent* is that pciehp_is_native() means
"pciehp manages hotplug events on this bridge", but I don't think
that's true.

Today pciehp_is_native() returns "true" for every PCI device in a
hierarchy where _OSC says we can use pciehp.  That's obviously wrong
because in such a hierarchy, bridges without PCI_EXP_SLTCAP_HPC should
be managed by acpiphp.

There's only one caller (acpiphp_add_context()), which *tries* to make
it sensible by restricting it to bridges with "is_hotplug_bridge" set.
But "is_hotplug_bridge" tells us nothing about whether pciehp manages
the bridge because acpiphp sets it in check_hotplug_bridge() (under
conditions that are very difficult to figure out).

So evidently "pdev->is_hotplug_bridge && pciehp_is_native(pdev)" is
true for some bridges that are actually managed by acpiphp, which
prevents acpiphp_add_context() from registering some slots that it
should register.

Bjorn
diff mbox

Patch

diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index e438a2d734f2..8dcfd623ef1d 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -158,6 +158,7 @@  struct acpiphp_attention_info
 
 #define SLOT_ENABLED		(0x00000001)
 #define SLOT_IS_GOING_AWAY	(0x00000002)
+#define SLOT_IS_NATIVE		(0x00000004)
 
 /* function flags */
 
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index b45b375c0e6c..5efa21cdddc9 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -282,6 +282,9 @@  static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
 	slot->device = device;
 	INIT_LIST_HEAD(&slot->funcs);
 
+	if (pdev && pciehp_is_native(pdev))
+		slot->flags |= SLOT_IS_NATIVE;
+
 	list_add_tail(&slot->node, &bridge->slots);
 
 	/*
@@ -291,7 +294,7 @@  static acpi_status acpiphp_add_context(acpi_handle handle, u32 lvl, void *data,
 	 * expose slots to user space in those cases.
 	 */
 	if ((acpi_pci_check_ejectable(pbus, handle) || is_dock_device(adev))
-	    && !(pdev && pdev->is_hotplug_bridge && pciehp_is_native(pdev))) {
+	    && !(slot->flags & SLOT_IS_NATIVE && pdev->is_hotplug_bridge)) {
 		unsigned long long sun;
 		int retval;
 
@@ -430,6 +433,29 @@  static int acpiphp_rescan_slot(struct acpiphp_slot *slot)
 	return pci_scan_slot(slot->bus, PCI_DEVFN(slot->device, 0));
 }
 
+static void acpiphp_native_scan_bridge(struct pci_dev *bridge)
+{
+	struct pci_bus *bus = bridge->subordinate;
+	struct pci_dev *dev;
+	int max;
+
+	if (!bus)
+		return;
+
+	max = bus->busn_res.start;
+	/* Scan already configured non-hotplug bridges */
+	for_each_pci_bridge(dev, bus) {
+		if (!dev->is_hotplug_bridge)
+			max = pci_scan_bridge(bus, dev, max, 0);
+	}
+
+	/* Scan non-hotplug bridges that need to be reconfigured */
+	for_each_pci_bridge(dev, bus) {
+		if (!dev->is_hotplug_bridge)
+			max = pci_scan_bridge(bus, dev, max, 1);
+	}
+}
+
 /**
  * enable_slot - enable, configure a slot
  * @slot: slot to be enabled
@@ -442,25 +468,42 @@  static void enable_slot(struct acpiphp_slot *slot)
 	struct pci_dev *dev;
 	struct pci_bus *bus = slot->bus;
 	struct acpiphp_func *func;
-	int max, pass;
-	LIST_HEAD(add_list);
 
-	acpiphp_rescan_slot(slot);
-	max = acpiphp_max_busnr(bus);
-	for (pass = 0; pass < 2; pass++) {
+	if (slot->flags & SLOT_IS_NATIVE) {
+		/*
+		 * If native PCIe hotplug is used, it will take care of
+		 * hotplug slot management and resource allocation for
+		 * hotplug bridges. However, ACPI hotplug may still be
+		 * used for non-hotplug bridges to bring in additional
+		 * devices such as Thunderbolt host controller.
+		 */
 		for_each_pci_bridge(dev, bus) {
-			if (PCI_SLOT(dev->devfn) != slot->device)
-				continue;
-
-			max = pci_scan_bridge(bus, dev, max, pass);
-			if (pass && dev->subordinate) {
-				check_hotplug_bridge(slot, dev);
-				pcibios_resource_survey_bus(dev->subordinate);
-				__pci_bus_size_bridges(dev->subordinate, &add_list);
+			if (PCI_SLOT(dev->devfn) == slot->device)
+				acpiphp_native_scan_bridge(dev);
+		}
+		pci_assign_unassigned_bridge_resources(bus->self);
+	} else {
+		LIST_HEAD(add_list);
+		int max, pass;
+
+		acpiphp_rescan_slot(slot);
+		max = acpiphp_max_busnr(bus);
+		for (pass = 0; pass < 2; pass++) {
+			for_each_pci_bridge(dev, bus) {
+				if (PCI_SLOT(dev->devfn) != slot->device)
+					continue;
+
+				max = pci_scan_bridge(bus, dev, max, pass);
+				if (pass && dev->subordinate) {
+					check_hotplug_bridge(slot, dev);
+					pcibios_resource_survey_bus(dev->subordinate);
+					__pci_bus_size_bridges(dev->subordinate,
+							       &add_list);
+				}
 			}
 		}
+		__pci_bus_assign_resources(bus, &add_list, NULL);
 	}
-	__pci_bus_assign_resources(bus, &add_list, NULL);
 
 	acpiphp_sanitize_bus(bus);
 	pcie_bus_configure_settings(bus);