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

[Simon Horman]

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
---
net: ipa: Grab IMEM slice base/size from DTS

This commit reads the IMEM slice base address and size from a device
tree "sram" phandle, falling back to the existing struct fields when
the DT property is absent.

> diff --git a/drivers/net/ipa/ipa_mem.c b/drivers/net/ipa/ipa_mem.c
> index 835a3c9c1fd4..583aea625709 100644
> --- a/drivers/net/ipa/ipa_mem.c
> +++ b/drivers/net/ipa/ipa_mem.c

[ ... ]

> @@ -656,7 +659,23 @@ 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);

Here, res still points into the platform device's resource array from
the earlier platform_get_resource_byname() call.  Won't
of_address_to_resource() overwrite the platform device's "ipa-shared"
resource entry with the SRAM resource data?

The needed values (mem_addr, mem_size) were already copied out, but
this silently corrupts the platform device's resource table.  Would it
be better to use a local struct resource for this call?

Simon says: I think this is a false positive as I don't see how reusing res
here results in resource table corruption.

> +		of_node_put(ipa_slice_np);
> +		if (ret)
> +			return ret;
                        ^^^^^^^^^^

At this point ipa->mem_virt holds a valid memremap() pointer.  Does
this direct return leak the mapping?  Every other error path after the
memremap() in this function uses goto err_unmap, which calls
memunmap(ipa->mem_virt).  Should this be goto err_unmap instead?

Simon says: This seems to me to be an actual problem.

> +
> +		imem_base = res->start;
> +		imem_size = resource_size(res);

[ ... ]

> +	ret = ipa_imem_init(ipa, imem_base, imem_size);
>  	if (ret)
>  		goto err_unmap;