Re: [PATCH 2/3] PCI: Do not coalesce host bridge resource structs in place

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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