[PATCH 0/2] PCI: Simplify pci_bus_distribute_available_resources()

[PATCH 0/2] PCI: Simplify pci_bus_distribute_available_resources()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

These are extracted from Nicholas's patch at [1].  I'm pretty sure
these don't change the behavior, but please tell me if I'm wrong.

The second raises some questions about how we distribute resources to
non-hotplug bridges with hotplug bridges below them.  Details in that
commit log.

Nicholas, if you post another revision, feel free to include these at
the beginning.  In that case, you should drop my signed-off-by (which
I always add during the merge process).  Otherwise, let me know if
these look OK and I'll add your signed-off-by.  I dropped it because I
changed things slightly from your original patch, and I didn't want to
attribute any of my errors to you.

[1] https://lore.kernel.org/r/PS2P216MB0642C7A485649D2D787A1C6F80000@PS2P216MB0642.KORP216.PROD.OUTLOOK.COM

Bjorn Helgaas (2):
  PCI: Simplify pci_bus_distribute_available_resources()
  PCI: Skip resource distribution when no hotplug bridges

 drivers/pci/setup-bus.c | 55 +++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 27 deletions(-)

-- 
2.22.0.410.gd8fdbe21b5-goog

[PATCH 1/2] PCI: Simplify pci_bus_distribute_available_resources()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Reorder pci_bus_distribute_available_resources() to group related code
together.  No functional change intended.

Link: https://lore.kernel.org/r/PS2P216MB0642C7A485649D2D787A1C6F80000@PS2P216MB0642.KORP216.PROD.OUTLOOK.COM
Based-on-patch-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
[bhelgaas: extracted from larger patch]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

---

The original order was:

  1) remaining_io = available_io

  2) for_each_pci_bridge(dev, bus)
       # count hotplug_bridges, normal_bridges

  3) for_each_pci_bridge(dev, bus)
       if (!dev->is_hotplug_bridge)
	 # reduce remaining_io by dev window

  4) if (hotplug_bridges + normal_bridges == 1)
       # distribute available_io to the single bridge
       # return

  5) for_each_pci_bridge(dev, bus)
       # distribute remaining_io to hotplug bridges

Blocks 2 and 4 don't require remaining_io, so do them first.  Blocks 1, 3,
5 deal with remaining_io, so group them together.
---
 drivers/pci/setup-bus.c | 50 ++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 0cdd5ff389de..af28af898e42 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1860,16 +1860,6 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 	extend_bridge_window(bridge, mmio_pref_res, add_list,
 			     available_mmio_pref);
 
-	/*
-	 * Calculate the total amount of extra resource space we can
-	 * pass to bridges below this one.  This is basically the
-	 * extra space reduced by the minimal required space for the
-	 * non-hotplug bridges.
-	 */
-	remaining_io = available_io;
-	remaining_mmio = available_mmio;
-	remaining_mmio_pref = available_mmio_pref;
-
 	/*
 	 * Calculate how many hotplug bridges and normal bridges there
 	 * are on this bus.  We will distribute the additional available
@@ -1882,6 +1872,31 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			normal_bridges++;
 	}
 
+	/*
+	 * There is only one bridge on the bus so it gets all available
+	 * resources which it can then distribute to the possible hotplug
+	 * bridges below.
+	 */
+	if (hotplug_bridges + normal_bridges == 1) {
+		dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
+		if (dev->subordinate) {
+			pci_bus_distribute_available_resources(dev->subordinate,
+				add_list, available_io, available_mmio,
+				available_mmio_pref);
+		}
+		return;
+	}
+
+	/*
+	 * Calculate the total amount of extra resource space we can
+	 * pass to bridges below this one.  This is basically the
+	 * extra space reduced by the minimal required space for the
+	 * non-hotplug bridges.
+	 */
+	remaining_io = available_io;
+	remaining_mmio = available_mmio;
+	remaining_mmio_pref = available_mmio_pref;
+
 	for_each_pci_bridge(dev, bus) {
 		const struct resource *res;
 
@@ -1905,21 +1920,6 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 			remaining_mmio_pref -= resource_size(res);
 	}
 
-	/*
-	 * There is only one bridge on the bus so it gets all available
-	 * resources which it can then distribute to the possible hotplug
-	 * bridges below.
-	 */
-	if (hotplug_bridges + normal_bridges == 1) {
-		dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
-		if (dev->subordinate) {
-			pci_bus_distribute_available_resources(dev->subordinate,
-				add_list, available_io, available_mmio,
-				available_mmio_pref);
-		}
-		return;
-	}
-
 	/*
 	 * Go over devices on this bus and distribute the remaining
 	 * resource space between hotplug bridges.
-- 
2.22.0.410.gd8fdbe21b5-goog

[PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

If "hotplug_bridges == 0", "!dev->is_hotplug_bridge" is always true, so the
loop that divides the remaining resources among hotplug-capable bridges
does nothing.

Check for "hotplug_bridges == 0" earlier, so we don't even have to compute
the amount of remaining resources.  No functional change intended.

---

I'm pretty sure this patch preserves the previous behavior of
pci_bus_distribute_available_resources(), but I'm not sure that
behavior is what we want.

For example, in the following topology, when we process bus 10, we
find two non-hotplug bridges and no hotplug bridges, so IIUC we return
without distributing any resources to them.  But I would think we
should try to give 10:1c.0 more space if possible because it has a
hotplug bridge below it.

  00:1c.0: hotplug bridge to [bus 10-2f]
    10:1c.0: non-hotplug bridge to [bus 11-2e]
      11:00.0: hotplug bridge to [bus 12-2e]
    10:1c.1: non-hotplug bridge to [bus 2f]
---
 drivers/pci/setup-bus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index af28af898e42..04adeebe8866 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1887,6 +1887,9 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 		return;
 	}
 
+	if (hotplug_bridges == 0)
+		return;
+
 	/*
 	 * Calculate the total amount of extra resource space we can
 	 * pass to bridges below this one.  This is basically the
@@ -1936,8 +1939,6 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 		 * Distribute available extra resources equally between
 		 * hotplug-capable downstream ports taking alignment into
 		 * account.
-		 *
-		 * Here hotplug_bridges is always != 0.
 		 */
 		align = pci_resource_alignment(bridge, io_res);
 		io = div64_ul(available_io, hotplug_bridges);
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH 1/2] PCI: Simplify pci_bus_distribute_available_resources()

[Mika Westerberg]

On Sat, Jun 22, 2019 at 04:03:10PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Reorder pci_bus_distribute_available_resources() to group related code
> together.  No functional change intended.
> 
> Link: https://lore.kernel.org/r/PS2P216MB0642C7A485649D2D787A1C6F80000@PS2P216MB0642.KORP216.PROD.OUTLOOK.COM
> Based-on-patch-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> [bhelgaas: extracted from larger patch]
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Mika Westerberg]

On Sat, Jun 22, 2019 at 04:03:11PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> If "hotplug_bridges == 0", "!dev->is_hotplug_bridge" is always true, so the
> loop that divides the remaining resources among hotplug-capable bridges
> does nothing.
> 
> Check for "hotplug_bridges == 0" earlier, so we don't even have to compute
> the amount of remaining resources.  No functional change intended.
> 
> ---
> 
> I'm pretty sure this patch preserves the previous behavior of
> pci_bus_distribute_available_resources(), but I'm not sure that
> behavior is what we want.
> 
> For example, in the following topology, when we process bus 10, we
> find two non-hotplug bridges and no hotplug bridges, so IIUC we return
> without distributing any resources to them.  But I would think we
> should try to give 10:1c.0 more space if possible because it has a
> hotplug bridge below it.
> 
>   00:1c.0: hotplug bridge to [bus 10-2f]
>     10:1c.0: non-hotplug bridge to [bus 11-2e]
>       11:00.0: hotplug bridge to [bus 12-2e]
>     10:1c.1: non-hotplug bridge to [bus 2f]

Yes, I agree in this case we want to preserve more space for 10:1c.0.

For this patch,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 1/2] PCI: Simplify pci_bus_distribute_available_resources()

[Logan Gunthorpe]

On 2019-06-22 3:03 p.m., Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Reorder pci_bus_distribute_available_resources() to group related code
> together.  No functional change intended.
> 
> Link: https://lore.kernel.org/r/PS2P216MB0642C7A485649D2D787A1C6F80000@PS2P216MB0642.KORP216.PROD.OUTLOOK.COM
> Based-on-patch-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> [bhelgaas: extracted from larger patch]
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Looks correct to me:

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

> ---
> 
> The original order was:
> 
>   1) remaining_io = available_io
> 
>   2) for_each_pci_bridge(dev, bus)
>        # count hotplug_bridges, normal_bridges
> 
>   3) for_each_pci_bridge(dev, bus)
>        if (!dev->is_hotplug_bridge)
> 	 # reduce remaining_io by dev window
> 
>   4) if (hotplug_bridges + normal_bridges == 1)
>        # distribute available_io to the single bridge
>        # return
> 
>   5) for_each_pci_bridge(dev, bus)
>        # distribute remaining_io to hotplug bridges
> 
> Blocks 2 and 4 don't require remaining_io, so do them first.  Blocks 1, 3,
> 5 deal with remaining_io, so group them together.
> ---
>  drivers/pci/setup-bus.c | 50 ++++++++++++++++++++---------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 0cdd5ff389de..af28af898e42 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1860,16 +1860,6 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
>  	extend_bridge_window(bridge, mmio_pref_res, add_list,
>  			     available_mmio_pref);
>  
> -	/*
> -	 * Calculate the total amount of extra resource space we can
> -	 * pass to bridges below this one.  This is basically the
> -	 * extra space reduced by the minimal required space for the
> -	 * non-hotplug bridges.
> -	 */
> -	remaining_io = available_io;
> -	remaining_mmio = available_mmio;
> -	remaining_mmio_pref = available_mmio_pref;
> -
>  	/*
>  	 * Calculate how many hotplug bridges and normal bridges there
>  	 * are on this bus.  We will distribute the additional available
> @@ -1882,6 +1872,31 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
>  			normal_bridges++;
>  	}
>  
> +	/*
> +	 * There is only one bridge on the bus so it gets all available
> +	 * resources which it can then distribute to the possible hotplug
> +	 * bridges below.
> +	 */
> +	if (hotplug_bridges + normal_bridges == 1) {
> +		dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
> +		if (dev->subordinate) {
> +			pci_bus_distribute_available_resources(dev->subordinate,
> +				add_list, available_io, available_mmio,
> +				available_mmio_pref);
> +		}
> +		return;
> +	}
> +
> +	/*
> +	 * Calculate the total amount of extra resource space we can
> +	 * pass to bridges below this one.  This is basically the
> +	 * extra space reduced by the minimal required space for the
> +	 * non-hotplug bridges.
> +	 */
> +	remaining_io = available_io;
> +	remaining_mmio = available_mmio;
> +	remaining_mmio_pref = available_mmio_pref;
> +
>  	for_each_pci_bridge(dev, bus) {
>  		const struct resource *res;
>  
> @@ -1905,21 +1920,6 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
>  			remaining_mmio_pref -= resource_size(res);
>  	}
>  
> -	/*
> -	 * There is only one bridge on the bus so it gets all available
> -	 * resources which it can then distribute to the possible hotplug
> -	 * bridges below.
> -	 */
> -	if (hotplug_bridges + normal_bridges == 1) {
> -		dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
> -		if (dev->subordinate) {
> -			pci_bus_distribute_available_resources(dev->subordinate,
> -				add_list, available_io, available_mmio,
> -				available_mmio_pref);
> -		}
> -		return;
> -	}
> -
>  	/*
>  	 * Go over devices on this bus and distribute the remaining
>  	 * resource space between hotplug bridges.
> 

Re: [PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Logan Gunthorpe]

On 2019-06-22 3:03 p.m., Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> If "hotplug_bridges == 0", "!dev->is_hotplug_bridge" is always true, so the
> loop that divides the remaining resources among hotplug-capable bridges
> does nothing.
> 
> Check for "hotplug_bridges == 0" earlier, so we don't even have to compute
> the amount of remaining resources.  No functional change intended.

Makes sense.

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

> ---
> 
> I'm pretty sure this patch preserves the previous behavior of
> pci_bus_distribute_available_resources(), but I'm not sure that
> behavior is what we want.
> 
> For example, in the following topology, when we process bus 10, we
> find two non-hotplug bridges and no hotplug bridges, so IIUC we return
> without distributing any resources to them.  But I would think we
> should try to give 10:1c.0 more space if possible because it has a
> hotplug bridge below it.
> 
>   00:1c.0: hotplug bridge to [bus 10-2f]
>     10:1c.0: non-hotplug bridge to [bus 11-2e]
>       11:00.0: hotplug bridge to [bus 12-2e]
>     10:1c.1: non-hotplug bridge to [bus 2f]
> ---
>  drivers/pci/setup-bus.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index af28af898e42..04adeebe8866 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1887,6 +1887,9 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
>  		return;
>  	}
>  
> +	if (hotplug_bridges == 0)
> +		return;
> +
>  	/*
>  	 * Calculate the total amount of extra resource space we can
>  	 * pass to bridges below this one.  This is basically the
> @@ -1936,8 +1939,6 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
>  		 * Distribute available extra resources equally between
>  		 * hotplug-capable downstream ports taking alignment into
>  		 * account.
> -		 *
> -		 * Here hotplug_bridges is always != 0.
>  		 */
>  		align = pci_resource_alignment(bridge, io_res);
>  		io = div64_ul(available_io, hotplug_bridges);
> 

Re: [PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Benjamin Herrenschmidt]

On Mon, 2019-06-24 at 14:24 +0300, Mika Westerberg wrote:
> > 
> > I'm pretty sure this patch preserves the previous behavior of
> > pci_bus_distribute_available_resources(), but I'm not sure that
> > behavior is what we want.
> > 
> > For example, in the following topology, when we process bus 10, we
> > find two non-hotplug bridges and no hotplug bridges, so IIUC we
> > return
> > without distributing any resources to them.  But I would think we
> > should try to give 10:1c.0 more space if possible because it has a
> > hotplug bridge below it.
> > 
> >    00:1c.0: hotplug bridge to [bus 10-2f]
> >      10:1c.0: non-hotplug bridge to [bus 11-2e]
> >        11:00.0: hotplug bridge to [bus 12-2e]
> >      10:1c.1: non-hotplug bridge to [bus 2f]
> 
> Yes, I agree in this case we want to preserve more space for 10:1c.0.

I sitll can't make sense of any of this stuff though.

We only every distribute resources when using
pci_assign_unassigned_bridge_resources which we only use some cases,
and it's completely non obvious why we would use it there and not in
other places.

We also don't distribute during the initial root survey meaning afaik
that we get toast for any hotplug bridge that has stuff already there
at boot.

Also, distributing the "available" space means we leave nothing for
potential SR-IOV siblings... have we ended up bloting the very PCIe-
centric assumption that it's "unlikely" that a hotplug bridge has an
SR-IOV sibling ?

This is all a terrible mess and I feel that all these little tweaks
here or there are just making it even more impossible to completely
grasp or predict how it will behave.

Ben.

Re: [PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Mika Westerberg]

On Tue, Jun 25, 2019 at 09:45:04AM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2019-06-24 at 14:24 +0300, Mika Westerberg wrote:
> > > 
> > > I'm pretty sure this patch preserves the previous behavior of
> > > pci_bus_distribute_available_resources(), but I'm not sure that
> > > behavior is what we want.
> > > 
> > > For example, in the following topology, when we process bus 10, we
> > > find two non-hotplug bridges and no hotplug bridges, so IIUC we
> > > return
> > > without distributing any resources to them.  But I would think we
> > > should try to give 10:1c.0 more space if possible because it has a
> > > hotplug bridge below it.
> > > 
> > >    00:1c.0: hotplug bridge to [bus 10-2f]
> > >      10:1c.0: non-hotplug bridge to [bus 11-2e]
> > >        11:00.0: hotplug bridge to [bus 12-2e]
> > >      10:1c.1: non-hotplug bridge to [bus 2f]
> > 
> > Yes, I agree in this case we want to preserve more space for 10:1c.0.
> 
> I sitll can't make sense of any of this stuff though.
> 
> We only every distribute resources when using
> pci_assign_unassigned_bridge_resources which we only use some cases,
> and it's completely non obvious why we would use it there and not in
> other places.

We added it only for native PCIe hotplug path with the assumption that
the boot firmware takes care of the initial resource allocation. I don't
see any particular reason why it could not be called for other paths as
well, though.

> We also don't distribute during the initial root survey meaning afaik
> that we get toast for any hotplug bridge that has stuff already there
> at boot.

The boot firmware obviously needs to follow the same logic. AFAICT
recent PCs and Macs using native PCIe hotplug handle it.

> Also, distributing the "available" space means we leave nothing for
> potential SR-IOV siblings... have we ended up bloting the very PCIe-
> centric assumption that it's "unlikely" that a hotplug bridge has an
> SR-IOV sibling ?

Looking at the code, I'm not sure we reserved any additional resource
space for the SR-IOV even before pci_bus_distribute_available_resources()
was introduced. We do reserve extra bus numbers for SR-IOV in
pci_scan_child_bus_extend() so maybe we can add something similar to
resource allocation path.

Re: [PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Benjamin Herrenschmidt]

On Tue, 2019-06-25 at 13:05 +0300, Mika Westerberg wrote:
> > We only every distribute resources when using
> > pci_assign_unassigned_bridge_resources which we only use some cases,
> > and it's completely non obvious why we would use it there and not in
> > other places.
> 
> We added it only for native PCIe hotplug path with the assumption that
> the boot firmware takes care of the initial resource allocation. I don't
> see any particular reason why it could not be called for other paths as
> well, though.

Ok, we need to look into this for all the platforms who just reassign
everything in Linux (ie, ignore whatever the boot firmware did, if it
did anything).

I feel like all these platforms today will have a hard time getting
anything useful out of hotplug with our default "2M" add to the hotplug
bridges :)

> > We also don't distribute during the initial root survey meaning afaik
> > that we get toast for any hotplug bridge that has stuff already there
> > at boot.
> 
> The boot firmware obviously needs to follow the same logic. AFAICT
> recent PCs and Macs using native PCIe hotplug handle it.

What's your experience in that area ? How (well) do they handle it in
the boot firmware ? at least on arm64, boot firmwares are rather
catastrophic when it comes to PCI, and on other embedded devices they
are basically non-existent.

> > Also, distributing the "available" space means we leave nothing for
> > potential SR-IOV siblings... have we ended up bloting the very PCIe-
> > centric assumption that it's "unlikely" that a hotplug bridge has an
> > SR-IOV sibling ?
> 
> Looking at the code, I'm not sure we reserved any additional resource
> space for the SR-IOV even before pci_bus_distribute_available_resources()
> was introduced. We do reserve extra bus numbers for SR-IOV in
> pci_scan_child_bus_extend() so maybe we can add something similar to
> resource allocation path.

Ok. I'll look more. I think we do somewhat cater for SR-IOV in in the
bridge sizing code actually. It's a bit obscure...

I also need to look a bit more closely at what happens with
Thunderbolt.

Thanks !

Cheers
Ben.

Re: [PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Mika Westerberg]

On Tue, Jun 25, 2019 at 09:48:19PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2019-06-25 at 13:05 +0300, Mika Westerberg wrote:
> > > We only every distribute resources when using
> > > pci_assign_unassigned_bridge_resources which we only use some cases,
> > > and it's completely non obvious why we would use it there and not in
> > > other places.
> > 
> > We added it only for native PCIe hotplug path with the assumption that
> > the boot firmware takes care of the initial resource allocation. I don't
> > see any particular reason why it could not be called for other paths as
> > well, though.
> 
> Ok, we need to look into this for all the platforms who just reassign
> everything in Linux (ie, ignore whatever the boot firmware did, if it
> did anything).
> 
> I feel like all these platforms today will have a hard time getting
> anything useful out of hotplug with our default "2M" add to the hotplug
> bridges :)

Yeah, at least if Thunderbolt is involved each "daisy-chained" device
adds a complete PCIe switch running out of resources rather quick.

> > > We also don't distribute during the initial root survey meaning afaik
> > > that we get toast for any hotplug bridge that has stuff already there
> > > at boot.
> > 
> > The boot firmware obviously needs to follow the same logic. AFAICT
> > recent PCs and Macs using native PCIe hotplug handle it.
> 
> What's your experience in that area ? How (well) do they handle it in
> the boot firmware ? at least on arm64, boot firmwares are rather
> catastrophic when it comes to PCI, and on other embedded devices they
> are basically non-existent.

Well my experience is quite limited to recent Macs and PCs which usually
handle the initial resource allocation just fine. In case of Thunderbolt
some "older" PCs handle everything in firmware, even the runtime
resource allocation via SMI handler accompanied with ACPI hotplug.

Re: [PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Benjamin Herrenschmidt]

On Tue, 2019-06-25 at 15:04 +0300, Mika Westerberg wrote:
> > What's your experience in that area ? How (well) do they handle it in
> > the boot firmware ? at least on arm64, boot firmwares are rather
> > catastrophic when it comes to PCI, and on other embedded devices they
> > are basically non-existent.
> 
> Well my experience is quite limited to recent Macs and PCs which usually
> handle the initial resource allocation just fine. In case of Thunderbolt
> some "older" PCs handle everything in firmware, even the runtime
> resource allocation via SMI handler accompanied with ACPI hotplug.

Ah so this is what Lenovo calls "Thunderbolt firmware assist" in the
BIOS ? I turned that on, it did help with Linux :)

Cheers,
Ben.

Re: [PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Mika Westerberg]

On Tue, Jun 25, 2019 at 10:23:33PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2019-06-25 at 15:04 +0300, Mika Westerberg wrote:
> > > What's your experience in that area ? How (well) do they handle it in
> > > the boot firmware ? at least on arm64, boot firmwares are rather
> > > catastrophic when it comes to PCI, and on other embedded devices they
> > > are basically non-existent.
> > 
> > Well my experience is quite limited to recent Macs and PCs which usually
> > handle the initial resource allocation just fine. In case of Thunderbolt
> > some "older" PCs handle everything in firmware, even the runtime
> > resource allocation via SMI handler accompanied with ACPI hotplug.
> 
> Ah so this is what Lenovo calls "Thunderbolt firmware assist" in the
> BIOS ?

Yes, exactly.

> I turned that on, it did help with Linux :)

Well, it should also work using native PCIe with recent kernels. At
least that's what I've been trying to get working since last year ;-)

Re: [PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Benjamin Herrenschmidt]

On Tue, 2019-06-25 at 15:04 +0300, Mika Westerberg wrote:
> > What's your experience in that area ? How (well) do they handle it in
> > the boot firmware ? at least on arm64, boot firmwares are rather
> > catastrophic when it comes to PCI, and on other embedded devices they
> > are basically non-existent.
> 
> Well my experience is quite limited to recent Macs and PCs which usually
> handle the initial resource allocation just fine. In case of Thunderbolt
> some "older" PCs handle everything in firmware, even the runtime
> resource allocation via SMI handler accompanied with ACPI hotplug.

So I'm tempted to toy a bit with the "realloc everything" platforms
(all non-x86 embedded basically) use
pci_bus_distribute_available_resources on the PCIe root ports
unconditionally.

I don't think it will be a problem with SR-IOV as I very much doubt
you'll end up with a setup where we have under the root port SR-IOV
devices that are *siblings* with a switch. If that ever becomes the
case, SR-IOV should be hanlded somewhat as part of the add_list of the
bus sizing anyway.

I might do that as a test patch or behind a test config option, see how
it goes, after I've consolidated all those platforms to go through the
same generic code path, it will be a lot easier.

I'm keen to limit that to PCIe root ports though, old stuff can stay as
it is and die happily :)

Cheers,
Ben.

Re: [PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Bjorn Helgaas]

On Tue, Jun 25, 2019 at 09:45:04AM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2019-06-24 at 14:24 +0300, Mika Westerberg wrote:
> > > 
> > > I'm pretty sure this patch preserves the previous behavior of
> > > pci_bus_distribute_available_resources(), but I'm not sure that
> > > behavior is what we want.
> > > 
> > > For example, in the following topology, when we process bus 10, we
> > > find two non-hotplug bridges and no hotplug bridges, so IIUC we
> > > return
> > > without distributing any resources to them.  But I would think we
> > > should try to give 10:1c.0 more space if possible because it has a
> > > hotplug bridge below it.
> > > 
> > >    00:1c.0: hotplug bridge to [bus 10-2f]
> > >      10:1c.0: non-hotplug bridge to [bus 11-2e]
> > >        11:00.0: hotplug bridge to [bus 12-2e]
> > >      10:1c.1: non-hotplug bridge to [bus 2f]
> > 
> > Yes, I agree in this case we want to preserve more space for 10:1c.0.
> 
> I sitll can't make sense of any of this stuff though.
> 
> We only every distribute resources when using
> pci_assign_unassigned_bridge_resources which we only use some cases,
> and it's completely non obvious why we would use it there and not in
> other places.
> 
> We also don't distribute during the initial root survey meaning afaik
> that we get toast for any hotplug bridge that has stuff already there
> at boot.
> 
> Also, distributing the "available" space means we leave nothing for
> potential SR-IOV siblings... have we ended up bloting the very PCIe-
> centric assumption that it's "unlikely" that a hotplug bridge has an
> SR-IOV sibling ?
> 
> This is all a terrible mess and I feel that all these little tweaks
> here or there are just making it even more impossible to completely
> grasp or predict how it will behave.

No argument about it being a mess.

I agree that tweaks clutter the history, which is definitely a
downside.  Do you think these actually change the way things work or
make the code harder to read?

I think there is value in even minor simplifications that make the
code easier to understand.  Small improvements compound over time and
expose opportunities for more significant improvement.

Bjorn

Re: [PATCH 2/2] PCI: Skip resource distribution when no hotplug bridges

[Benjamin Herrenschmidt]

On Wed, 2019-06-26 at 12:35 -0500, Bjorn Helgaas wrote:
> No argument about it being a mess.
> 
> I agree that tweaks clutter the history, which is definitely a
> downside.  Do you think these actually change the way things work or
> make the code harder to read?
> 
> I think there is value in even minor simplifications that make the
> code easier to understand.  Small improvements compound over time and
> expose opportunities for more significant improvement.

Oh I absolutely agree. And I love that your patches come with more cset
comment than actual patch lines :-)

The main issue I've had so far trying to untangle things is the sheer
amount of subtle changes and tweaks that went in over the year without
any useful explanation as to why things are done.

For example, do you have any idea why this:

d65245c3297ac63abc51a976d92f45f2195d2854
PCI: don't shrink bridge resources

Was added by Yinghai in 2010 ? :-)

The main issue with this is that previous to this commit,
pbus_size_{io,mem} would essentially ignore the previous state of the
bridge resources, and calculate from scratch (provided the resources
are unclaimed).

After this, it has this subtle dependency.

This is what broke Lorenzo attempts at moving pci_read_bridge_bases()
to the geneneric code a couple of years ago for example. There may be a
good reason to do that on x86, though it's not explained, but it's
definitely not right if the platform requires a full re-assignment.

Now I'll "work around" it by making that function look at the
assignment policy set by the arch/platform, but it's a fix on top of a
quirk on top of a band-aid. However, what else can we do without
understanding the root issue that lead to that patch being created ?

Cheers,
Ben.