Thanks a lot for splitting these up. It makes these dramatically
easier to read.
On Mon, Jan 06, 2020 at 03:47:46PM +0000, Nicholas Johnson wrote:
> Change extend_bridge_window() to set resource size directly instead of
> using additional resource lists.
>
> Because additional resource lists are optional resources, any algorithm
> that requires guaranteed allocation that uses them cannot be guaranteed
> to work.
There is never a guarantee that PCI resource assignment will work.
It's always possible that we don't have enough resources to allow us
to enable a device. So I'm not sure what this is telling me, and it
doesn't seem like a justification for setting the resource size
directly here.
Prior to this patch, I think all the assignment and changes to
dev->resource[] were in __assign_resources_sorted(). Maybe it's safe
to do some here and some in __assign_resources_sorted(), but I don't
understand it well enough to be confident.
> Remove the resource from add_list, as a zero-sized additional resource
> is redundant.
>
> Update comment in pci_bus_distribute_available_resources() to reflect
> the above changes.
>
> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@xxxxxxxxxxxxxx>
> ---
> drivers/pci/setup-bus.c | 25 ++++++++-----------------
> 1 file changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index de43815be..0c51f4937 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1836,7 +1836,7 @@ static void adjust_bridge_window(struct pci_dev *bridge, struct resource *res,
> struct list_head *add_list,
> resource_size_t new_size)
> {
> - struct pci_dev_resource *dev_res;
> + resource_size_t add_size;
>
> if (res->parent)
> return;
> @@ -1844,17 +1844,10 @@ static void adjust_bridge_window(struct pci_dev *bridge, struct resource *res,
> if (resource_size(res) >= new_size)
> return;
>
> - dev_res = res_to_dev_res(add_list, res);
> - if (!dev_res)
> - return;
> -
> - /* Is there room to extend the window? */
> - if (new_size - resource_size(res) <= dev_res->add_size)
> - return;
> -
> - dev_res->add_size = new_size - resource_size(res);
> - pci_dbg(bridge, "bridge window %pR extended by %pa\n", res,
> - &dev_res->add_size);
> + add_size = new_size - resource_size(res);
> + pci_dbg(bridge, "bridge window %pR extended by %pa\n", res, &add_size);
> + res->end = res->start + new_size - 1;
How do we know it's safe to extend this, i.e., how do we know there's
nothing immediately after res?
> + remove_from_list(add_list, res);
> }
>
> static void pci_bus_distribute_available_resources(struct pci_bus *bus,
> @@ -1889,11 +1882,9 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
> mmio_pref.start = min(ALIGN(mmio_pref.start, align),
> mmio_pref.end + 1);
>
> - /*
> - * Update additional resource list (add_list) to fill all the
> - * extra resource space available for this port except the space
> - * calculated in __pci_bus_size_bridges() which covers all the
> - * devices currently connected to the port and below.
> + /*
> + * Now that we have adjusted for alignment, update the bridge window
> + * resources to fill as much remaining resource space as possible.
> */
> adjust_bridge_window(bridge, io_res, add_list, resource_size(&io));
> adjust_bridge_window(bridge, mmio_res, add_list, resource_size(&mmio));
> --
> 2.24.1
>
