Re: [PATCH 1/1] PCI: Coalesce host bridge contiguous apertures without sorting

[Guenter Roeck]

On 7/13/21 1:49 AM, Kai-Heng Feng wrote:
> On Tue, Jul 13, 2021 at 4:45 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > On 7/13/21 12:57 AM, Kai-Heng Feng wrote:
> > > Commit 65db04053efe ("PCI: Coalesce host bridge contiguous apertures")
> > > sorts the resources by address so the resources can be swapped.
> > >
> > > Before:
> > > 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])
> > >
> > > And after:
> > > 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])
> > >
> > > However, the sorted resources make NVMe stops working on QEMU ppc:sam460ex.
> > >
> > > Resources in the original issue are already in ascending order:
> > > pci_bus 0000:00: root bus resource [mem 0x10020200000-0x100303fffff window]
> > > pci_bus 0000:00: root bus resource [mem 0x10030400000-0x100401fffff window]
> > >
> > > So remove the sorting part to resolve the issue.
> > >
> > > Reported-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > > Fixes: 65db04053efe ("PCI: Coalesce host bridge contiguous apertures")
> > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> >
> > I think the original commit message would make more sense here. This patch
> > doesn't fix 65db04053efe, it replaces it. The commit message now misses
> > the point, and the patch coalesces continuous apertures without explaining
> > the reason.
>
> Because the message is already in the git log so I didn't think that's
> necessary.

Hmm, not my decision to make, but the original commit got reverted.
The commit log associated with this patch should still reflect what
the patch does.

Guenter

> Will send another one with the original message along with this one.
>
> Kai-Heng
>
> > Guenter
> >
> > > ---
> > >    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);