Re: [PATCH 4/4] remoteproc: switch drivers to optional resource-table helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Propagating errors from the `parse_fw` callback exposes a memory leak and list corruption in the remoteproc core's error handling.
- [Low] A `static inline` function is assigned directly to a function pointer across multiple compilation units.
- [Low] Implicit downgrade of the diagnostic log level for missing resource tables.
--

commit 4ae41a0f00b09b541655967f84eb5307e9228a6a
Author: Ben Levinsky <ben.levinsky@amd.com>

remoteproc: switch drivers to optional resource-table helper

This patch switches several remoteproc drivers to use the shared
rproc_elf_load_rsc_table_optional helper for parsing firmware.
This keeps missing resource tables non-fatal while allowing other
parsing errors to propagate to the caller.

> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 6249815b54d88..5509048f7e192 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
[ ... ]
> @@ -777,7 +766,7 @@ static const struct rproc_ops imx_rproc_ops = {
>  	.kick		= imx_rproc_kick,
>  	.da_to_va       = imx_rproc_da_to_va,
>  	.load		= rproc_elf_load_segments,
> -	.parse_fw	= imx_rproc_parse_fw,
> +	.parse_fw	= rproc_elf_load_rsc_table_optional,

Drivers like imx_rproc, stm32_rproc, and rcar_rproc allocate memory for
carveouts and coredump segments in their .prepare callbacks, appending
them to rproc->carveouts and rproc->dump_segments.

In rproc_fw_boot(), rproc_prepare_device() is called before
rproc_parse_fw(). Before this patch, the .parse_fw callbacks of these
drivers swallowed all errors, including -ENOMEM from
rproc_elf_load_rsc_table(), and unconditionally returned 0.

By switching to rproc_elf_load_rsc_table_optional(), failures like
-ENOMEM are now correctly propagated.

If rproc_parse_fw() returns an error, rproc_fw_boot() branches to the
unprepare_rproc label, which entirely skips the clean_up_resources block
and its call to rproc_resource_cleanup().

Because these drivers lack an .unprepare callback to manually free the
carveouts, the allocated memory is permanently leaked and the entries
remain on the rproc->carveouts list. Subsequent boot attempts will run
.prepare again, adding duplicate entries and leading to list corruption.

Does propagating errors from this callback expose a memory leak and list
corruption in the remoteproc core's error handling?

Also, rproc_elf_load_rsc_table_optional is defined as static inline in
a header file.

Taking the address of a static inline function to use as a callback
forces the compiler to generate a standalone, out-of-line copy of the
function in every referencing compilation unit.

Does assigning it directly to a function pointer across multiple drivers
cause unnecessary binary code duplication?

>  	.find_loaded_rsc_table = imx_rproc_elf_find_loaded_rsc_table,
>  	.get_loaded_rsc_table = imx_rproc_get_loaded_rsc_table,
>  	.sanity_check	= rproc_elf_sanity_check,
[ ... ]
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index 7ac8265b60ac7..007175dcd7afa 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -232,14 +232,6 @@ static int stm32_rproc_prepare(struct rproc *rproc)
>  	}
>  }
>  
> -static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> -{
> -	if (rproc_elf_load_rsc_table(rproc, fw))
> -		dev_warn(&rproc->dev, "no resource table found for this firmware\n");

This isn't a bug, but prior to this change, drivers such as stm32_rproc
emitted a dev_warn, and others emitted dev_info when no resource table
was found.

The shared helper implicitly replaces these with a dev_dbg log level.

While this suppresses warnings for an optional component, it is an
undocumented side-effect that silently degrades observability for users
relying on kernel logs to verify firmware IPC configuration.

Could this change implicitly downgrade the diagnostic log level for
missing resource tables?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511211841.284809-1-ben.levinsky@amd.com?part=4