Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210713075726.1232938-1-kai.heng.feng@canonical.com> <f18e36f7-cb18-1a4a-e7e8-4fbb253581ae@roeck-us.net>
In-Reply-To: <f18e36f7-cb18-1a4a-e7e8-4fbb253581ae@roeck-us.net>
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
Date:   Tue, 13 Jul 2021 16:49:23 +0800
Message-ID: <CAAd53p50502g1_7Db4xzUXrzcofmtBDO=+-HLqfs+J7NvVhB+A@mail.gmail.com>
Subject: Re: [PATCH 1/1] PCI: Coalesce host bridge contiguous apertures
 without sorting
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        "open list:PCI SUBSYSTEM" <linux-pci@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jul 13, 2021 at 4:45 PM Guenter Roeck <linux@roeck-us.net> 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@roeck-us.net>
> > Fixes: 65db04053efe ("PCI: Coalesce host bridge contiguous apertures")
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>
> 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.
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);
> >
>