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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `rproc_elf_load_rsc_table_optional` macro hides unconditional function returns, making the trailing `return 0;` in all calling functions unreachable.
- [Medium] Undocumented change in error propagation for multiple drivers which previously ignored non-EINVAL errors.
--

commit 9602dbc4d243a49c62f52f899683404a0f547124
Author: Ben Levinsky <ben.levinsky@amd.com>

remoteproc: switch drivers to optional resource-table helper

This commit updates several remoteproc drivers to use the new shared
rproc_elf_load_rsc_table_optional() helper for handling optional resource
tables.

> Keep thin local parse_fw() wrappers in each driver so the helper only
> centralizes the return-value handling while each platform retains
> control over whether the missing-table case is logged and at
> what severity.

[Severity: Medium]
Is this description entirely accurate? Prior to this patch, drivers like
imx_dsp_rproc and rcar_rproc completely ignored all errors from
rproc_elf_load_rsc_table() and unconditionally returned 0.

The new rproc_elf_load_rsc_table_optional() macro propagates non--EINVAL
errors (such as -ENOMEM). While this propagation is likely a functional
improvement, it is a behavior change for these drivers rather than just
centralizing the return-value handling. Should this change in error
propagation be explicitly documented in the commit message?

> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index 2d9f14fbef1da..fd60c67ba8a9f 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -956,9 +956,8 @@ static int imx_dsp_rproc_elf_load_segments(struct rproc *rproc, const struct fir
>  
>  static int imx_dsp_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");
> -
> +	rproc_elf_load_rsc_table_optional(rproc, fw, dev_warn,
> +					  "no resource table found for this firmware\n");
>  	return 0;

[Severity: Medium]
Does this leave the trailing 'return 0;' unreachable?

Looking at the definition of rproc_elf_load_rsc_table_optional() in
drivers/remoteproc/remoteproc_internal.h:

#define rproc_elf_load_rsc_table_optional(rproc, fw, dev_func, fmt, ...) \
    ({                                                                   \
        int ret = rproc_elf_load_rsc_table(rproc, fw);                   \
        if (ret == -EINVAL) {                                            \
            dev_func(&rproc->dev, fmt, ##__VA_ARGS__);                   \
            return 0;                                                    \
        } else {                                                         \
            return ret;                                                  \
        }                                                                \
    })

Because the macro uses explicit return statements within the statement
expression, it exits the enclosing function entirely. This hides the control
flow and renders the trailing 'return 0;' dead code in every driver modified
by this patch.

[ ... ]

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