Re: [RFC PATCH 2/2] PCI: Allow pci=hpreserve= to reserve bus numbers for a hotplug bridge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: hotplug_share calculation ignores buses used by normal bridges
- [Medium] pci: reserved bus numbers are lost if bridge is firmware-configured
--

commit 34e2d44bc1e7840b0e80a2d0ea3f891f8e316ab9
Author: Maciej Grochowski <maciej.grochowski@sony.com>

PCI: Allow pci=hpreserve= to reserve bus numbers for a hotplug bridge

When the kernel assigns bus numbers to the bridges on a bus,
pci_scan_child_bus_extend() divides the spare bus numbers equally
between the hotplug bridges. This commit adds a bus= key to
pci=hpreserve= to allow a reserved hotplug bridge to get a
requested number of buses, with others sharing the remainder.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 27008e2ea5afc..086b7a549c27e 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
[ ... ]
> @@ -3115,12 +3127,28 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  	 * buses between hotplug bridges.
>  	 */
>  	for_each_pci_bridge(dev, bus) {
> -		if (dev->is_hotplug_bridge)
> +		if (dev->is_hotplug_bridge) {
> +			unsigned int reserved = pci_hotplug_reserved_buses(dev);
> +
>  			hotplug_bridges++;
> -		else
> +			if (reserved) {
> +				reserved_bridges++;
> +				reserved_buses += reserved;
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Will these reserved bus numbers be lost if the bridge was already configured by
firmware?

When a user reserves buses for a bridge that was assigned a smaller number of
secondary buses by firmware, the full reservation is deducted from the shared
pool here.

However, if the bridge is already configured, it retains its original
allocation and the remaining reserved buses are never added to used_buses.

> +			}
> +		} else {
>  			normal_bridges++;
> +		}
>  	}
>  
> +	/*
> +	 * Hotplug bridges with a "pci=hpreserve=" bus reservation get the
> +	 * number of buses they asked for; the others share what is left.
> +	 */
> +	if (hotplug_bridges > reserved_bridges &&
> +	    available_buses > reserved_buses)
> +		hotplug_share = (available_buses - reserved_buses) /
> +				(hotplug_bridges - reserved_bridges);
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this calculation over-allocate buses to non-reserved hotplug bridges?

When the parent bus contains a mix of normal bridges and non-reserved hotplug
bridges, available_buses does not account for buses already taken by the
normal bridges.

This inflates the calculated hotplug_share.

> +
>  	/*
>  	 * Scan bridges that are already configured. We don't touch them
>  	 * unless they are misconfigured (which will be done in the second
[ ... ]
> @@ -3152,12 +3180,34 @@ static unsigned int pci_scan_child_bus_extend(struct pci_bus *bus,
>  			 */
>  			buses = available_buses;
>  		} else if (dev->is_hotplug_bridge) {
> +			unsigned int reserved = pci_hotplug_reserved_buses(dev);
> +
>  			/*
>  			 * Distribute the extra buses between hotplug
>  			 * bridges if any.
>  			 */
> -			buses = available_buses / hotplug_bridges;
> -			buses = min(buses, available_buses - used_buses + 1);
> +			if (!reserved) {
> +				buses = hotplug_share;
                                ^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
If a non-reserved bridge is processed before the reserved bridge, it consumes
this inflated hotplug_share. This can deplete the pool and leave the reserved
bridge with fewer buses than requested.

> +				buses = min(buses, available_buses - used_buses + 1);
> +			} else {
> +				/* used_buses already includes this bridge's bus */
> +				buses = available_buses + 1 > used_buses ?
> +					available_buses + 1 - used_buses : 0;
> +				buses = min(buses, reserved);
> +			}
> +
> +			/* Bridges configured in the first pass keep their buses */
> +			if (reserved && !dev->subordinate) {
                                        ^^^^^^^^^^^^^^^^

[Severity: Medium]
If the bridge is already firmware-configured, dev->subordinate evaluates to
true. This suppresses the warning about failing to fulfill the reservation,
masking the silent loss of the reserved bus numbers deducted from the pool
earlier.

> +				/* The secondary bus is allocated even for 0 */
> +				unsigned int granted = max(buses, 1U);
> +
> +				if (granted < reserved)
> +					pci_warn(dev, "only %u of %u reserved bus numbers available\n",
> +						 granted, reserved);
> +				else
> +					pci_info(dev, "reserving %u bus numbers\n",
> +						 granted);
> +			}
>  		}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002193111.51637-1-maciej.grochowski@sony.com?part=2