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

[Alex Elder <elder@riscstar.com>]

On 10/9/26 5:43 AM, Ilpo Järvinen wrote:
> 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>

Thanks for your review.  As I comment below, I'll add the
#includes you mention, to ensure the symbols used aren't
declared by accident.

You commented on patch 3/3 too, but I'll respond here:
you suggested adding an include, and I'll fix that in
version 4 of this series.
>> ---
>>   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()

That's better.  I'll use that.

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

I'll gladly add this and the others you mention below.  I like
to include whatever I need but I currently only do that by
manually scanning through (and fixing it when there's a warning).
> 
>> +			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?

Wow, you're right.  This line needs to go too.

					-Alex


>>   
>>   	return ret;
>>
>