Message-ID: <5331e942ff28bb191d62bb403b03ceb7d750856c.camel@sipsolutions.net>
Subject: Re: [PATCH v2] PCI: Reinstate "PCI: Coalesce host bridge contiguous
 apertures"
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Kai-Heng Feng <kai.heng.feng@canonical.com>, bhelgaas@google.com
Cc:     Guenter Roeck <linux@roeck-us.net>,
        "open list:PCI SUBSYSTEM" <linux-pci@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Date:   Wed, 17 Nov 2021 15:12:17 +0100
In-Reply-To: <20210713125007.1260304-1-kai.heng.feng@canonical.com>
References: <20210713075726.1232938-1-kai.heng.feng@canonical.com>
         <20210713125007.1260304-1-kai.heng.feng@canonical.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.1 (3.42.1-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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