PCI: Fixes for native PCIe and ACPI hotplug

When distributing extra buses between hotplug bridges we need to make sure each bridge reserve at least one bus number, even if there is currently nothing connected to it. For instance ACPI hotplug may bring in additional devices to non-hotplug bridges later on.

Here is what happens on one system when a Thunderbolt device is plugged in:

pci 0000:01:00.0: PCI bridge to [bus 02-39]
...
pci_bus 0000:04: [bus 04-39] extended by 0x35
pci_bus 0000:04: bus scan returning with max=39
pci_bus 0000:04: busn_res: [bus 04-39] end is updated to 39
pci 0000:02:02.0: scanning [bus 00-00] behind bridge, pass 1 pci_bus 0000:3a: scanning bus
pci_bus 0000:3a: bus scan returning with max=3a
pci_bus 0000:3a: busn_res: [bus 3a] end is updated to 3a pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:02 [bus 02-39] pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:01 [bus 01-39] pci_bus 0000:02: bus scan returning with max=3a
pci_bus 0000:02: busn_res: [bus 02-39] end can not be updated to 3a

Resulting 'lspci -t' output looks like this:

  +-1b.0-[01-39]----00.0-[02-3a]--+-00.0-[03]----00.0
                                  +-01.0-[04-39]--
                                  \-02.0-[3a]----00.0

The device behind downstream port at 02:02 is the integrated xHCI (USB 3 host controller) and is not fully accessible because the hotplug bridge is reserving too many bus numbers.

To make sure we don't run out of bus numbers for non-hotplug bridges reserve one bus number for them upfront before distributing buses for hotplug bridges.

Fixes 1c02ea810065 ("PCI: Distribute available buses to hotplug-capable bridges")

Reported-by Mario Limonciello <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Cc [email protected]

---
 drivers/pci/probe.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/pci/probe.c +0 -0

index ef5377438a1e..6cefd47556e3 100644

drivers/pci/probe.c +4 -2

 	}
 
 	/* Scan bridges that need to be reconfigured */

@@ -2584,12 +2587,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 			 * bridges if any.
 			 */
 			buses = available_buses / hotplug_bridges;
-			buses = min(buses, available_buses - used_buses);
+			buses = min(buses, available_buses - used_buses + 1);
 		}
 
 		cmax = max;
 		max = pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
-		used_buses += max - cmax;
+		/* One bus is already accounted so don't add it again */
+		if (max - cmax > 1)
+			used_buses += max - cmax - 1;
 	}
 
 	/*

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Mika Westerberg

When distributing extra buses between hotplug bridges we need to make sure each bridge reserve at least one bus number, even if there is currently nothing connected to it. For instance ACPI hotplug may bring in additional devices to non-hotplug bridges later on.

I guess you mean ACPI hotplug can add devices below bridges that have "bridge->is_hotplug_bridge == 0"? Why don't we set is_hotplug_bridge in that case? I do see that acpiphp sets it in *some* cases (see check_hotplug_bridge()). Are we missing some case?

Quoted 18 lines

Here is what happens on one system when a Thunderbolt device is plugged in:

pci 0000:01:00.0: PCI bridge to [bus 02-39]
...
pci_bus 0000:04: [bus 04-39] extended by 0x35
pci_bus 0000:04: bus scan returning with max=39
pci_bus 0000:04: busn_res: [bus 04-39] end is updated to 39
pci 0000:02:02.0: scanning [bus 00-00] behind bridge, pass 1 pci_bus 0000:3a: scanning bus
pci_bus 0000:3a: bus scan returning with max=3a
pci_bus 0000:3a: busn_res: [bus 3a] end is updated to 3a pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:02 [bus 02-39] pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:01 [bus 01-39] pci_bus 0000:02: bus scan returning with max=3a
pci_bus 0000:02: busn_res: [bus 02-39] end can not be updated to 3a

Resulting 'lspci -t' output looks like this:

Quoted 7 lines

  +-1b.0-[01-39]----00.0-[02-3a]--+-00.0-[03]----00.0
                                  +-01.0-[04-39]--
                                  \-02.0-[3a]----00.0

The device behind downstream port at 02:02 is the integrated xHCI (USB 3 host controller) and is not fully accessible because the hotplug bridge is reserving too many bus numbers.

Thanks for the details here, but I can't tell what happened before and was broken, vs. what happens now. Which is the hotplug bridge? Which is the Thunderbolt controller?

I guess 02:01.0 must be the bridge consuming too many bus numbers ([bus 04-39])?

And 02:02.0 might be the Thunderbolt controller that wants to use bus 3a? But obviously that won't work because 1b.0 doesn't route things to bus 3a, since it only consumes [bus 01-39].

(The device behind 02:02.0 is more than just "not fully accessible" -- it's not accessible via config space *at all*.)

I guess the 'lspci -t' above must be without this patch, and with this patch, we'd have

pci 0000:02:00.0: PCI bridge to [bus 03]
pci 0000:02:01.0: PCI bridge to [bus 04-38]
pci 0000:02:02.0: PCI bridge to [bus 39]

This patch might fix the situation for simple hot-added devices, but won't we have the same problem again if we hot-add a bridge? It seems like we need a more comprehensive solution. I don't mean we need to go whole hog and reassign everybody's bus numbers dynamically, but we ought to at least be able to notice the situation, decline to enable the bridge leading to devices we can't reach, and give a meaningful error message.

Nit unrelated to this patch: "bridge 0000:02" is not a bridge, it's a bus. Apparently bus 3a is hidden because 1b.0's subordinate bus is 39.

Quoted 12 lines

To make sure we don't run out of bus numbers for non-hotplug bridges reserve one bus number for them upfront before distributing buses for hotplug bridges.

Fixes 1c02ea810065 ("PCI: Distribute available buses to hotplug-capable bridges")

Reported-by Mario Limonciello <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Cc [email protected]

---
 drivers/pci/probe.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/pci/probe.c static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, · line 2567

Quoted 13 lines

drivers/pci/probe.c +4 -1

index ef5377438a1e..6cefd47556e3 100644
@@ -2561,7 +2561,10 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 	for_each_pci_bridge(dev, bus) {
 		cmax = max;
 		max = pci_scan_bridge_extend(bus, dev, max, 0, 0);
-		used_buses += cmax - max;
+		/* Reserve one bus for each bridge */
+		used_buses++;
+		if (cmax - max > 1)
+			used_buses += cmax - max - 1;

Sorry, this should be trivial, but I'm having a hard time wrapping my mind around it.

AFAICT, "cmax" is the highest known bus number below this bus, "max" is the highest bus number below "dev" (one of the bridges on "bus").

I assume "used_buses++" accounts for the fact that every enabled bridge must consume one bus number for its secondary side.

And I guess "used_buses += cmax - max - 1" adds in the bus numbers downstream from "dev" (subtracting the one used for its secondary bus)?

pci_scan_bridge_extend() seems to return something related to the number of bus numbers used below "dev". Why doesn't *it* account for the secondary bus number of "dev"?

It might help if the pci_scan_bridge_extend() function comment were extended to say what it actually returns.

Quoted 27 lines

drivers/pci/probe.c +4 -2

 	}
 
 	/* Scan bridges that need to be reconfigured */

@@ -2584,12 +2587,14 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 			 * bridges if any.
 			 */
 			buses = available_buses / hotplug_bridges;
-			buses = min(buses, available_buses - used_buses);
+			buses = min(buses, available_buses - used_buses + 1);
 		}
 
 		cmax = max;
 		max = pci_scan_bridge_extend(bus, dev, cmax, buses, 1);
-		used_buses += max - cmax;
+		/* One bus is already accounted so don't add it again */
+		if (max - cmax > 1)
+			used_buses += max - cmax - 1;
 	}
 
 	/*

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · Bjorn Helgaas

Quoted 4 lines · Mika Westerberg

When distributing extra buses between hotplug bridges we need to make sure each bridge reserve at least one bus number, even if there is currently nothing connected to it. For instance ACPI hotplug may bring in additional devices to non-hotplug bridges later on.

Quoted 4 lines

I guess you mean ACPI hotplug can add devices below bridges that have "bridge->is_hotplug_bridge == 0"? Why don't we set is_hotplug_bridge in that case? I do see that acpiphp sets it in *some* cases (see check_hotplug_bridge()). Are we missing some case?

We don't know upfront that these ports are going to get devices below them. Only thing that happens in these cases is that we may get ACPI Notify() to the root port leading to these ports.

Also the allocation strategy we use is based on ->is_hotplug_bridge == 1. Those bridges will be assigned all the remaining bus space and resources. If we somehow set ->is_hotplug_bridge == 1 for these non-hotplug ports it means that we now include those ports also when resources are distributed which defeats the reason why ACPI Notify() is used there in the first place (to preserve bus numbers).

Quoted 26 lines

Quoted 18 lines

Here is what happens on one system when a Thunderbolt device is plugged in:

pci 0000:01:00.0: PCI bridge to [bus 02-39]
...
pci_bus 0000:04: [bus 04-39] extended by 0x35
pci_bus 0000:04: bus scan returning with max=39
pci_bus 0000:04: busn_res: [bus 04-39] end is updated to 39
pci 0000:02:02.0: scanning [bus 00-00] behind bridge, pass 1 pci_bus 0000:3a: scanning bus
pci_bus 0000:3a: bus scan returning with max=3a
pci_bus 0000:3a: busn_res: [bus 3a] end is updated to 3a pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:02 [bus 02-39] pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:01 [bus 01-39] pci_bus 0000:02: bus scan returning with max=3a
pci_bus 0000:02: busn_res: [bus 02-39] end can not be updated to 3a

Resulting 'lspci -t' output looks like this:

Quoted 7 lines

  +-1b.0-[01-39]----00.0-[02-3a]--+-00.0-[03]----00.0
                                  +-01.0-[04-39]--
                                  \-02.0-[3a]----00.0

The device behind downstream port at 02:02 is the integrated xHCI (USB 3 host controller) and is not fully accessible because the hotplug bridge is reserving too many bus numbers.

Quoted 6 lines

Thanks for the details here, but I can't tell what happened before and was broken, vs. what happens now. Which is the hotplug bridge? Which is the Thunderbolt controller?

I guess 02:01.0 must be the bridge consuming too many bus numbers ([bus 04-39])?

Yes, that's correct.

Quoted 3 lines

And 02:02.0 might be the Thunderbolt controller that wants to use bus 3a? But obviously that won't work because 1b.0 doesn't route things to bus 3a, since it only consumes [bus 01-39].

In fact 02:02 leads to xHCI controller which in this case is inaccessible which means that currently when you plug in USB 3 device to systems with this setup, it won't work.

Quoted 2 lines

(The device behind 02:02.0 is more than just "not fully accessible" -- it's not accessible via config space *at all*.)

Right.

Quoted 3 lines

I guess the 'lspci -t' above must be without this patch, and with this patch, we'd have

Quoted 3 lines

pci 0000:02:00.0: PCI bridge to [bus 03]
pci 0000:02:01.0: PCI bridge to [bus 04-38]
pci 0000:02:02.0: PCI bridge to [bus 39]

That's correct.

Do you want me to amend the changelog to include this information as well?

Quoted 7 lines

This patch might fix the situation for simple hot-added devices, but won't we have the same problem again if we hot-add a bridge? It seems like we need a more comprehensive solution. I don't mean we need to go whole hog and reassign everybody's bus numbers dynamically, but we ought to at least be able to notice the situation, decline to enable the bridge leading to devices we can't reach, and give a meaningful error message.

The problem is that you don't know upfront what is going to be hotplugged. Thus it is hard to guess how many buses you want to reserve there. Doing that afterwards is not going to work because of the nature how we do scan and add devices, without rewriting the whole scanning logic.

The strategy we use here is the same as Windows does (e.g reserve one bus for these bridges, just in case ACPI Notify() brings in a new device. This is kind of special case used to hotplug TBT and xHCI controller (not bridges). It will not work properly if you hotplug a bridge but at least it works better than just failing miserably.

The sanity check at the end of pci_scan_bridge_extend() already detects cases where things went wrong:

   pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:02 [bus 02-39]
   pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:01 [bus 01-39]

I'm not sure whether we want to add more error prints to confuse users.

(We actually look for these strings in our test automation for native
PCIe hotplug).

Quoted 3 lines

Nit unrelated to this patch: "bridge 0000:02" is not a bridge, it's a bus. Apparently bus 3a is hidden because 1b.0's subordinate bus is 39.

Indeed.

Quoted 12 lines

Quoted 12 lines

To make sure we don't run out of bus numbers for non-hotplug bridges reserve one bus number for them upfront before distributing buses for hotplug bridges.

Fixes 1c02ea810065 ("PCI: Distribute available buses to hotplug-capable bridges")

Reported-by Mario Limonciello <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Cc [email protected]

---
 drivers/pci/probe.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Quoted 16 lines

drivers/pci/probe.c static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, · line 2567

Quoted 13 lines

drivers/pci/probe.c +4 -1

index ef5377438a1e..6cefd47556e3 100644
@@ -2561,7 +2561,10 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 	for_each_pci_bridge(dev, bus) {
 		cmax = max;
 		max = pci_scan_bridge_extend(bus, dev, max, 0, 0);
-		used_buses += cmax - max;
+		/* Reserve one bus for each bridge */
+		used_buses++;
+		if (cmax - max > 1)
+			used_buses += cmax - max - 1;

Sorry, this should be trivial, but I'm having a hard time wrapping my mind around it.

I don't blame you, it is getting quite complex. That's why I added the comments there, hopefully makeing it easier to understand.

Quoted 5 lines

AFAICT, "cmax" is the highest known bus number below this bus, "max" is the highest bus number below "dev" (one of the bridges on "bus").

I assume "used_buses++" accounts for the fact that every enabled bridge must consume one bus number for its secondary side.

Exactly.

Quoted 3 lines

And I guess "used_buses += cmax - max - 1" adds in the bus numbers downstream from "dev" (subtracting the one used for its secondary bus)?

That's also correct.

Quoted 3 lines

pci_scan_bridge_extend() seems to return something related to the number of bus numbers used below "dev". Why doesn't *it* account for the secondary bus number of "dev"?

It returns new "max" i.e maximum subordinate number the bridge occupies.

Reason why we handle this one in pci_scan_child_bus_extend() instead is that we then have the bus number distribution logic pretty much in a single function making it easier to understand what happens (well, it is getting quite complex but I still think it makes sense that way). If you insist, I can move it to pci_scan_bridge_extend() instead.

Quoted 2 lines

It might help if the pci_scan_bridge_extend() function comment were extended to say what it actually returns.

I can make a separate patch adding comment about the return value. These functions all return new "max" but I suppose it makes sense to document it.

Quoted 7 lines · Mika Westerberg

Quoted 6 lines · Bjorn Helgaas

Quoted 4 lines · Mika Westerberg

When distributing extra buses between hotplug bridges we need to make sure each bridge reserve at least one bus number, even if there is currently nothing connected to it. For instance ACPI hotplug may bring in additional devices to non-hotplug bridges later on.

Quoted 8 lines

Quoted 4 lines

I guess you mean ACPI hotplug can add devices below bridges that have "bridge->is_hotplug_bridge == 0"? Why don't we set is_hotplug_bridge in that case? I do see that acpiphp sets it in *some* cases (see check_hotplug_bridge()). Are we missing some case?

We don't know upfront that these ports are going to get devices below them. Only thing that happens in these cases is that we may get ACPI Notify() to the root port leading to these ports.

Right, it makes sense that we can't tell in advance which devices might receive ACPI Notify() events.

We set "is_hotplug_bridge" in these cases:

1. PCIe bridge with PCI_EXP_SLTCAP_HPC (hot-plug capable) bit set.

Possibly this could be conditional on CONFIG_HOTPLUG_PCI_PCIE because we can't really handle hotplug anyway if that's not set. The "manual hotplug" scenario where the user initiates a rescan with /sys/bus/pci/rescan or similar might suffer resource problems, but that's sort of a corner case that doesn't feel super important to me.

2. acpiphp in check_hotplug_bridge(). bbd34fcdd1b2 ("ACPI / hotplug / PCI: Register all devices under the given bridge") suggests that we treat "all PCI bridges represented in the ACPI namespace are now considered 'hotplug' bridges". I guess this basically a hint that hotplug is more likely if we have ACPI objects and possibly things like _RMV, _EJx, etc.

3. The PLX 6254/HINT HB6 quirk, which I think we can disregard.

Quoted 6 lines

Also the allocation strategy we use is based on ->is_hotplug_bridge == 1. Those bridges will be assigned all the remaining bus space and resources. If we somehow set ->is_hotplug_bridge == 1 for these non-hotplug ports it means that we now include those ports also when resources are distributed which defeats the reason why ACPI Notify() is used there in the first place (to preserve bus numbers).

So as a general rule, can we say that we currently distribute resources across bridges that have PCI_EXP_SLTCAP_HPC or are described in the ACPI namespace, and we don't reserve anything for other bridges?

And the point of this patch is that we want to reserve at least one bus number for *every* bridge because acpiphp may add something below it?

Presumably we should reserve some I/O and MMIO space, too? Is that already covered elsewhere? Maybe we only need to do this if acpiphp is actually compiled in?

Quoted 55 lines

Quoted 26 lines

Quoted 18 lines

Here is what happens on one system when a Thunderbolt device is plugged in:

pci 0000:01:00.0: PCI bridge to [bus 02-39]
...
pci_bus 0000:04: [bus 04-39] extended by 0x35
pci_bus 0000:04: bus scan returning with max=39
pci_bus 0000:04: busn_res: [bus 04-39] end is updated to 39
pci 0000:02:02.0: scanning [bus 00-00] behind bridge, pass 1 pci_bus 0000:3a: scanning bus
pci_bus 0000:3a: bus scan returning with max=3a
pci_bus 0000:3a: busn_res: [bus 3a] end is updated to 3a pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:02 [bus 02-39] pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:01 [bus 01-39] pci_bus 0000:02: bus scan returning with max=3a
pci_bus 0000:02: busn_res: [bus 02-39] end can not be updated to 3a

Resulting 'lspci -t' output looks like this:

Quoted 7 lines

  +-1b.0-[01-39]----00.0-[02-3a]--+-00.0-[03]----00.0
                                  +-01.0-[04-39]--
                                  \-02.0-[3a]----00.0

The device behind downstream port at 02:02 is the integrated xHCI (USB 3 host controller) and is not fully accessible because the hotplug bridge is reserving too many bus numbers.

Quoted 6 lines

Thanks for the details here, but I can't tell what happened before and was broken, vs. what happens now. Which is the hotplug bridge? Which is the Thunderbolt controller?

I guess 02:01.0 must be the bridge consuming too many bus numbers ([bus 04-39])?

Yes, that's correct.

Quoted 3 lines

And 02:02.0 might be the Thunderbolt controller that wants to use bus 3a? But obviously that won't work because 1b.0 doesn't route things to bus 3a, since it only consumes [bus 01-39].

In fact 02:02 leads to xHCI controller which in this case is inaccessible which means that currently when you plug in USB 3 device to systems with this setup, it won't work.

Quoted 2 lines

(The device behind 02:02.0 is more than just "not fully accessible" -- it's not accessible via config space *at all*.)

Right.

Quoted 6 lines

I guess the 'lspci -t' above must be without this patch, and with this patch, we'd have

pci 0000:02:00.0: PCI bridge to [bus 03]
pci 0000:02:01.0: PCI bridge to [bus 04-38]
pci 0000:02:02.0: PCI bridge to [bus 39]

Quoted 4 lines

That's correct.

Do you want me to amend the changelog to include this information as well?

Yes, please; I think it's very helpful to understand exactly what changes as a result of this patch.

Quoted 8 lines

Quoted 7 lines

This patch might fix the situation for simple hot-added devices, but won't we have the same problem again if we hot-add a bridge? It seems like we need a more comprehensive solution. I don't mean we need to go whole hog and reassign everybody's bus numbers dynamically, but we ought to at least be able to notice the situation, decline to enable the bridge leading to devices we can't reach, and give a meaningful error message.

Quoted 5 lines

The problem is that you don't know upfront what is going to be hotplugged. Thus it is hard to guess how many buses you want to reserve there. Doing that afterwards is not going to work because of the nature how we do scan and add devices, without rewriting the whole scanning logic.

That's what I meant about "not going whole hog" -- it's impractical right now to solve the general problem. But we should be clear in *this* patch that we're adding a special case to handle a common
situation: hot-adding a single endpoint.

And if we encounter the more difficult cases like hot-adding a bridge where we don't have resources (bus numbers, I/O, or MMIO) for the hierarchy, we can fail with a clear message. The "partially hidden behind bridge" message does not count as clear :)

Quoted 6 lines

The strategy we use here is the same as Windows does (e.g reserve one bus for these bridges, just in case ACPI Notify() brings in a new device. This is kind of special case used to hotplug TBT and xHCI controller (not bridges). It will not work properly if you hotplug a bridge but at least it works better than just failing miserably.

The same issue could happen on any system where we use acpiphp, so I don't think Thunderbolt is really relevant here, and it's easy to confuse things by mentioning it.

IIUC, the problem we're trying to solve here is simply this:

A hot-added endpoint is useless unless we can assign a bus number for it.

I think if this changelog mentioned that fact, what we plan to do (assign one bus number for every bridge, and potentially more for hotplug bridges), and a simple example of the previous and new bus number assignments, that would be about right.

Quoted 10 lines

The sanity check at the end of pci_scan_bridge_extend() already detects cases where things went wrong:

   pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:02 [bus 02-39]
   pci_bus 0000:3a: [bus 3a] partially hidden behind bridge 0000:01 [bus 01-39]

I'm not sure whether we want to add more error prints to confuse users.

(We actually look for these strings in our test automation for native
PCIe hotplug).

I think we can make things less confusing for users if we improve or replace that message. Maybe something along the lines of "devices behind bridge X are unusable because we can't assign a bus number for them."

We can't be very specific about what the new devices *are*, because without a bus number, we can't even read their Vendor/Device IDs.

We should make sure that we don't create pci_devs for any of these hidden devices (I suspect this is already the case).

Also, the "Has only triggered on CardBus, fixup is in yenta_socket" comment above this block looks obsolete. We should do some archaeology on this and probably remove it, because you're seeing this, and I don't think you're using CardBus or yenta_socket.

Quoted 56 lines

Quoted 3 lines

Nit unrelated to this patch: "bridge 0000:02" is not a bridge, it's a bus. Apparently bus 3a is hidden because 1b.0's subordinate bus is 39.

Indeed.

Quoted 12 lines

Quoted 12 lines

To make sure we don't run out of bus numbers for non-hotplug bridges reserve one bus number for them upfront before distributing buses for hotplug bridges.

Fixes 1c02ea810065 ("PCI: Distribute available buses to hotplug-capable bridges")

Reported-by Mario Limonciello <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Cc [email protected]

---
 drivers/pci/probe.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Quoted 16 lines

drivers/pci/probe.c static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus, · line 2567

Quoted 13 lines

drivers/pci/probe.c +4 -1

index ef5377438a1e..6cefd47556e3 100644
@@ -2561,7 +2561,10 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
 	for_each_pci_bridge(dev, bus) {
 		cmax = max;
 		max = pci_scan_bridge_extend(bus, dev, max, 0, 0);
-		used_buses += cmax - max;
+		/* Reserve one bus for each bridge */
+		used_buses++;
+		if (cmax - max > 1)
+			used_buses += cmax - max - 1;

Sorry, this should be trivial, but I'm having a hard time wrapping my mind around it.

I don't blame you, it is getting quite complex. That's why I added the comments there, hopefully makeing it easier to understand.

Quoted 5 lines

AFAICT, "cmax" is the highest known bus number below this bus, "max" is the highest bus number below "dev" (one of the bridges on "bus").

I assume "used_buses++" accounts for the fact that every enabled bridge must consume one bus number for its secondary side.

Exactly.

Quoted 3 lines

And I guess "used_buses += cmax - max - 1" adds in the bus numbers downstream from "dev" (subtracting the one used for its secondary bus)?

That's also correct.

Quoted 3 lines

pci_scan_bridge_extend() seems to return something related to the number of bus numbers used below "dev". Why doesn't *it* account for the secondary bus number of "dev"?

Quoted 7 lines

It returns new "max" i.e maximum subordinate number the bridge occupies.

Reason why we handle this one in pci_scan_child_bus_extend() instead is that we then have the bus number distribution logic pretty much in a single function making it easier to understand what happens (well, it is getting quite complex but I still think it makes sense that way). If you insist, I can move it to pci_scan_bridge_extend() instead.

I could be wrong, but I think the code would make more sense if all the bus number consumption computation were in one place. It looks like pci_scan_bridge_extend() already figures out the [secondary + 1 .. subordinate] interval, so it seems natural to me to have it figure out the entire [secondary .. subordinate] interval.

Quoted 3 lines · Bjorn Helgaas

The same issue could happen on any system where we use acpiphp, so I don't think Thunderbolt is really relevant here, and it's easy to confuse things by mentioning it.

This issue can happen regardless whether acpiphp is used or not.

Quoted 5 lines · Mika Westerberg

Quoted 3 lines · Bjorn Helgaas

The same issue could happen on any system where we use acpiphp, so I don't think Thunderbolt is really relevant here, and it's easy to confuse things by mentioning it.

Quoted 1 line

This issue can happen regardless whether acpiphp is used or not.

If the platform has yielded hotplug control to the OS via _OSC, I don't see how the platform could hot-add devices. So surely reserving a bus number for a bridge without anything below it can be constrained to the !pciehp_is_native(bridge) case?

Quoted 6 lines · Lukas Wunner

Quoted 5 lines · Mika Westerberg

Quoted 3 lines · Bjorn Helgaas

The same issue could happen on any system where we use acpiphp, so I don't think Thunderbolt is really relevant here, and it's easy to confuse things by mentioning it.

Quoted 6 lines

Quoted 1 line

This issue can happen regardless whether acpiphp is used or not.

If the platform has yielded hotplug control to the OS via _OSC, I don't see how the platform could hot-add devices. So surely reserving a bus number for a bridge without anything below it can be constrained to the !pciehp_is_native(bridge) case?

Nothing prevents ACPI Notify() happening while native PCIe hotplug is used on non-hotplug ports (the ones not controlled by pciehp). And it cannot be constrained to !pciehp_is_native(bridge) because it is the root port that has the _OSC but below it can be non-hotplug ports where ACPI Notify() is used to bring in additional devices.

Quoted 14 lines · Mika Westerberg

Quoted 6 lines · Lukas Wunner

Quoted 5 lines · Mika Westerberg

Quoted 3 lines · Bjorn Helgaas

The same issue could happen on any system where we use acpiphp, so I don't think Thunderbolt is really relevant here, and it's easy to confuse things by mentioning it.

Quoted 6 lines

Quoted 1 line

This issue can happen regardless whether acpiphp is used or not.

If the platform has yielded hotplug control to the OS via _OSC, I don't see how the platform could hot-add devices. So surely reserving a bus number for a bridge without anything below it can be constrained to the !pciehp_is_native(bridge) case?

Quoted 5 lines

Nothing prevents ACPI Notify() happening while native PCIe hotplug is used on non-hotplug ports (the ones not controlled by pciehp). And it cannot be constrained to !pciehp_is_native(bridge) because it is the root port that has the _OSC but below it can be non-hotplug ports where ACPI Notify() is used to bring in additional devices.

That sounds like a violation of the spec to me.

ACPI 6.1 table 6-178 says if OS is granted control over PCIe hotplug, the firmware "must ensure that all hot plug events are routed to device interrupts", which wouldn't be the case for Notify() because the interrupt generated is an SCI, not an MSI or INTx interrupt for the hotplug port itself.

Moreover, "after control is transferred to the OS, firmware must not update the state of hot plug slots, including the state of the indicators and power controller."

Maybe I've misunderstood the spec all the time, my understanding was that if OS is granted control, the firmware won't do anything with hotplug ports below the host bridge, period.

Quoted 14 lines · Mika Westerberg

Quoted 6 lines · Lukas Wunner

Quoted 5 lines · Mika Westerberg

Quoted 3 lines · Bjorn Helgaas

The same issue could happen on any system where we use acpiphp, so I don't think Thunderbolt is really relevant here, and it's easy to confuse things by mentioning it.

Quoted 6 lines

Quoted 1 line

This issue can happen regardless whether acpiphp is used or not.

If the platform has yielded hotplug control to the OS via _OSC, I don't see how the platform could hot-add devices. So surely reserving a bus number for a bridge without anything below it can be constrained to the !pciehp_is_native(bridge) case?

Quoted 5 lines

Nothing prevents ACPI Notify() happening while native PCIe hotplug is used on non-hotplug ports (the ones not controlled by pciehp). And it cannot be constrained to !pciehp_is_native(bridge) because it is the root port that has the _OSC but below it can be non-hotplug ports where ACPI Notify() is used to bring in additional devices.

That sounds like a violation of the spec to me.

ACPI 6.1 table 6-178 says if OS is granted control over PCIe hotplug, the firmware "must ensure that all hot plug events are routed to device interrupts", which wouldn't be the case for Notify() because the interrupt generated is an SCI, not an MSI or INTx interrupt for the hotplug port itself.

Moreover, "after control is transferred to the OS, firmware must not update the state of hot plug slots, including the state of the indicators and power controller."

Maybe I've misunderstood the spec all the time, my understanding was that if OS is granted control, the firmware won't do anything with hotplug ports below the host bridge, period.

Quoted 33 lines · Lukas Wunner

Quoted 14 lines · Mika Westerberg

Quoted 6 lines · Lukas Wunner

Quoted 5 lines · Mika Westerberg

Quoted 3 lines · Bjorn Helgaas

The same issue could happen on any system where we use acpiphp, so I don't think Thunderbolt is really relevant here, and it's easy to confuse things by mentioning it.

Quoted 6 lines

Quoted 1 line

This issue can happen regardless whether acpiphp is used or not.

If the platform has yielded hotplug control to the OS via _OSC, I don't see how the platform could hot-add devices. So surely reserving a bus number for a bridge without anything below it can be constrained to the !pciehp_is_native(bridge) case?

Quoted 5 lines

Nothing prevents ACPI Notify() happening while native PCIe hotplug is used on non-hotplug ports (the ones not controlled by pciehp). And it cannot be constrained to !pciehp_is_native(bridge) because it is the root port that has the _OSC but below it can be non-hotplug ports where ACPI Notify() is used to bring in additional devices.

That sounds like a violation of the spec to me.

ACPI 6.1 table 6-178 says if OS is granted control over PCIe hotplug, the firmware "must ensure that all hot plug events are routed to device interrupts", which wouldn't be the case for Notify() because the interrupt generated is an SCI, not an MSI or INTx interrupt for the hotplug port itself.

Moreover, "after control is transferred to the OS, firmware must not update the state of hot plug slots, including the state of the indicators and power controller."

Quoted 3 lines

Maybe I've misunderstood the spec all the time, my understanding was that if OS is granted control, the firmware won't do anything with hotplug ports below the host bridge, period.

The whole point here is that those are *not* hotplug slots just regular downstream ports.

Quoted 2 lines

The whole point here is that those are *not* hotplug slots just regular downstream ports.

Okay, understood. Is this about the NHI or XHCI? Because at least on Alpine Ridge (C step), the bridge above the XHCI *is* a hotplug bridge. Only the bridge above the NHI is not.

Quoted 6 lines · Lukas Wunner

Quoted 2 lines

The whole point here is that those are *not* hotplug slots just regular downstream ports.

Okay, understood. Is this about the NHI or XHCI? Because at least on Alpine Ridge (C step), the bridge above the XHCI *is* a hotplug bridge. Only the bridge above the NHI is not.

Yes, exactly. I tried to clarify this mechanism a bit better in the other email I just sent.

Quoted 8 lines · Mika Westerberg

Quoted 6 lines · Lukas Wunner

Quoted 2 lines

The whole point here is that those are *not* hotplug slots just regular downstream ports.

Okay, understood. Is this about the NHI or XHCI? Because at least on Alpine Ridge (C step), the bridge above the XHCI *is* a hotplug bridge. Only the bridge above the NHI is not.

Quoted 2 lines

Yes, exactly. I tried to clarify this mechanism a bit better in the other email I just sent.

But in the e-mail you just sent, the bridge above the XHCI is not a hotplug bridge and according to the lspci output of a MacBookPro13,3 I have here, it *is* a hotplug bridge on Alpine Ridge (C step).

Quoted 33 lines · Lukas Wunner

Quoted 14 lines · Mika Westerberg

Quoted 6 lines · Lukas Wunner

Quoted 5 lines · Mika Westerberg

Quoted 3 lines · Bjorn Helgaas

The same issue could happen on any system where we use acpiphp, so I don't think Thunderbolt is really relevant here, and it's easy to confuse things by mentioning it.

Quoted 6 lines

Quoted 1 line

This issue can happen regardless whether acpiphp is used or not.

If the platform has yielded hotplug control to the OS via _OSC, I don't see how the platform could hot-add devices. So surely reserving a bus number for a bridge without anything below it can be constrained to the !pciehp_is_native(bridge) case?

Quoted 5 lines

Nothing prevents ACPI Notify() happening while native PCIe hotplug is used on non-hotplug ports (the ones not controlled by pciehp). And it cannot be constrained to !pciehp_is_native(bridge) because it is the root port that has the _OSC but below it can be non-hotplug ports where ACPI Notify() is used to bring in additional devices.

That sounds like a violation of the spec to me.

ACPI 6.1 table 6-178 says if OS is granted control over PCIe hotplug, the firmware "must ensure that all hot plug events are routed to device interrupts", which wouldn't be the case for Notify() because the interrupt generated is an SCI, not an MSI or INTx interrupt for the hotplug port itself.

Moreover, "after control is transferred to the OS, firmware must not update the state of hot plug slots, including the state of the indicators and power controller."

Quoted 3 lines

Maybe I've misunderstood the spec all the time, my understanding was that if OS is granted control, the firmware won't do anything with hotplug ports below the host bridge, period.

The whole point here is that those are *not* hotplug slots just regular downstream ports.

Quoted 2 lines

The whole point here is that those are *not* hotplug slots just regular downstream ports.

I'm not sure what scenario exactly you are referring to to be honest.

Something related to Thunderbolt I suppose?

Quoted 6 lines · Rafael J. Wysocki

Quoted 2 lines

The whole point here is that those are *not* hotplug slots just regular downstream ports.

I'm not sure what scenario exactly you are referring to to be honest.

Something related to Thunderbolt I suppose?

Here is an example that hopefully clarifies. This example is from a system using Thunderbolt in "native" mode but I think it is not specific to Thunderbolt. The idea is that when you don't have anything connected to Thunderbolt ports you have following PCI topology:

00:1b.0 --

So a root port that is hotplug capable and handled by pciehp.

Next when you plug in a Thunderbolt enpdoint, you get native PCIe hotplug event and after it is handled the topology looks like:

00:1b.0 --- 01:00.0 --+- 02:00.0 --

                        +- 02:01.0 (hotplug) --
                        \- 02:02.0 --

In other words there is a PCIe switch with one hotplug port (02:01.0) that is again handled by pciehp (this is used to daisy chain further devices). However, downstream ports 02:00.0 and 02:02.0 are not marked as hotplug capable so pciehp is not controlling them.

To bring in xHCI and/or Thunderbolt host controller we get ACPI Notify() to the root port 00:1b.0 which should result following topology after handled by acpiphp:

00:1b.0 --- 01:00.0 --+- 02:00.0 -- Thunderbolt host controller

                        +- 02:01.0 (hotplug) --
                        \- 02:02.0 -- xHCI host controller

In other words ACPI Notify() is used to populate devices connected to non-hotplug downstream ports. It is also used to "hot-unplug" them in the same way (for example if you only connect standard USB-C device to the port the Thunderbolt host controller is hot-unplugged using this mechanism).

Rest of the devices in the chain are hotplugged using standard native PCIe hotplug so pciehp will be controlling then.

I also think that it would be good to put the above example somewhere like a git commit changelog or even a comment in the code, so people in the future don't have to wonder what this is all about.

When connecting a Thunderbolt endpoint which itself has internal PCIe switch (not the integrated one) the way we currently distribute

From a PCI point of view, I assume you're adding a switch with 4 NIC

resources does not always work well because devices below non-hotplug bridges might need to have their MMIO resources aligned to something else than 1 MB boundary. As an example connecting a server grade 4-port GbE add in card adds another PCIe switch leading to GbE devices that want to have their MMIO resources aligned to 2 MB boundary instead.

Current resource distribution code does not take this alignment into account and might try to add too much resources for the extension hotplug bridges. The resulting bridge window is then too big which makes Linux to re-allocate minimal amount of resources, making future extension impossible.

Make this work better by substracting properly aligned non-hotplug downstream bridge window size from the remaining resources.

Fixes 1a5767725cec ("PCI: Distribute available resources to hotplug-capable bridges")

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Cc [email protected]

---
 drivers/pci/setup-bus.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

drivers/pci/setup-bus.c +30 -1

index 3cce29a069e6..f1e6172734f0 100644
@@ -1882,6 +1882,7 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 	resource_size_t available_mmio, resource_size_t available_mmio_pref)
 {
 	resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+	resource_size_t io_start, mmio_start, mmio_pref_start;
 	unsigned int normal_bridges = 0, hotplug_bridges = 0;
 	struct resource *io_res, *mmio_res, *mmio_pref_res;
 	struct pci_dev *dev, *bridge = bus->self;

@@ -1946,11 +1947,16 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			remaining_mmio_pref -= resource_size(res);
 	}
 
+	io_start = io_res->start;
+	mmio_start = mmio_res->start;
+	mmio_pref_start = mmio_pref_res->start;
+
 	/*
 	 * Go over devices on this bus and distribute the remaining
 	 * resource space between hotplug bridges.
 	 */
 	for_each_pci_bridge(dev, bus) {
+		resource_size_t align;
 		struct pci_bus *b;
 
 		b = dev->subordinate;

@@ -1968,7 +1974,7 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 				available_io, available_mmio,
 				available_mmio_pref);
 		} else if (dev->is_hotplug_bridge) {
-			resource_size_t align, io, mmio, mmio_pref;
+			resource_size_t io, mmio, mmio_pref;
 
 			/*
 			 * Distribute available extra resources equally

@@ -1981,11 +1987,13 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			io = div64_ul(available_io, hotplug_bridges);
 			io = min(ALIGN(io, align), remaining_io);
 			remaining_io -= io;
+			io_start += io;
 
 			align = pci_resource_alignment(bridge, mmio_res);
 			mmio = div64_ul(available_mmio, hotplug_bridges);
 			mmio = min(ALIGN(mmio, align), remaining_mmio);
 			remaining_mmio -= mmio;
+			mmio_start += mmio;
 
 			align = pci_resource_alignment(bridge, mmio_pref_res);
 			mmio_pref = div64_ul(available_mmio_pref,

@@ -1993,9 +2001,40 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			mmio_pref = min(ALIGN(mmio_pref, align),
 					remaining_mmio_pref);
 			remaining_mmio_pref -= mmio_pref;
+			mmio_pref_start += mmio_pref;
 
 			pci_bus_distribute_available_resources(b, add_list, io,
 							       mmio, mmio_pref);
+		} else {
+			/*
+			 * For normal bridges, track start of the parent
+			 * bridge window to make sure we align the
+			 * remaining space which is distributed to the
+			 * hotplug bridges properly.
+			 */
+			resource_size_t aligned;
+			struct resource *res;
+
+			res = &dev->resource[PCI_BRIDGE_RESOURCES + 0];
+			io_start += resource_size(res);
+			aligned = ALIGN(io_start,
+					pci_resource_alignment(dev, res));
+			if (aligned > io_start)
+				remaining_io -= aligned - io_start;
+
+			res = &dev->resource[PCI_BRIDGE_RESOURCES + 1];
+			mmio_start += resource_size(res);
+			aligned = ALIGN(mmio_start,

drivers/pci/setup-bus.c +0 -0

 		}
 	}
 }

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 2 lines · Mika Westerberg

When connecting a Thunderbolt endpoint which itself has internal PCIe switch (not the integrated one) the way we currently distribute

I don't understand this distinction between "internal PCIe switch" and "integrated one".

Quoted 1 line

From a PCI point of view, I assume you're adding a switch with 4 NIC

endpoints below it.

Quoted 5 lines

resources does not always work well because devices below non-hotplug bridges might need to have their MMIO resources aligned to something else than 1 MB boundary. As an example connecting a server grade 4-port GbE add in card adds another PCIe switch leading to GbE devices that want to have their MMIO resources aligned to 2 MB boundary instead.

Is there something unique about Thunderbolt here? I assume from a PCI point of view, these NIC ports have BARs that are 2 MB or larger and hence need 2 MB or larger alignment?

Quoted 5 lines

Current resource distribution code does not take this alignment into account and might try to add too much resources for the extension hotplug bridges. The resulting bridge window is then too big which makes Linux to re-allocate minimal amount of resources, making future extension impossible.

Maybe a concrete example would make this clearer to me.

Quoted 91 lines

Make this work better by substracting properly aligned non-hotplug downstream bridge window size from the remaining resources.

Fixes 1a5767725cec ("PCI: Distribute available resources to hotplug-capable bridges")

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

Cc [email protected]

---
 drivers/pci/setup-bus.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

drivers/pci/setup-bus.c +30 -1

index 3cce29a069e6..f1e6172734f0 100644
@@ -1882,6 +1882,7 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 	resource_size_t available_mmio, resource_size_t available_mmio_pref)
 {
 	resource_size_t remaining_io, remaining_mmio, remaining_mmio_pref;
+	resource_size_t io_start, mmio_start, mmio_pref_start;
 	unsigned int normal_bridges = 0, hotplug_bridges = 0;
 	struct resource *io_res, *mmio_res, *mmio_pref_res;
 	struct pci_dev *dev, *bridge = bus->self;

@@ -1946,11 +1947,16 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			remaining_mmio_pref -= resource_size(res);
 	}
 
+	io_start = io_res->start;
+	mmio_start = mmio_res->start;
+	mmio_pref_start = mmio_pref_res->start;
+
 	/*
 	 * Go over devices on this bus and distribute the remaining
 	 * resource space between hotplug bridges.
 	 */
 	for_each_pci_bridge(dev, bus) {
+		resource_size_t align;
 		struct pci_bus *b;
 
 		b = dev->subordinate;

@@ -1968,7 +1974,7 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 				available_io, available_mmio,
 				available_mmio_pref);
 		} else if (dev->is_hotplug_bridge) {
-			resource_size_t align, io, mmio, mmio_pref;
+			resource_size_t io, mmio, mmio_pref;
 
 			/*
 			 * Distribute available extra resources equally

@@ -1981,11 +1987,13 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			io = div64_ul(available_io, hotplug_bridges);
 			io = min(ALIGN(io, align), remaining_io);
 			remaining_io -= io;
+			io_start += io;
 
 			align = pci_resource_alignment(bridge, mmio_res);
 			mmio = div64_ul(available_mmio, hotplug_bridges);
 			mmio = min(ALIGN(mmio, align), remaining_mmio);
 			remaining_mmio -= mmio;
+			mmio_start += mmio;
 
 			align = pci_resource_alignment(bridge, mmio_pref_res);
 			mmio_pref = div64_ul(available_mmio_pref,

@@ -1993,9 +2001,40 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			mmio_pref = min(ALIGN(mmio_pref, align),
 					remaining_mmio_pref);
 			remaining_mmio_pref -= mmio_pref;
+			mmio_pref_start += mmio_pref;
 
 			pci_bus_distribute_available_resources(b, add_list, io,
 							       mmio, mmio_pref);
+		} else {
+			/*
+			 * For normal bridges, track start of the parent
+			 * bridge window to make sure we align the
+			 * remaining space which is distributed to the
+			 * hotplug bridges properly.
+			 */
+			resource_size_t aligned;
+			struct resource *res;
+
+			res = &dev->resource[PCI_BRIDGE_RESOURCES + 0];
+			io_start += resource_size(res);
+			aligned = ALIGN(io_start,
+					pci_resource_alignment(dev, res));
+			if (aligned > io_start)
+				remaining_io -= aligned - io_start;
+
+			res = &dev->resource[PCI_BRIDGE_RESOURCES + 1];
+			mmio_start += resource_size(res);
+			aligned = ALIGN(mmio_start,

drivers/pci/setup-bus.c static void pci_bus_distribute_available_resources(struct pci_bus *bus, · line 2037

Quoted 10 lines

drivers/pci/setup-bus.c +10 -0

+					pci_resource_alignment(dev, res));
+			if (aligned > mmio_start)
+				remaining_mmio -= aligned - mmio_start;
+
+			res = &dev->resource[PCI_BRIDGE_RESOURCES + 2];
+			mmio_pref_start += resource_size(res);
+			aligned = ALIGN(mmio_pref_start,
+					pci_resource_alignment(dev, res));
+			if (aligned > mmio_pref_start)
+				remaining_mmio_pref -= aligned - mmio_pref_start;

I don't understand how this can work. You have this:

  for_each_pci_bridge(dev, bus) {
    if (!hotplug_bridges && normal_bridges == 1) {
      pci_bus_distribute_available_resources(...);   # block 1
    } else if (dev->is_hotplug_bridge) {
      ...                                            # block 2
      pci_bus_distribute_available_resources(...);
    } else {
      ...                                            # block 3
    }
  }

This patch adds block 3. Block 3 doesn't do anything with side effects, i.e., it only updates local variables like io_start, remaining_io, mmio_start, remaining_mmio, etc.

Those may be used in subsequent iterations, but it's only useful if there *are* subsequent iterations, so this is dependent on the order we iterate through the bridges. It seems like we want the same results no matter what the order.

Quoted 6 lines · Bjorn Helgaas

Quoted 2 lines · Mika Westerberg

When connecting a Thunderbolt endpoint which itself has internal PCIe switch (not the integrated one) the way we currently distribute

I don't understand this distinction between "internal PCIe switch" and "integrated one".

I tried to explain here that Thunderbolt endpoint itself has an internal (integrated) PCIe switch.

Quoted 2 lines

Quoted 1 line

From a PCI point of view, I assume you're adding a switch with 4 NIC

endpoints below it.

Yes, exactly.

The PCIe device in question is Intel Gigabit ET2 Quad Port Server adapter.

Quoted 7 lines

Quoted 6 lines

resources does not always work well because devices below non-hotplug bridges might need to have their MMIO resources aligned to something else than 1 MB boundary. As an example connecting a server grade 4-port GbE add in card adds another PCIe switch leading to GbE devices that want to have their MMIO resources aligned to 2 MB boundary instead.

Quoted 3 lines

Is there something unique about Thunderbolt here? I assume from a PCI point of view, these NIC ports have BARs that are 2 MB or larger and hence need 2 MB or larger alignment?

No, this is not unique to Thunderbolt in any way.

Quoted 13 lines

I don't understand how this can work. You have this:

  for_each_pci_bridge(dev, bus) {
    if (!hotplug_bridges && normal_bridges == 1) {
      pci_bus_distribute_available_resources(...);   # block 1
    } else if (dev->is_hotplug_bridge) {
      ...                                            # block 2
      pci_bus_distribute_available_resources(...);
    } else {
      ...                                            # block 3
    }
  }

Quoted 3 lines

This patch adds block 3. Block 3 doesn't do anything with side effects, i.e., it only updates local variables like io_start, remaining_io, mmio_start, remaining_mmio, etc.

It updates remaining_* so that the next bridge resource is aligned properly (if there is going to be one).

Quoted 4 lines

Those may be used in subsequent iterations, but it's only useful if there *are* subsequent iterations, so this is dependent on the order we iterate through the bridges. It seems like we want the same results no matter what the order.

Here again we walk over the bridges using recursive calls so we need to know upfront the alignment before we do next call to pci_bus_distribute_available_resources() and that information is only available when previous bridge is already handled. I suppose this can be done in a better way but unfortunately I have no idea how :-( Can you suggest something concrete I could do instead if this is not sufficient?

Quoted 8 lines · Mika Westerberg

Quoted 6 lines · Bjorn Helgaas

Quoted 2 lines · Mika Westerberg

When connecting a Thunderbolt endpoint which itself has internal PCIe switch (not the integrated one) the way we currently distribute

I don't understand this distinction between "internal PCIe switch" and "integrated one".

Quoted 2 lines

I tried to explain here that Thunderbolt endpoint itself has an internal (integrated) PCIe switch.

I was confused by this because I thought you meant "Endpoint" in the sense used in the PCIe spec, i.e., something with a type 0 config header. But you're using "endpoint" to mean "some collection of devices with a Thunderbolt interface on one side".

Quoted 15 lines

Quoted 2 lines

Quoted 1 line

From a PCI point of view, I assume you're adding a switch with 4 NIC

endpoints below it.

Yes, exactly.

The PCIe device in question is Intel Gigabit ET2 Quad Port Server adapter.

Quoted 7 lines

Quoted 6 lines

resources does not always work well because devices below non-hotplug bridges might need to have their MMIO resources aligned to something else than 1 MB boundary. As an example connecting a server grade 4-port GbE add in card adds another PCIe switch leading to GbE devices that want to have their MMIO resources aligned to 2 MB boundary instead.

Quoted 5 lines

Quoted 3 lines

Is there something unique about Thunderbolt here? I assume from a PCI point of view, these NIC ports have BARs that are 2 MB or larger and hence need 2 MB or larger alignment?

No, this is not unique to Thunderbolt in any way.

I think this is a case of confusing things by mentioning irrelevant details, e.g., leading off with "When connecting a Thunderbolt endpoint" when the problem can occur when hot-adding any PCIe device (or maybe only when adding a bridge? I don't know).

It's good to mention a specific example, but I think it's best if you can describe the situation as generically as possible first, then cite the case at hand in a way that it's obvious this is just one of many possibilities.

Quoted 17 lines

Quoted 16 lines

I don't understand how this can work. You have this:

  for_each_pci_bridge(dev, bus) {
    if (!hotplug_bridges && normal_bridges == 1) {
      pci_bus_distribute_available_resources(...);   # block 1
    } else if (dev->is_hotplug_bridge) {
      ...                                            # block 2
      pci_bus_distribute_available_resources(...);
    } else {
      ...                                            # block 3
    }
  }

This patch adds block 3. Block 3 doesn't do anything with side effects, i.e., it only updates local variables like io_start, remaining_io, mmio_start, remaining_mmio, etc.

Quoted 2 lines

It updates remaining_* so that the next bridge resource is aligned properly (if there is going to be one).

What do you mean by "aligned properly"? The spec requires that bridge memory windows be 1MB aligned and I/O windows 4KB aligned. Does the current code do that wrong?

Quoted 5 lines

Quoted 4 lines

Those may be used in subsequent iterations, but it's only useful if there *are* subsequent iterations, so this is dependent on the order we iterate through the bridges. It seems like we want the same results no matter what the order.

Quoted 7 lines

Here again we walk over the bridges using recursive calls so we need to know upfront the alignment before we do next call to pci_bus_distribute_available_resources() and that information is only available when previous bridge is already handled. I suppose this can be done in a better way but unfortunately I have no idea how :-( Can you suggest something concrete I could do instead if this is not sufficient?

I don't understand this well enough to have any great suggestions.

I did notice that the "!hotplug_bridges && normal_bridges == 1" case doesn't look like it needs to be in the for_each_pci_bridge() loop because there's only one bridge (I think "hotplug_bridges == 0 && normal_bridges == 1" would be slightly easier to read).

And I suspect you might want to handle the "hotplug_bridges == 1 && normal_bridges == 0" case identically. That would mean you could pull these "list_is_singular()" cases out of the loop, which makes sense because if there's only one bridge, it should get everything.

Following PCIehp mark the unplugged PCI devices disconnected. This makes sure PCI core code leaves the now missing hardware registers alone.

Signed-off-by Mika Westerberg <[email protected]>

Reviewed-by Rafael J. Wysocki <[email protected]>

---
 drivers/pci/hotplug/acpiphp_glue.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/pci/hotplug/acpiphp_glue.c +0 -0

index 5efa21cdddc9..0aef35ee665a 100644

drivers/pci/hotplug/acpiphp_glue.c +1 -0

+
 		pci_stop_and_remove_bus_device(dev);
 		if (adev)
 			acpi_bus_trim(adev);

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I hate this pci_dev_set_disconnected() thing and hope to remove it someday. It's racy and adds additional states that are hard to manage, e.g., "device has been removed and will not respond to PCI accesses, but pci_dev_set_disconnected() hasn't been called yet". If we can handle that case (and we should), we shouldn't need pci_dev_set_disconnected().

Quoted 6 lines · Bjorn Helgaas

I hate this pci_dev_set_disconnected() thing and hope to remove it someday. It's racy and adds additional states that are hard to manage, e.g., "device has been removed and will not respond to PCI accesses, but pci_dev_set_disconnected() hasn't been called yet". If we can handle that case (and we should), we shouldn't need pci_dev_set_disconnected().

Absent pci_dev_is_disconnected(), how can the PCI core detect if a device is still there (versus has been hot-removed)? The only valid method I know of is reading the vendor ID from config space. However reading config spaces takes time. To properly handle surprise removal, we ought to check presence of the device frequently lest we clutter the logs with errors, unnecessarily occupy the CPU and bus or in some cases hang the machine. Ideally, we should check presence of the device every time we access it.

Now, imagine every time we access config or mmio space, we read the vendor ID from config space, so we essentially double the number of accesses to the device. Does that make sense, performance-wise?

If pci_dev_is_disconnected() returns true, the device is either there or was until very recently, so in general checking that instead of reading the vendor ID should be sufficient and should result in acceptable performance.

I keep hearing these complaints about the PCI_DEV_DISCONNECTED flag, from Greg but now also from you, but I'm not seeing constructive criticism how checking presence of a device should be handled instead, in a way that doesn't negatively impact performance.

IMO it was a mistake to constrain visibility of the flag to the PCI core (at Greg's behest), it should have been made available to drivers so they're afforded a better method to detect surprise removal than just reading the vendor ID every time they access the device.

Quoted 9 lines

If pci_dev_is_disconnected() returns true, the device is either there or was until very recently, so in general checking that instead of reading the vendor ID should be sufficient and should result in acceptable performance.

I keep hearing these complaints about the PCI_DEV_DISCONNECTED flag, from Greg but now also from you, but I'm not seeing constructive criticism how checking presence of a device should be handled instead, in a way that doesn't negatively impact performance.

Quoted 4 lines

IMO it was a mistake to constrain visibility of the flag to the PCI core (at Greg's behest), it should have been made available to drivers so they're afforded a better method to detect surprise removal than just reading the vendor ID every time they access the device.

To avoid the race, drivers have to check for a valid value anyway. If they *also* check pci_dev_is_disconnected(), that clutters the code and gives a false sense of security.

When a read fails, it normally returns ~0 as the data to the driver. The driver knows the semantics of the registers it reads, and often it can tell immediately that a successful read of the register could never return that value, so it doesn't need to try any more accesses to Vendor ID or anything else.

If ~0 *is* a possible valid value, the driver can read some other register (maybe Vendor ID, or maybe an MMIO register that can be read faster).

Hi,

When Thunderbolt controller is configured to be in native enumeration mode
it actually includes two slightly different modes. First there is non- RTD3
mode where the Thunderbolt host controller is only present when there is a
device connected. The second one is RTD3 mode where the controller is always present.

In non-RTD3 mode the Thunderbolt host controller (NHI) and USB host (xHCI)
controller are not hotplugged using native PCIe hotplug but instead they
will be hotplugged via BIOS triggered ACPI Notify() to the root port. This
is done to preserve resources since the NHI and xHCI only need 1 MB of MMIO
space and no additional buses. Currently Linux does not support this very
well and ends up failing the hotplug in one way or another. More detailed
explanation is in changelog of patch [4/5].

This series fixes this issue and in addition includes fixes for few other
issues found during testing on a system that has Thunderbolt controller in
non-RTD3 native PCIe enumeration mode. However, the fixes here are not in
any way Thunderbolt specific and should be applicable to other systems as
well.

The previous versions of the patch series can be found here:

v2: https://www.spinics.net/lists/linux-pci/msg69186.html
v1: https://www.spinics.net/lists/linux-acpi/msg80607.html

Changes from v2:

- Added Rafael's tag to patch [1/5].
- Updated changelog of patch [1/5] to include more details about how
the
problem can be observed from dmesg and lspci output.

Changes from v1:

- Drop 'cmax - max ?: 1' and use similar construct than we use in
second
pass loop in patch [1/5].
- Drop unnecessary parentheses in patch [1/5].
- Added Rafael's tag to patches [2-5/5].

Mika Westerberg (5):
PCI: Make sure all bridges reserve at least one bus number
PCI: Take bridge window alignment into account when distributing
resources
PCI: pciehp: Clear Presence Detect and Data Link Layer Status
Changed on resume
ACPI / hotplug / PCI: Do not scan all bridges when native PCIe
hotplug is used
ACPI / hotplug / PCI: Mark stale PCI devices disconnected

 drivers/pci/hotplug/acpiphp.h      |  1 +
 drivers/pci/hotplug/acpiphp_glue.c | 78
++++++++++++++++++++++++++++++--------
 drivers/pci/hotplug/pciehp.h       |  2 +-
 drivers/pci/hotplug/pciehp_core.c  |  2 +-
 drivers/pci/hotplug/pciehp_hpc.c   | 13 ++++++-
 drivers/pci/probe.c                | 11 ++++--
 drivers/pci/setup-bus.c            | 41 +++++++++++++++++++-
 7 files changed, 126 insertions(+), 22 deletions(-)

Hi,

When Thunderbolt controller is configured to be in native enumeration mode it actually includes two slightly different modes. First there is non-RTD3 mode where the Thunderbolt host controller is only present when there is a device connected. The second one is RTD3 mode where the controller is always present.

In non-RTD3 mode the Thunderbolt host controller (NHI) and USB host (xHCI) controller are not hotplugged using native PCIe hotplug but instead they will be hotplugged via BIOS triggered ACPI Notify() to the root port. This is done to preserve resources since the NHI and xHCI only need 1 MB of MMIO space and no additional buses. Currently Linux does not support this very well and ends up failing the hotplug in one way or another. More detailed explanation is in changelog of patch [4/5].

This series fixes this issue and in addition includes fixes for few other issues found during testing on a system that has Thunderbolt controller in non-RTD3 native PCIe enumeration mode. However, the fixes here are not in any way Thunderbolt specific and should be applicable to other systems as well.

The previous versions of the patch series can be found here:

v2: https://www.spinics.net/lists/linux-pci/msg69186.html
v1: https://www.spinics.net/lists/linux-acpi/msg80607.html

Changes from v2:

- Added Rafael's tag to patch [1/5].
- Updated changelog of patch [1/5] to include more details about how the problem can be observed from dmesg and lspci output.

Changes from v1:

- Drop 'cmax - max ?: 1' and use similar construct than we use in second pass loop in patch [1/5].
- Drop unnecessary parentheses in patch [1/5].
- Added Rafael's tag to patches [2-5/5].

Do you have any comments on the series? It would be nice to get it merged for v4.17 if no objections.