Message-ID: <8401388b-2957-0853-d80b-4479e02c47f0@linux.intel.com>
Date: Mon, 20 Oct 2025 20:21:50 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
cc: linux-pci@...r.kernel.org, Bjorn Helgaas <bhelgaas@...gle.com>, 
    Geert Uytterhoeven <geert@...ux-m68k.org>, 
    Kai-Heng Feng <kaihengf@...dia.com>, Rob Herring <robh@...nel.org>, 
    LKML <linux-kernel@...r.kernel.org>, 
    Krzysztof Wilczyński <kw@...ux.com>, 
    Linux-Renesas <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH 2/3] PCI: Do not coalesce host bridge resource structs
 in place

On Wed, 15 Oct 2025, Andy Shevchenko wrote:

> On Fri, Oct 10, 2025 at 05:42:30PM +0300, Ilpo Järvinen wrote:
> > The resource coalescing for host bridge windows in
> > pci_register_host_bridge() can alter the underlying struct resource
> > which is inherently dangerous as this code has no knowledge of who else
> > holds a pointer the same resource.
> > 
> > Merge the struct resource inside a newly added
> > resource_list_merge_entries() which uses the internal __res member of
> > the struct resource_entry to store the merged resource, thus preserving
> > the original resource structs.
> 
> ...
> 
> > +static void resource_clear_tree_links(struct resource *res)
> > +{
> > +	res->parent = NULL;
> > +	res->child = NULL;
> > +	res->sibling = NULL;
> > +}
> 
> Not sure if this is the best location to inject a new helper to in the code
> (I mean the position in the file). But I leave it to you just to give another
> look in case something more suitable can be found.

This placement was far from random. I'd also want to start clearing links 
of any childs  resources (direct or grand) on a release of a resource 
(when called with  __release_resource(..., release_child=true). It's what 
lead to placing this helper here right above __release_resource().

Currently, released child resources have no way of knowing they've been 
removed from the resource tree as the resource tree links are all left in 
place (only old->parent is set to NULL by __release_resource(), strictly 
speaking even that wouldn't be necessary if we don't care for stale 
links).

My goal is to make res->parent invariant that unambiguously tells whether 
the resource is within the resource tree or not (sans the root "anchor" 
resources that are parentless).

(But as you could see, it's not part of this series.)

I initially tried to also change old->parent = NULL in 
__release_resource() to use this new helper but then realized there can be 
children too which will have stale links so to make all resource links 
coherent, a bigger change would have been needed so I left it to a later 
patch as this series was to fix PCI host bridge resource coalescing 
algorithm.

Clearing stale links from the children will come with potential 
performance penalty as the entire subtree have to be walked so it might 
result in discussion and perhaps some even opposing the idea. But I'd 
assume it to be small and likely not measurable in practice, and how 
much resource are removed from the resource tree anyway, not much I 
think except perhaps in some hotplug stress test.

I've not yet investigated how often there are unreleased children still 
remaining in first place when calling __release_resource(). It could be 
that the calling code has released those before calling release of the 
resource itself (making the performance impact nil in practice).

> >  static int __release_resource(struct resource *old, bool release_child)
> 
> ...
> 
> > +/**
> > + * resource_mergeable - Test if resources are contiguous and can be merged
> > + * @r1: first resource
> > + * @r2: second resource
> > + *
> > + * Tests @r1 is followed by @r2 contiguously and share the metadata.
> 
> This needs an additional explanation about name equivalence that's not only by
> pointers, but by a content.

Okay. The point was to check names are the same, the pointer check was 
just an optimization as these resources are expected to carry the same 
name even on the pointer level.

> > + * Return: %true if resources are mergeable non-destructively.
> > + */
> > +static bool resource_mergeable(struct resource *r1, struct resource *r2)
> > +{
> > +	if ((r1->flags != r2->flags) ||
> > +	    (r1->desc != r2->desc) ||
> > +	    (r1->parent != r2->parent) ||
> > +	    (r1->end + 1 != r2->start))
> > +		return false;
> 
> > +	if (r1->name == r2->name)
> > +		return true;
> > +
> > +	if (r1->name && r2->name && !strcmp(r1->name, r2->name))
> > +		return true;
> > +
> > +	return false;
> 
> Hmm... Can we keep the logic more straight as in returning false cases as soon
> as possible?
> 
> I think of something like this:
> 
> 	if (r1->name && r2->name)
> 		return strcmp(r1->name, r2->name) == 0;
> 
> 	return r1->name == r2->name;

But the point the order above was to avoid strcmp() when the pointer 
itself is same which I think is quite common case. I don't think strcmp() 
itself checks whether the pointer is the same.

Thanks for the review.

-- 
 i.

> > +}
> 
> ...
> 
> > +	new_res->start = res->start;
> > +	new_res->end = next_res->end;
> > +	new_res->name = res->name;
> > +	new_res->flags = res->flags;
> > +	new_res->desc = res->desc;
> 
> Hmm... IIRC I saw similar code lines a few times in the kernel in resource.c
> and might be elsewhere. Perhaps a time for another helper?
> 
> 
> ...
> 
> > +	/* prepare for step 2), find res & next_res from child/sibling chain. */
> > +	p = &parent->child;
> > +	while (1) {
> > +		tmp = *p;
> > +		if (tmp == res)
> > +			break;
> > +
> > +		/*  No res in child/sibling, the resource tree is corrupted! */
> 
> Extra space which is not needed.
> 
> > +		if (WARN_ON_ONCE(!tmp))
> > +			return -EINVAL;
> > +
> > +		p = &tmp->sibling;
> > +	}
> 
> ...
> 
> > static bool system_ram_resources_mergeable(struct resource *r1,
> >  					   struct resource *r2)
> >  {
> >  	/* We assume either r1 or r2 is IORESOURCE_SYSRAM_MERGEABLE. */
> > -	return r1->flags == r2->flags && r1->end + 1 == r2->start &&
> > -	       r1->name == r2->name && r1->desc == r2->desc &&
> > +	return resource_mergeable(r1, r2) &&
> >  	       !r1->child && !r2->child;
> 
> Now one line?
> 
> >  }
> 
> > +struct resource_entry *resource_list_merge_entries(struct resource_entry *entry,
> > +						   struct resource_entry *next)
> > +{
> > +	struct resource *res = entry->res, *next_res = next->res, *new_res;
> > +	int ret;
> > +
> > +	if ((entry->offset != next->offset) ||
> > +	    !resource_mergeable(res, next_res))
> 
> One line? (It's only 82 characters long)
> 
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	/* Use the internal __res to not mutate the input resources. */
> > +	struct resource_entry __free(kfree) *new = resource_list_create_entry(NULL, 0);
> > +	if (!new)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	new->offset = next->offset;
> > +	new_res = new->res;
> > +
> > +	ret = resource_coalesce(res, next_res, new_res);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	resource_list_add_tail(new, &entry->node);
> > +	resource_list_destroy_entry(entry);
> > +	resource_list_destroy_entry(next);
> > +
> > +	return no_free_ptr(new);
> > +}
> 
> 

Powered by blists - more mailing lists