On Mon, Feb 26, 2018 at 04:21:09PM +0300, Mika Westerberg wrote:
> 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".
>From a PCI point of view, I assume you're adding a switch with 4 NIC
endpoints below it.
> 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?
> 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.
> 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 <mika.westerberg@xxxxxxxxxxxxxxx>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> drivers/pci/setup-bus.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 3cce29a069e6..f1e6172734f0 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -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,
> + 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.
> }
> }
> }
> --
> 2.16.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
