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;
Sender: Guenter Roeck <groeck7@gmail.com>
Subject: Re: [PATCH 1/1] PCI: Coalesce host bridge contiguous apertures
 without sorting
To:     Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc:     Bjorn Helgaas <bhelgaas@google.com>,
        "open list:PCI SUBSYSTEM" <linux-pci@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
References: <20210713075726.1232938-1-kai.heng.feng@canonical.com>
 <f18e36f7-cb18-1a4a-e7e8-4fbb253581ae@roeck-us.net>
 <CAAd53p50502g1_7Db4xzUXrzcofmtBDO=+-HLqfs+J7NvVhB+A@mail.gmail.com>
From:   Guenter Roeck <linux@roeck-us.net>
Message-ID: <346ae622-de15-b81d-c129-24c9e77b59d4@roeck-us.net>
Date:   Tue, 13 Jul 2021 02:06:18 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <CAAd53p50502g1_7Db4xzUXrzcofmtBDO=+-HLqfs+J7NvVhB+A@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/13/21 1:49 AM, Kai-Heng Feng wrote:
> 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.

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);
>>>
>>