[PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[Sanath S]

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

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

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216000
Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
Signed-off-by: Sanath S <Sanath.S@amd.com>
---
 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) {
+		pci_info(dev, "Bridge has subordinate 0x%x but max busn 0x%x, reconfiguring\n",
+			 subordinate, end);
+		broken = 1;
+	}
+
 	/*
 	 * Disable Master-Abort Mode during probing to avoid reporting of
 	 * bus errors in some architectures.
-- 
2.34.1

Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[Sanath S]

Adding Mika.

On 8/16/2023 10:49 AM, 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
>
> 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
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216000
> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
> Signed-off-by: Sanath S <Sanath.S@amd.com>
> ---
>   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) {
> +		pci_info(dev, "Bridge has subordinate 0x%x but max busn 0x%x, reconfiguring\n",
> +			 subordinate, end);
> +		broken = 1;
> +	}
> +
>   	/*
>   	 * Disable Master-Abort Mode during probing to avoid reporting of
>   	 * bus errors in some architectures.

Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[Mika Westerberg]

On Wed, Aug 16, 2023 at 06:48:35PM +0530, Sanath S wrote:
> Adding Mika.

Thanks!

> On 8/16/2023 10:49 AM, 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

This is something the BIOS should be doing but for some reason it is
not on that particular system.

> > In case of a thunderbolt capable bridge, reconfigure the buses allocated

Thunderbolt

> > 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.

This is necessary only when there is no proper BIOS allocation for the
resources.

> > 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
> > 
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216000

Did you get confirmation that this actually solves the issue?

> > Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
> > Signed-off-by: Sanath S <Sanath.S@amd.com>
> > ---
> >   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) {
> > +		pci_info(dev, "Bridge has subordinate 0x%x but max busn 0x%x, reconfiguring\n",
> > +			 subordinate, end);
> > +		broken = 1;
> > +	}
> > +
> >   	/*
> >   	 * Disable Master-Abort Mode during probing to avoid reporting of
> >   	 * bus errors in some architectures.

Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[Sanath S]

On 8/17/2023 3:54 PM, Mika Westerberg wrote:
> On Wed, Aug 16, 2023 at 06:48:35PM +0530, Sanath S wrote:
>> Adding Mika.
> Thanks!
>
>> On 8/16/2023 10:49 AM, 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
> This is something the BIOS should be doing but for some reason it is
> not on that particular system.
Yes, BIOS should be doing it. Idea here is if BIOS has not distributed 
it correctly, OS
can reallocate and distribute it correctly.
>>> In case of a thunderbolt capable bridge, reconfigure the buses allocated
> Thunderbolt
Will correct it.
>
>>> 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.
> This is necessary only when there is no proper BIOS allocation for the
> resources.
Yes, will send out a v2 with updated commit message.
>
>>> 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
>>>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216000
> Did you get confirmation that this actually solves the issue?
I've tested this on my setup, it is resolving the issue.
>
>>> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
>>> Signed-off-by: Sanath S <Sanath.S@amd.com>
>>> ---
>>>    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) {
>>> +		pci_info(dev, "Bridge has subordinate 0x%x but max busn 0x%x, reconfiguring\n",
>>> +			 subordinate, end);
>>> +		broken = 1;
>>> +	}
>>> +
>>>    	/*
>>>    	 * Disable Master-Abort Mode during probing to avoid reporting of
>>>    	 * bus errors in some architectures.

Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[Mika Westerberg]

On Fri, Aug 18, 2023 at 09:01:50AM +0530, Sanath S wrote:
> 
> On 8/17/2023 3:54 PM, Mika Westerberg wrote:
> > On Wed, Aug 16, 2023 at 06:48:35PM +0530, Sanath S wrote:
> > > Adding Mika.
> > Thanks!
> > 
> > > On 8/16/2023 10:49 AM, 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
> > This is something the BIOS should be doing but for some reason it is
> > not on that particular system.
> Yes, BIOS should be doing it. Idea here is if BIOS has not distributed it
> correctly, OS
> can reallocate and distribute it correctly.
> > > > In case of a thunderbolt capable bridge, reconfigure the buses allocated
> > Thunderbolt
> Will correct it.
> > 
> > > > 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.
> > This is necessary only when there is no proper BIOS allocation for the
> > resources.
> Yes, will send out a v2 with updated commit message.
> > 
> > > > 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
> > > > 
> > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216000
> > Did you get confirmation that this actually solves the issue?
> I've tested this on my setup, it is resolving the issue.

Right, but it would be good to get confirmation from the person who
reported the issue that this actually helps. There is nothing in the
bugzilla whether the patch worked or not.

Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[Maciej W. Rozycki]

On Fri, 18 Aug 2023, Mika Westerberg wrote:

> > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216000
> > > Did you get confirmation that this actually solves the issue?
> > I've tested this on my setup, it is resolving the issue.
> 
> Right, but it would be good to get confirmation from the person who
> reported the issue that this actually helps. There is nothing in the
> bugzilla whether the patch worked or not.

 If you do change the defaults, then please don't forget to update 
Documentation/admin-guide/kernel-parameters.txt in the same commit 
accordingly for `hpbussize=nn', etc.

 NB it seems a common problem with vendor firmware failing to assign a 
reasonable quantity of downstream buses for hot-plug ports.  E.g. with my 
production laptop back from 2018 a single-device ExpressCard option, such 
as a PCIe serial port works just fine with hot-plug, however if I hot-plug 
a whole bus hierarchy in an external enclosure, then the system runs out 
of buses at the first PCIe switch (unless I use `hpbussize=nn', etc.).

  Maciej

Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[kernel test robot]

Hello,

kernel test robot noticed disk issues on:

commit: 4f0dbecaea6160d50d568fcaa4e09346052f7423 ("[PATCH] PCI: Allocate maximum available buses to help extending the daisy chain")
url: https://github.com/intel-lab-lkp/linux/commits/Sanath-S/PCI-Allocate-maximum-available-buses-to-help-extending-the-daisy-chain/20230816-132324
base: https://git.kernel.org/cgit/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/all/20230816051923.2287912-1-Sanath.S@amd.com/
patch subject: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

in testcase: filebench
version: filebench-x86_64-22620e6-1_20221010
with following parameters:

	fs: xfs
	test: filemicro_statfile.f
	cpufreq_governor: performance



compiler: gcc-12
test machine: 96 threads 2 sockets Intel(R) Xeon(R) Platinum 8260L CPU @ 2.40GHz (Cascade Lake) with 128G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)




If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202308232106.50c8f492-oliver.sang@intel.com


[    6.738560][    T1] pci 0000:17:00.0: Bridge has subordinate 0x19 but max busn 0x39, reconfiguring
...
[  238.092454][ T1230] ls: cannot access '/dev/nvme1n1p1': No such file or directory
[  238.092457][ T1230]
[  238.526845][ T1230] disk number 2 mismatch with 1 real disks: /dev/nvme1n1p1 /dev/nvme0n1p1


before this commit, we have below two disks on server:

# ls -l /dev/disk/by-path/
lrwxrwxrwx 1 root root  13 Aug 23 23:00 pci-0000:83:00.0-nvme-1 -> ../../nvme0n1
lrwxrwxrwx 1 root root  15 Aug 23 23:00 pci-0000:83:00.0-nvme-1-part1 -> ../../nvme0n1p1
lrwxrwxrwx 1 root root  13 Aug 23 23:00 pci-0000:84:00.0-nvme-1 -> ../../nvme1n1
lrwxrwxrwx 1 root root  15 Aug 23 23:00 pci-0000:84:00.0-nvme-1-part1 -> ../../nvme1n1p1

but after this commit, there is no nvme1n1p1 detected



The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20230823/202308232106.50c8f492-oliver.sang@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[Bjorn Helgaas]

[+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.

I guess this patch is on hold for now because the kernel test robot
complained:
https://lore.kernel.org/r/202308232106.50c8f492-oliver.sang@intel.com
and this hasn't been resolved or explained yet.

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216000
> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
> Signed-off-by: Sanath S <Sanath.S@amd.com>
> ---
>  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
> 

Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[Mario Limonciello]

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@intel.com
> 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@black.fi.intel.com/

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@amd.com>
>> Signed-off-by: Sanath S <Sanath.S@amd.com>
>> ---
>>   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
>>

Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[Bjorn Helgaas]

On Fri, Dec 08, 2023 at 04:29:42PM -0600, Mario Limonciello wrote:
> On 12/8/2023 16:24, Bjorn Helgaas wrote:
> > 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.
> ...

> 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@black.fi.intel.com/
> 
> So Sanath has a new patch that does this that is under testing right
> now and will be posted soon.

Hmm, ok.  I don't know what a "downstream port reset" does or how it
resolves the bus number allocation issue, but I'm happy if you have a
fix that doesn't need PCI core changes.

Bjorn

Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[Mario Limonciello]

On 12/8/2023 16:44, Bjorn Helgaas wrote:
> On Fri, Dec 08, 2023 at 04:29:42PM -0600, Mario Limonciello wrote:
>> On 12/8/2023 16:24, Bjorn Helgaas wrote:
>>> 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.
>> ...
> 
>> 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@black.fi.intel.com/
>>
>> So Sanath has a new patch that does this that is under testing right
>> now and will be posted soon.
> 
> Hmm, ok.  I don't know what a "downstream port reset" does or how it
> resolves the bus number allocation issue, but I'm happy if you have a
> fix that doesn't need PCI core changes.
> 
> Bjorn

The issue is specifically with resources that were assigned with BIOS in 
this "static case".  The downstream port reset ends up resetting the 
topology and thus the resources get assigned by Linux instead and will
be better balanced for more devices to be daisy chained.

Re: [PATCH] PCI: Allocate maximum available buses to help extending the daisy chain

[Bjorn Helgaas]

On Mon, Dec 11, 2023 at 03:48:41PM -0600, Mario Limonciello wrote:
> On 12/8/2023 16:44, Bjorn Helgaas wrote:
> > On Fri, Dec 08, 2023 at 04:29:42PM -0600, Mario Limonciello wrote:
> > > On 12/8/2023 16:24, Bjorn Helgaas wrote:
> > > > 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.
> > > ...
> > 
> > > 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@black.fi.intel.com/
> > > 
> > > So Sanath has a new patch that does this that is under testing right
> > > now and will be posted soon.
> > 
> > Hmm, ok.  I don't know what a "downstream port reset" does or how it
> > resolves the bus number allocation issue, but I'm happy if you have a
> > fix that doesn't need PCI core changes.
> 
> The issue is specifically with resources that were assigned with BIOS in
> this "static case".  The downstream port reset ends up resetting the
> topology and thus the resources get assigned by Linux instead and will
> be better balanced for more devices to be daisy chained.

It sounds like the downstream port reset maybe just resets the bridge
secondary/subordinate bus numbers, which forces Linux to reassign
them?  But Linux isn't smart enough to proactively reassign them?  If
so, the reset sounds a little like a band-aid, not a real fix, but I'm
guessing nobody is signing up to rework that PCI core reassignment
code ;)

Bjorn