Re: [net-next,v5,3/3] net: ipa: Grab IMEM slice base/size from DTS

[Konrad Dybcio]

On 2/27/26 2:54 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---

[...]

>> @@ -656,7 +659,24 @@ int ipa_mem_init(struct ipa *ipa, struct platform_device *pdev,
>>  	ipa->mem_addr = res->start;
>>  	ipa->mem_size = resource_size(res);
>>
>> -	ret = ipa_imem_init(ipa, mem_data->imem_addr, mem_data->imem_size);
>> +	ipa_slice_np = of_parse_phandle(dev->of_node, "sram", 0);
>> +	if (ipa_slice_np) {
>> +		ret = of_address_to_resource(ipa_slice_np, 0, res);
>> +		of_node_put(ipa_slice_np);
>> +		if (ret)
>> +			goto err_unmap;
>> +
>> +		imem_base = res->start;
>> +		imem_size = resource_size(res);
>                      ^^^^
> 
> Does this code corrupt the platform device's resource array? The variable
> 'res' was obtained from platform_get_resource_byname() earlier in this
> function, which returns a pointer directly into pdev->resource[]:
> 
> drivers/base/platform.c:platform_get_resource_byname()
>     return platform_get_resource(dev, type, num);
>         => return &dev->resource[i];
> 
> Passing this pointer to of_address_to_resource() causes
> __of_address_to_resource() to memset the entire structure to zero and then
> overwrite all fields:
> 
> drivers/of/address.c:__of_address_to_resource()
>     memset(r, 0, sizeof(struct resource));
>     ...
>     r->flags = ...
>     r->name = ...
>     r->start = ...
>     r->end = ...
> 
> This permanently corrupts the platform device's "ipa-shared" resource entry.
> While ipa->mem_addr and ipa->mem_size were saved before the corruption, the
> platform device's resource array is shared kernel infrastructure.
> 
> After this corruption:
> - /proc/iomem and sysfs will report incorrect resource information
> - If the driver is unbound and rebound, platform_get_resource_byname(pdev,
>   IORESOURCE_MEM, "ipa-shared") will fail because the resource's name field
>   was overwritten with the sram DT node's full_name, causing probe to fail
>   with -ENODEV
> 
> Would it be better to use a separate local variable for the
> of_address_to_resource() call?

Yes, the computer proved useful once again - I tunnel-visioned-out the
prior use of 'res'.

Konrad