| Message ID | 20210713125007.1260304-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7c3855c423b17f6ca211858afb0cef20569914c7 |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2] PCI: Reinstate "PCI: Coalesce host bridge contiguous apertures" | expand |
On Tue, Jul 13, 2021 at 08:50:07PM +0800, Kai-Heng Feng wrote: > Built-in graphics on HP EliteDesk 805 G6 doesn't work because graphics > can't get the BAR it needs: > pci_bus 0000:00: root bus resource [mem 0x10020200000-0x100303fffff window] > pci_bus 0000:00: root bus resource [mem 0x10030400000-0x100401fffff window] > > pci 0000:00:08.1: bridge window [mem 0xd2000000-0xd23fffff] > pci 0000:00:08.1: bridge window [mem 0x10030000000-0x100401fffff 64bit pref] > pci 0000:00:08.1: can't claim BAR 15 [mem 0x10030000000-0x100401fffff 64bit pref]: no compatible bridge window > pci 0000:00:08.1: [mem 0x10030000000-0x100401fffff 64bit pref] clipped to [mem 0x10030000000-0x100303fffff 64bit pref] > pci 0000:00:08.1: bridge window [mem 0x10030000000-0x100303fffff 64bit pref] > pci 0000:07:00.0: can't claim BAR 0 [mem 0x10030000000-0x1003fffffff 64bit pref]: no compatible bridge window > pci 0000:07:00.0: can't claim BAR 2 [mem 0x10040000000-0x100401fffff 64bit pref]: no compatible bridge window > > However, the root bus has two contiguous apertures that can contain the > child resource requested. > > Coalesce contiguous apertures so we can allocate from the entire contiguous > region. > > This is the second take of commit 65db04053efe ("PCI: Coalesce host > bridge contiguous apertures"). The original approach sorts the apertures > by address, but that makes NVMe stop working on QEMU ppc:sam460ex: > PCI host bridge to bus 0002:00 > pci_bus 0002:00: root bus resource [io 0x0000-0xffff] > pci_bus 0002:00: root bus resource [mem 0xd80000000-0xdffffffff] (bus address [0x80000000-0xffffffff]) > pci_bus 0002:00: root bus resource [mem 0xc0ee00000-0xc0eefffff] (bus address [0x00000000-0x000fffff]) > > After the offending commit: > PCI host bridge to bus 0002:00 > pci_bus 0002:00: root bus resource [io 0x0000-0xffff] > pci_bus 0002:00: root bus resource [mem 0xc0ee00000-0xc0eefffff] (bus address [0x00000000-0x000fffff]) > pci_bus 0002:00: root bus resource [mem 0xd80000000-0xdffffffff] (bus address [0x80000000-0xffffffff]) > > Since the apertures on HP EliteDesk 805 G6 are already in ascending > order, doing a precautious sorting is not necessary. > > Remove the sorting part to avoid the regression on ppc:sam460ex. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212013 > Cc: Guenter Roeck <linux@roeck-us.net> > Suggested-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Applied to pci/resource for v5.16, thanks! > --- > v2: > - Bring back the original commit message. > > drivers/pci/probe.c | 31 +++++++++++++++++++++++++++---- > 1 file changed, 27 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 79177ac37880..5de157600466 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -877,11 +877,11 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) > static int pci_register_host_bridge(struct pci_host_bridge *bridge) > { > struct device *parent = bridge->dev.parent; > - struct resource_entry *window, *n; > + struct resource_entry *window, *next, *n; > struct pci_bus *bus, *b; > - resource_size_t offset; > + resource_size_t offset, next_offset; > LIST_HEAD(resources); > - struct resource *res; > + struct resource *res, *next_res; > char addr[64], *fmt; > const char *name; > int err; > @@ -961,11 +961,34 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) > if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) > dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); > > + /* Coalesce contiguous windows */ > + resource_list_for_each_entry_safe(window, n, &resources) { > + if (list_is_last(&window->node, &resources)) > + break; > + > + next = list_next_entry(window, node); > + offset = window->offset; > + res = window->res; > + next_offset = next->offset; > + next_res = next->res; > + > + if (res->flags != next_res->flags || offset != next_offset) > + continue; > + > + if (res->end + 1 == next_res->start) { > + next_res->start = res->start; > + res->flags = res->start = res->end = 0; > + } > + } > + > /* Add initial resources to the bus */ > resource_list_for_each_entry_safe(window, n, &resources) { > - list_move_tail(&window->node, &bridge->windows); > offset = window->offset; > res = window->res; > + if (!res->end) > + continue; > + > + list_move_tail(&window->node, &bridge->windows); > > if (res->flags & IORESOURCE_BUS) > pci_bus_insert_busn_res(bus, bus->number, res->end); > -- > 2.31.1 >
Hi! So this patch landed now ... :) > + /* Coalesce contiguous windows */ > + resource_list_for_each_entry_safe(window, n, &resources) { > + if (list_is_last(&window->node, &resources)) > + break; > + > + next = list_next_entry(window, node); > + offset = window->offset; > + res = window->res; > + next_offset = next->offset; > + next_res = next->res; > + > + if (res->flags != next_res->flags || offset != next_offset) > + continue; > + > + if (res->end + 1 == next_res->start) { > + next_res->start = res->start; > + res->flags = res->start = res->end = 0; > + } > + } > Maybe this was already a problem before - but I had a stupid thing in arch/um/drivers/virt-pci.c (busn_resource has start == end == 0), and your changes here caused that resource to be dropped off the list. Now this wouldn't be a problem, but we add it using pci_add_resource() and then that does a memory allocation, but you don't free it here? I'm not sure it'd even be safe to free it here and I'll just set busn_resource to have end==1 instead (it's all kind of virtual). But I still wanted to ask if this might be a problem here for others. johannes
Hi Johannes, On Wed, Nov 17, 2021 at 6:41 PM Johannes Berg <johannes@sipsolutions.net> wrote: > So this patch landed now ... :) > > > + /* Coalesce contiguous windows */ > > + resource_list_for_each_entry_safe(window, n, &resources) { > > + if (list_is_last(&window->node, &resources)) > > + break; > > + > > + next = list_next_entry(window, node); > > + offset = window->offset; > > + res = window->res; > > + next_offset = next->offset; > > + next_res = next->res; > > + > > + if (res->flags != next_res->flags || offset != next_offset) > > + continue; > > + > > + if (res->end + 1 == next_res->start) { > > + next_res->start = res->start; > > + res->flags = res->start = res->end = 0; > > + } > > + } > > > > Maybe this was already a problem before - but I had a stupid thing in > arch/um/drivers/virt-pci.c (busn_resource has start == end == 0), and > your changes here caused that resource to be dropped off the list. > > Now this wouldn't be a problem, but we add it using pci_add_resource() > and then that does a memory allocation, but you don't free it here? I'm > not sure it'd even be safe to free it here and I'll just set > busn_resource to have end==1 instead (it's all kind of virtual). > > But I still wanted to ask if this might be a problem here for others. Yes it is. I've sent a fix https://lore.kernel.org/r/9c41a4372b27420c732ff5599d823e363de00c6d.1657704829.git.geert+renesas@glider.be Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 79177ac37880..5de157600466 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -877,11 +877,11 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus) static int pci_register_host_bridge(struct pci_host_bridge *bridge) { struct device *parent = bridge->dev.parent; - struct resource_entry *window, *n; + struct resource_entry *window, *next, *n; struct pci_bus *bus, *b; - resource_size_t offset; + resource_size_t offset, next_offset; LIST_HEAD(resources); - struct resource *res; + struct resource *res, *next_res; char addr[64], *fmt; const char *name; int err; @@ -961,11 +961,34 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge) if (nr_node_ids > 1 && pcibus_to_node(bus) == NUMA_NO_NODE) dev_warn(&bus->dev, "Unknown NUMA node; performance will be reduced\n"); + /* Coalesce contiguous windows */ + resource_list_for_each_entry_safe(window, n, &resources) { + if (list_is_last(&window->node, &resources)) + break; + + next = list_next_entry(window, node); + offset = window->offset; + res = window->res; + next_offset = next->offset; + next_res = next->res; + + if (res->flags != next_res->flags || offset != next_offset) + continue; + + if (res->end + 1 == next_res->start) { + next_res->start = res->start; + res->flags = res->start = res->end = 0; + } + } + /* Add initial resources to the bus */ resource_list_for_each_entry_safe(window, n, &resources) { - list_move_tail(&window->node, &bridge->windows); offset = window->offset; res = window->res; + if (!res->end) + continue; + + list_move_tail(&window->node, &bridge->windows); if (res->flags & IORESOURCE_BUS) pci_bus_insert_busn_res(bus, bus->number, res->end);
Built-in graphics on HP EliteDesk 805 G6 doesn't work because graphics can't get the BAR it needs: pci_bus 0000:00: root bus resource [mem 0x10020200000-0x100303fffff window] pci_bus 0000:00: root bus resource [mem 0x10030400000-0x100401fffff window] pci 0000:00:08.1: bridge window [mem 0xd2000000-0xd23fffff] pci 0000:00:08.1: bridge window [mem 0x10030000000-0x100401fffff 64bit pref] pci 0000:00:08.1: can't claim BAR 15 [mem 0x10030000000-0x100401fffff 64bit pref]: no compatible bridge window pci 0000:00:08.1: [mem 0x10030000000-0x100401fffff 64bit pref] clipped to [mem 0x10030000000-0x100303fffff 64bit pref] pci 0000:00:08.1: bridge window [mem 0x10030000000-0x100303fffff 64bit pref] pci 0000:07:00.0: can't claim BAR 0 [mem 0x10030000000-0x1003fffffff 64bit pref]: no compatible bridge window pci 0000:07:00.0: can't claim BAR 2 [mem 0x10040000000-0x100401fffff 64bit pref]: no compatible bridge window However, the root bus has two contiguous apertures that can contain the child resource requested. Coalesce contiguous apertures so we can allocate from the entire contiguous region. This is the second take of commit 65db04053efe ("PCI: Coalesce host bridge contiguous apertures"). The original approach sorts the apertures by address, but that makes NVMe stop working on QEMU ppc:sam460ex: PCI host bridge to bus 0002:00 pci_bus 0002:00: root bus resource [io 0x0000-0xffff] pci_bus 0002:00: root bus resource [mem 0xd80000000-0xdffffffff] (bus address [0x80000000-0xffffffff]) pci_bus 0002:00: root bus resource [mem 0xc0ee00000-0xc0eefffff] (bus address [0x00000000-0x000fffff]) After the offending commit: PCI host bridge to bus 0002:00 pci_bus 0002:00: root bus resource [io 0x0000-0xffff] pci_bus 0002:00: root bus resource [mem 0xc0ee00000-0xc0eefffff] (bus address [0x00000000-0x000fffff]) pci_bus 0002:00: root bus resource [mem 0xd80000000-0xdffffffff] (bus address [0x80000000-0xffffffff]) Since the apertures on HP EliteDesk 805 G6 are already in ascending order, doing a precautious sorting is not necessary. Remove the sorting part to avoid the regression on ppc:sam460ex. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=212013 Cc: Guenter Roeck <linux@roeck-us.net> Suggested-by: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v2: - Bring back the original commit message. drivers/pci/probe.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)