Re: [RFC PATCH v2 1/2] PCI: Allow hpmmiosize and hpmmioprefsize for specific hotplug bridges

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCI: pbus_size_mem() adds children_add_size on top of scoped hp_size
--

commit 926f817ace771cfa772ae856e4b49cefab5e63e8
Author: Maciej Grochowski <maciej.grochowski@sony.com>

PCI: Allow hpmmiosize and hpmmioprefsize for specific hotplug bridges

This commit extends the pci=hpmmiosize= and pci=hpmmioprefsize= boot
parameters to accept per-bridge <size>@<pci_dev> entries, applying the
specified size as a required minimum window size and head alignment in
pbus_size_mem().

> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index ed16ef7c26fa7..f3992859d90f2 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
[ ... ]
> @@ -1349,6 +1351,27 @@ static void pbus_size_mem(struct pci_bus *bus, struct resource *b_res,
>  	win_align = pci_min_window_alignment(bus, b_res->flags);
>  	min_align = calculate_head_align(aligns, max_order);
>  	min_align = max(min_align, win_align);
> +
> +	/*
> +	 * A hotplug size given for this bridge is a required minimum, not
> +	 * optional space like the global size: aligned for the largest BAR
> +	 * that fits in it, so it is in place before the devices that need
> +	 * it appear below the bridge.
> +	 */
> +	if (bus->self &&
> +	    pci_hotplug_dev_size(bus->self, b_res->flags, &hp_size)) {
> +		if (hp_size) {
> +			size = max(size, hp_size);
> +			min_align = max_t(resource_size_t, min_align,
> +					  rounddown_pow_of_two(hp_size));
> +			pci_info(bus->self, "bridge window to %pR: reserving %s size %#lx align %#llx\n",
> +				 &bus->busn_res,
> +				 b_res->flags & IORESOURCE_PREFETCH ?
> +				 "prefetchable" : "non-prefetchable",
> +				 hp_size, (unsigned long long)min_align);
> +		}
> +	}
> +
>  	size0 = calculate_memsize(size, realloc_head ? 0 : add_size,
>  				  0, win_align);

[Severity: Medium]
In pbus_size_mem(), when realloc_head is non-NULL, required child resources
are summed into size while optional child resources (such as child hotplug
bridges, SR-IOV BARs, or disabled ROMs) are accumulated into
children_add_size via pbus_size_mem_optional().

Subsequently, size1 is calculated as:

	if (realloc_head && (add_size > 0 || children_add_size > 0)) {
		add_align = max(min_align, add_align);
		size1 = calculate_memsize(size, add_size, children_add_size,
					  win_align);
	}

where calculate_memsize() computes max(size + children_add_size, min_size).

Because size is overwritten with max(size, hp_size) before size1 is
calculated, doesn't calculate_memsize() add children_add_size on top of
hp_size rather than fitting those optional child resources within the
hp_size minimum window? When hp_size > size and children_add_size > 0, size1
becomes ALIGN(hp_size + children_add_size, win_align) even when
size + children_add_size <= hp_size, queueing an extra add_size = size1 -
size0 on realloc_head.

By contrast, when realloc_head is NULL (such as in earlier passes of
pci_assign_unassigned_root_bus_resources() with pci=realloc),
pbus_size_mem_optional() returns false and optional child resources are
summed into size before size = max(size, hp_size), so they fit within
hp_size.

Would it make sense to keep size as the sum of required child resources and
apply hp_size as a minimum to size0 and size1 instead (for example, passing
max(add_size, hp_size) as min_size when computing size1)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793724065.git.Maciej.Grochowski@sony.com?part=1