Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain
From: Mario Limonciello
Date: Fri Dec 08 2023 - 17:29:52 EST
On 12/8/2023 16:24, Bjorn Helgaas wrote:
[+cc Mika, Maciej]
On Wed, Aug 16, 2023 at 10:49:23AM +0530, Sanath S wrote:
In the case of Thunderbolt, it contains a PCIe switch and one or
more hotplug-capable PCIe downstream ports where the daisy chain
can be extended.
Currently when a Thunderbolt Dock is plugged in during S5/Reboot,
System BIOS allocates a very minimal number of buses for bridges and
hot-plug capable PCIe downstream ports to enumerate the dock during
boot. Because of this, we run out of bus space pretty quickly when
more PCIe devices are attached to hotplug downstream ports in order
to extend the chain.
Before:
+-04.0
+-04.1-[63-c1]----00.0-[64-69]--+-00.0-[65]--
| +-01.0-[66]--
| +-02.0-[67]--
| +-03.0-[68]--
| \-04.0-[69]--
+-08.0
Looks like a clear issue here because there's no other use for
buses 70-c1. But what would happen if there were more hotplug-capable
downstream ports, e.g., assume one at 08.1 leading to [bus c2-c7]?
The 04.1 bridge has a lot of space, but 08.1 has very little. With
this patch, would we distribute it more evenly across 04.1 and 08.1?
If not, I think we'll just have the same problem when somebody plugs
in a similar hierarchy at 08.1.
In case of a thunderbolt capable bridge, reconfigure the buses allocated
by BIOS to the maximum available buses. So that the hot-plug bridges gets
maximum buses and chain can be extended to accommodate more PCIe devices.
This fix is necessary for all the PCIe downstream ports where the daisy
chain can be extended.
After:
+-04.0
+-04.1-[63-c1]----00.0-[64-c1]--+-00.0-[65]--
| +-01.0-[66-84]--
| +-02.0-[85-a3]--
| +-03.0-[a4-c0]--
| \-04.0-[c1]--
+-08.0
This doesn't look like anything specific to Thunderbolt; it's just
that we don't do a good job of reassigning bus numbers in general,
right? We shouldn't just punt and say "BIOS should have done
something" because not all machines *have* BIOS, and the OS can
reconfigure bus numbers as needed. The patch certainly isn't
Thunderbolt-specific.
From the discussions Sanath and I have been in related to this issue
the BIOS is pretty static with it's initialization under the presumption
that the OS will rebalance things if necessary.
I guess this patch is on hold for now because the kernel test robot
complained:
https://lore.kernel.org/r/202308232106.50c8f492-oliver.sang@xxxxxxxxx
and this hasn't been resolved or explained yet.
For this particular issue it's being approached a different way.
Windows never rebalances things but doesn't suffer from this issue.
That's because Windows actually does a "Downstream port reset" when it
encounters a USB4 router.
Sanath posted a quirk that aligned this behavior when encountering an
AMD USB4 router, but as part of the discussion I suggested that we do it
for everyone.
https://lore.kernel.org/linux-usb/20231123065739.GC1074920@xxxxxxxxxxxxxxxxxx/
So Sanath has a new patch that does this that is under testing right now
and will be posted soon.
Thanks!
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216000
Signed-off-by: Sanjay R Mehta <sanju.mehta@xxxxxxx>
Signed-off-by: Sanath S <Sanath.S@xxxxxxx>
---
drivers/pci/probe.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8bac3ce02609..ab7e90ef2382 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1263,6 +1263,8 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
bool fixed_buses;
u8 fixed_sec, fixed_sub;
int next_busnr;
+ int start = bus->busn_res.start;
+ int end = bus->busn_res.end;
/*
* Make sure the bridge is powered on to be able to access config
@@ -1292,6 +1294,13 @@ static int pci_scan_bridge_extend(struct pci_bus *bus, struct pci_dev *dev,
broken = 1;
}
+ /* Reconfigure, If maximum buses are not allocated */
+ if (!pass && start != 0 && end != 0xff && subordinate != end) {
I don't quite understand the test here. In the "Before" example
above, I think bus->busn_res is [bus 63-c1], and subordinate is 69.
That certainly makes this condition true, but wouldn't you also want
to reallocate bus numbers if bus->busn_res were [bus 63-ff] and
subordinate were 69?
+ pci_info(dev, "Bridge has subordinate 0x%x but max busn 0x%x, reconfiguring\n",
Most other logging here starts with lower-case, e.g., "bridge has ..."
Print the bus numbers in the typical format ("%02x"). Maybe use "%pR"
and &bus->busn_res for the first part.
+ subordinate, end);
+ broken = 1;
+ }
+
/*
* Disable Master-Abort Mode during probing to avoid reporting of
* bus errors in some architectures.
--
2.34.1