Re: [PATCH v3 3/4] remoteproc: imx_rproc: add autoboot support for A-core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unexported kernel symbols `psci_ops` and `cpu_logical_map()` are referenced, causing link errors when compiled as a module.
- [High] Unconditional invocation of `psci_ops.affinity_info` can lead to a NULL pointer dereference.
- [High] Negative error codes and pending states from `psci_ops.affinity_info()` are incorrectly treated as 'ON' state.
- [High] State detection logic is flawed for multi-core configurations, potentially booting an already running core.
- [High] Empty CPU mask logic error leads to incorrect auto-attaching to non-existent cores.
--

commit af57d4202637d4c4549d5d1bd6d7af8d8a4bbb7e
Author: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

remoteproc: imx_rproc: add autoboot support for A-core

This commit adds autoboot support for Cortex-A Core remoteproc by
implementing PSCI state detection to manage CPU cores. It evaluates CPU
affinity and online status to set the remoteproc state accordingly.

> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 8a3de27c96b78..e8d239bef5c95 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -1286,7 +1286,24 @@ static int imx_rproc_sm_detect_mode(struct rproc *rproc)
>  
>  static int imx_rproc_psci_detect_mode(struct rproc *rproc)
>  {
> -	rproc->state = RPROC_OFFLINE;
> +	struct imx_rproc *priv = rproc->priv;
> +	unsigned int cpu;
> +	int cpu_aff;
> +
> +	rproc->state = RPROC_DETACHED;
> +	for_each_cpu(cpu, &priv->cpus) {
> +		cpu_aff = psci_ops.affinity_info(cpu_logical_map(cpu), 0);

Because CONFIG_IMX_REMOTEPROC can be built as a module, will referencing
psci_ops and cpu_logical_map() cause undefined reference errors during
modpost? Neither symbol appears to be exported to loadable modules.

Can psci_ops.affinity_info be NULL if the system runs an older PSCI version
like v0.1? Should we verify if psci_ops.affinity_info is populated before
dereferencing it to avoid a panic?

> +		if (cpu_aff == PSCI_0_2_AFFINITY_LEVEL_OFF) {

The affinity_info callback can return negative values for errors, such as
PSCI_RET_NOT_SUPPORTED, or 2 for PENDING. Does this logic safely handle
those cases, or will it bypass this check and inadvertently leave the state
as RPROC_DETACHED if the CPU is not online in Linux?

> +			rproc->state = RPROC_OFFLINE;
> +			break;
> +		}
> +
> +		/* in psci on state but running Linux */
> +		if (cpu_online(cpu)) {
> +			rproc->state = RPROC_OFFLINE;
> +			break;
> +		}

If there is a multi-core cluster where CPU 0 is ON and running the remote
OS, but CPU 1 is OFF, this loop will evaluate CPU 1, set the state to
RPROC_OFFLINE, and break. Will this cause the remoteproc framework to
incorrectly attempt to boot the primary core while it is already actively
running?

> +	}
>  
>  	return 0;
>  }

[ ... ]

> @@ -1391,7 +1408,6 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  		bitmap_copy(cpumask_bits(&priv->cpus), &cpus_bits,
>  				min((unsigned int)nr_cpumask_bits,
>  				    (unsigned int)sizeof(unsigned long)));
> -		rproc->auto_boot = false;
>  	}

If the device tree property fsl,cpus-bits is missing because the bindings
specify fsl,cpus-mask instead, the cpus mask will be cleared. With an empty
mask, the detection loop is bypassed, leaving the state as RPROC_DETACHED.
Will removing rproc->auto_boot = false here cause the framework to attempt
an attach on a remoteproc instance with no assigned CPUs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511023928.39640-1-Jiafei.Pan@nxp.com?part=3