Re: [PATCH v3 2/3] PCI: of: introduce of_pci_build_prop_ranges()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 24 Sep 2026, Alex Elder wrote:

> Move the bulk of what happens inside of_pci_prop_ranges() into
> a helper function that builds up the value to be passed to
> of_changeset_add_prop_u32_array().
> 
> This helper will be used in the next commit to build the ranges
> property for a statically-defined PCI endpoint devicetree node
> when it has a "pci-ep-bus" sub-node.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
>  drivers/pci/of_property.c | 129 +++++++++++++++++++++++++-------------
>  1 file changed, 86 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
> index a6c0cca986bdc..9f30b3c09a730 100644
> --- a/drivers/pci/of_property.c
> +++ b/drivers/pci/of_property.c
> @@ -102,57 +102,100 @@ static int of_pci_prop_bus_range(struct pci_dev *pdev,
>  					       ARRAY_SIZE(bus_range));
>  }
>  
> +/*
> + * Build a "ranges" property value that defines the mapping between
> + * child and parent PCI address space for each of the given PCI
> + * device's resources.
> + *
> + * Returns a dynamically allocated array of u32 devicetree cells, or
> + * a null pointer if allocation fails.  The cell array is built as a
> + * of_pci_prop_ranges structure consisting of 8 32-bit cells in host
> + * byte order.  It is suitable for use as the value of a PCI device
> + * node "ranges" property passed to of_changeset_add_prop_u32_array().
> + * The total number of cells in the array is returned in *count.
> + *
> + * Caller is responsible for ensuring the returned pointer gets freed.
> + */
> +static u32 *of_pci_build_prop_ranges(struct pci_dev *pdev, u32 *count)
> +{
> +	bool bridge_device = pci_is_bridge(pdev);
> +	struct of_pci_range_entry *entries;
> +	struct of_pci_range_entry *ep;
> +	u32 resource_count = 0;
> +	struct resource *res;
> +	u32 first;
> +	u32 num;
> +	u32 i;
> +
> +	if (bridge_device) {
> +		first = PCI_BRIDGE_RESOURCES;
> +		num = PCI_BRIDGE_RESOURCE_NUM;
> +	} else {
> +		first = PCI_STD_RESOURCES;
> +		num = PCI_STD_NUM_BARS;
> +	}
> +
> +	/* First count how many resources will get a range property */
> +	res = &pdev->resource[first];

pci_resource_n()

> +	for (i = 0; i < num; i++, res++)
> +		if (resource_size(res) && !of_pci_get_addr_flags(res, NULL))

+ ioport.h

(I know you're mostly moving code around in this change so these missing 
includes are a pre-existing problem but it would be nice to finally add 
them.)

> +			resource_count++;
> +
> +	entries = kzalloc_objs(*entries, resource_count);

include

> +	if (!entries)
> +		return NULL;
> +
> +	ep = entries;
> +	res = &pdev->resource[first];

pci_resource_n()

> +	for (i = 0; i < num; i++, res++) {
> +		u64 size = resource_size(res);
> +		u32 flags;
> +
> +		if (!size || of_pci_get_addr_flags(res, &flags))
> +			continue;
> +
> +		/* Record the size in the range entry */

> +		ep->size[0] = upper_32_bits(size);
> +		ep->size[1] = lower_32_bits(size);

include.

> +
> +		/* Record the parent bus address from the resource */
> +		of_pci_set_address(pdev, ep->parent_addr,
> +				   pci_bus_address(pdev, first + i),
> +				   flags, false);
> +
> +		/*
> +		 * For a bridge device, the child address matches the
> +		 * parent address (including its flags cell).  For an
> +		 * endpoint device, the (flags) cell contains the BAR
> +		 * number, and the two address cells are zero.
> +		 */
> +		if (bridge_device)
> +			memcpy(ep->child_addr, ep->parent_addr,
> +			       sizeof(ep->child_addr));
> +		else
> +			ep->child_addr[0] = i;
> +
> +		ep++;
> +	}
> +	*count = resource_count * sizeof(*ep) / sizeof(u32);
> +
> +	return (u32 *)entries;
> +}
> +
>  static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs,
>  			      struct device_node *np)
>  {
>  	struct of_pci_range_entry *rp;
> -	struct resource *res;
> -	int i, j, ret;
> -	u32 flags, num;
> -	u64 val64;
> +	u32 *value;
> +	u32 count;
> +	int ret;
>  
> -	if (pci_is_bridge(pdev)) {
> -		num = PCI_BRIDGE_RESOURCE_NUM;
> -		res = &pdev->resource[PCI_BRIDGE_RESOURCES];
> -	} else {
> -		num = PCI_STD_NUM_BARS;
> -		res = &pdev->resource[PCI_STD_RESOURCES];
> -	}
> -
> -	rp = kzalloc_objs(*rp, num);
> -	if (!rp)
> +	value = of_pci_build_prop_ranges(pdev, &count);
> +	if (!value)
>  		return -ENOMEM;
>  
> -	for (i = 0, j = 0; j < num; j++) {
> -		if (!resource_size(&res[j]))
> -			continue;
> +	ret = of_changeset_add_prop_u32_array(ocs, np, "ranges", value, count);
>  
> -		if (of_pci_get_addr_flags(&res[j], &flags))
> -			continue;
> -
> -		val64 = pci_bus_address(pdev, &res[j] - pdev->resource);
> -		of_pci_set_address(pdev, rp[i].parent_addr, val64, flags,
> -				   false);
> -		if (pci_is_bridge(pdev)) {
> -			memcpy(rp[i].child_addr, rp[i].parent_addr,
> -			       sizeof(rp[i].child_addr));
> -		} else {
> -			/*
> -			 * For endpoint device, the lower 64-bits of child
> -			 * address is always zero.
> -			 */
> -			rp[i].child_addr[0] = j;
> -		}
> -
> -		val64 = resource_size(&res[j]);
> -		rp[i].size[0] = upper_32_bits(val64);
> -		rp[i].size[1] = lower_32_bits(val64);
> -
> -		i++;
> -	}
> -
> -	ret = of_changeset_add_prop_u32_array(ocs, np, "ranges", (u32 *)rp,
> -					      i * sizeof(*rp) / sizeof(u32));
>  	kfree(rp);

Isn't totally rp unused now so this frees with an uninitialized value?

>  
>  	return ret;
> 

-- 
 i.