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.

Link: https://lore.kernel.org/linux-pci/CAMuHMdUjhq2ZLFyMYv9KYRW8brsvXMH5xb5NW8shsHJmx7w2QQ@mail.gmail.com/
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/probe.c          |   8 +--
 include/linux/resource_ext.h |   3 +
 kernel/resource.c            | 135 ++++++++++++++++++++++++++++++++++-
 3 files changed, 140 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 04523dea7d96..053bffc17146 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1061,10 +1061,10 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 		if (prev_res->flags != res->flags || prev_offset != offset)
 			continue;
 
-		if (prev_res->end + 1 == res->start) {
-			res->start = prev_res->start;
-			prev_res->flags = prev_res->start = prev_res->end = 0;
-		}
+		if (prev_res->end + 1 != res->start)
+			continue;
+
+		resource_list_merge_entries(prev, window);
 	}
 
 	/* Add initial resources to the bus */
diff --git a/include/linux/resource_ext.h b/include/linux/resource_ext.h
index ff0339df56af..4d6f2a926e6d 100644
--- a/include/linux/resource_ext.h
+++ b/include/linux/resource_ext.h
@@ -60,6 +60,9 @@ resource_list_destroy_entry(struct resource_entry *entry)
 	resource_list_free_entry(entry);
 }
 
+struct resource_entry *resource_list_merge_entries(struct resource_entry *entry,
+						   struct resource_entry *next);
+
 #define resource_list_for_each_entry(entry, list)	\
 	list_for_each_entry((entry), (list), node)
 
diff --git a/kernel/resource.c b/kernel/resource.c
index f9bb5481501a..c6e1872abb78 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -10,6 +10,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/cleanup.h>
 #include <linux/export.h>
 #include <linux/errno.h>
 #include <linux/ioport.h>
@@ -206,6 +207,13 @@ static struct resource * __request_resource(struct resource *root, struct resour
 	}
 }
 
+static void resource_clear_tree_links(struct resource *res)
+{
+	res->parent = NULL;
+	res->child = NULL;
+	res->sibling = NULL;
+}
+
 static int __release_resource(struct resource *old, bool release_child)
 {
 	struct resource *tmp, **p, *chd;
@@ -265,6 +273,101 @@ void release_child_resources(struct resource *r)
 	write_unlock(&resource_lock);
 }
 
+/**
+ * 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.
+ *
+ * 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;
+}
+
+static int resource_coalesce(struct resource *res, struct resource *next_res,
+			     struct resource *new_res)
+{
+	struct resource *parent, *tmp;
+	struct resource **p, **c;
+
+	if (!resource_mergeable(res, next_res))
+		return -EINVAL;
+
+	guard(write_lock)(&resource_lock);
+	parent = res->parent;
+
+	if (parent != next_res->parent)
+		return -EINVAL;
+
+	if (parent && res->sibling != next_res)
+		return -EINVAL;
+
+	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;
+
+	if (!parent)
+		return 0;
+
+	/* 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! */
+		if (WARN_ON_ONCE(!tmp))
+			return -EINVAL;
+
+		p = &tmp->sibling;
+	}
+
+	/* 1) set the parent */
+	new_res->parent = parent;
+
+	/* 2) inject into parent's child/sibling chain */
+	*p = new_res;
+	new_res->sibling = next_res->sibling;
+
+	/* 3) move children over and switch them to the new parent. */
+	c = &new_res->child;
+	*c = res->child;
+	while (*c) {
+		tmp = *c;
+		tmp->parent = new_res;
+		c = &tmp->sibling;
+	}
+	*c = next_res->child;
+	while (*c) {
+		tmp = *c;
+		tmp->parent = new_res;
+		c = &tmp->sibling;
+	}
+
+	resource_clear_tree_links(res);
+	resource_clear_tree_links(next_res);
+
+	return 0;
+}
+
 /**
  * request_resource_conflict - request and reserve an I/O or memory resource
  * @root: root resource descriptor
@@ -1503,8 +1606,7 @@ 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;
 }
 
@@ -1860,6 +1962,35 @@ void resource_list_free(struct list_head *head)
 }
 EXPORT_SYMBOL(resource_list_free);
 
+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))
+		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);
+}
+
 #ifdef CONFIG_GET_FREE_REGION
 #define GFR_DESCENDING		(1UL << 0)
 #define GFR_REQUEST_REGION	(1UL << 1)
-- 
2.39.5

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.

>  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.

> + * 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;

> +}

...

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

-- 
With Best Regards,
Andy Shevchenko

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

On Mon, Oct 20, 2025 at 08:21:50PM +0300, Ilpo Järvinen wrote:
> On Wed, 15 Oct 2025, Andy Shevchenko wrote:
> > On Fri, Oct 10, 2025 at 05:42:30PM +0300, Ilpo Järvinen wrote:

...

> > > +/**
> > > + * 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.

On the second thought I think comparing by the content is quite a behavioural
change here. Perhaps we may start without doing that first? Theoretically it
might be the case when the content of names is different, but resources are
the same. The case when name is the same (by content, but pointers) with the
idea of having different resources sounds to me quite an awkward case. TL;
DR: What are the cases that we have in practice now?

-- 
With Best Regards,
Andy Shevchenko

On Mon, 20 Oct 2025, Andy Shevchenko wrote:

> On Mon, Oct 20, 2025 at 08:21:50PM +0300, Ilpo Järvinen wrote:
> > On Wed, 15 Oct 2025, Andy Shevchenko wrote:
> > > On Fri, Oct 10, 2025 at 05:42:30PM +0300, Ilpo Järvinen wrote:
> 
> ...
> 
> > > > +/**
> > > > + * 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.
> 
> On the second thought I think comparing by the content is quite a behavioural
> change here.

Compared to what?

This code was previously only used for merging contiguous "System RAM" 
resources (AFAICT, I don't have way to check what the names in all those
resources truly were but in any case, the check was even stricter earlier, 
comparing pointer equality only so definitely the names were not different 
before this).

> Perhaps we may start without doing that first? Theoretically it
> might be the case when the content of names is different, but resources are
> the same.

Resources are NOT same, they're two contiguous memory regions and may 
originate from different source, and thus have different names.

Not caring about the names will lose one of them from /proc/iomem.

> The case when name is the same (by content, but pointers) with the
> idea of having different resources sounds to me quite an awkward case. TL;
> DR: What are the cases that we have in practice now?

In the original thread [1], PCI side resource coalescing did break the 
resources by merging without caring what the resource internals were. That 
problem was found after trying to fix another problem, thus it might not 
happen in practice except after fixing the other problem with root bus 
resources.

In the common case when merging PCI root bus resources, the resources 
typically have the same name - this happens all the time (e.g. io port 
ranges are split to many small ranges which form a contiguous region 
when coalesced). But that's not always the case, why do you think these 
two names should be merged losing some information:

     ee080000-ee08ffff : pci@ee090000
       ...
     ee090000-ee090bff : ee090000.pci pci@ee090000

?

(Also, the careless change in the underlying resource by the code this 
series tries to fix would have likely broken also devres release of the 
mangled resource, which admittedly, is not related to name at all).

[1] https://lore.kernel.org/linux-pci/CAMuHMdVgCHU80mRm1Vwo6GFgNAtQcf50yHBz_oAk4TrtjcMpYg@mail.gmail.com/


-- 
 i.

On Mon, Oct 20, 2025 at 09:15:08PM +0300, Ilpo Järvinen wrote:
> On Mon, 20 Oct 2025, Andy Shevchenko wrote:
> > On Mon, Oct 20, 2025 at 08:21:50PM +0300, Ilpo Järvinen wrote:
> > > On Wed, 15 Oct 2025, Andy Shevchenko wrote:
> > > > On Fri, Oct 10, 2025 at 05:42:30PM +0300, Ilpo Järvinen wrote:

...

> > > > > +/**
> > > > > + * 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.
> > 
> > On the second thought I think comparing by the content is quite a behavioural
> > change here.
> 
> Compared to what?
> 
> This code was previously only used for merging contiguous "System RAM" 
> resources (AFAICT, I don't have way to check what the names in all those
> resources truly were but in any case, the check was even stricter earlier, 
> comparing pointer equality only so definitely the names were not different 
> before this).
> 
> > Perhaps we may start without doing that first? Theoretically it
> > might be the case when the content of names is different, but resources are
> > the same.
> 
> Resources are NOT same, they're two contiguous memory regions and may 
> originate from different source, and thus have different names.
> 
> Not caring about the names will lose one of them from /proc/iomem.
> 
> > The case when name is the same (by content, but pointers) with the
> > idea of having different resources sounds to me quite an awkward case. TL;
> > DR: What are the cases that we have in practice now?
> 
> In the original thread [1], PCI side resource coalescing did break the 
> resources by merging without caring what the resource internals were. That 
> problem was found after trying to fix another problem, thus it might not 
> happen in practice except after fixing the other problem with root bus 
> resources.
> 
> In the common case when merging PCI root bus resources, the resources 
> typically have the same name - this happens all the time (e.g. io port 
> ranges are split to many small ranges which form a contiguous region 
> when coalesced). But that's not always the case, why do you think these 
> two names should be merged losing some information:
> 
>      ee080000-ee08ffff : pci@ee090000
>        ...
>      ee090000-ee090bff : ee090000.pci pci@ee090000
> 
> ?

I don't think it's a good idea (after reading the nice elaboration from you).
It seems I misunderstood the use case(s). That's why I asked for some elaboration
about the (new?) requirement to test the content of the names and not only pointer
equivalency.

> (Also, the careless change in the underlying resource by the code this 
> series tries to fix would have likely broken also devres release of the 
> mangled resource, which admittedly, is not related to name at all).
> 
> [1] https://lore.kernel.org/linux-pci/CAMuHMdVgCHU80mRm1Vwo6GFgNAtQcf50yHBz_oAk4TrtjcMpYg@mail.gmail.com/


-- 
With Best Regards,
Andy Shevchenko