Re: [PATCH v3 2/4] remoteproc: imx_rproc: add support for Cortex-A Core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] The CPU power-off polling loop in imx_rproc_psci_stop() silently ignores timeouts, leading to a critical Use-After-Free.
- [High] The error recovery path in imx_rproc_psci_start() improperly onlines CPUs that were administratively offline.
- [High] The IMX_SIP_CPU_OFF SMC call is passed a Linux logical CPU ID instead of the hardware MPIDR.
- [Medium] The driver references internal kernel symbols (psci_ops and cpu_logical_map) that are not exported, causing module build breakage.
- [Low] bitmap_copy is erroneously called with a byte count instead of a bit count, logically truncating the CPU mask.
--

commit 6acc0c2627860bf5f4e779a8968c861a2f039bd5
Author: Jiafei Pan <Jiafei.Pan@nxp.com>

remoteproc: imx_rproc: add support for Cortex-A Core

This commit adds Cortex-A remoteproc support. It uses PSCI and SIP SMC calls
to manage turning the Cortex-A cores on and off.

> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 0dd80e688b0ea..8a3de27c96b78 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
[ ... ]
> @@ -381,6 +392,45 @@ static int imx_rproc_sm_lmm_start(struct rproc *rproc)
>  	return 0;
>  }
>  
> +static int imx_rproc_psci_start(struct rproc *rproc)
> +{
[ ... ]
> +	cpu = cpumask_first(&priv->cpus);
> +	ret = psci_ops.cpu_on(cpu_logical_map(cpu), rproc->bootaddr);

Are the psci_ops and cpu_logical_map symbols exported for loadable modules?
If this driver is compiled as a module, will it cause linker errors due to
undefined symbols?

[ ... ]
> +err:
> +	for_each_cpu(cpu, &priv->cpus) {
> +		if (!cpu_online(cpu) && add_cpu(cpu) == 0)
> +			cpumask_clear_cpu(cpu, &priv->offlined_cpus);
> +	}
> +
> +	return ret;
> +}

Could this rollback path inadvertently bring up CPUs that were
administratively offline before the driver started?

In the stop path, it checks cpumask_test_cpu(cpu, &priv->offlined_cpus) to
verify ownership. Should this rollback path use the same check to avoid
overriding system power policies?

[ ... ]
> @@ -456,6 +506,50 @@ static int imx_rproc_sm_lmm_stop(struct rproc *rproc)
>  	return scmi_imx_lmm_operation(dcfg->lmid, SCMI_IMX_LMM_SHUTDOWN, 0);
>  }
>  
> +static int imx_rproc_psci_stop(struct rproc *rproc)
> +{
[ ... ]
> +	for_each_cpu(cpu, &priv->cpus) {
> +		/* Check CPU status */
> +		err = psci_ops.affinity_info(cpu_logical_map(cpu), 0);
> +		if (err == PSCI_0_2_AFFINITY_LEVEL_OFF)
> +			continue;
> +
> +		/* Bring CPU to be off */
> +		arm_smccc_smc(IMX_SIP_CPU_OFF, cpu, 0,
> +			0, 0, 0, 0, 0, &res);

Does the firmware expect a logical CPU index or a hardware MPIDR here?

The adjacent psci_ops.affinity_info() call uses cpu_logical_map(cpu) to
translate it. Would passing the logical index directly cause the firmware to
shut down the wrong physical core?

> +		start = jiffies;
> +		end = start + msecs_to_jiffies(100);
> +		do {
> +			err = psci_ops.affinity_info(cpu_logical_map(cpu), 0);
> +			if (err == PSCI_0_2_AFFINITY_LEVEL_OFF) {
> +				pr_info("CPU%d is killed (polled %d ms)\n", cpu,
> +					jiffies_to_msecs(jiffies - start));
> +				break;
> +			}
> +
> +			usleep_range(100, 1000);
> +		} while (time_before(jiffies, end));
> +	}
> +
> +	/* Return back freed CPU Core to Linux kernel */
> +	for_each_cpu(cpu, &priv->cpus) {
[ ... ]
> +	}
> +
> +	return 0;
> +}

If the polling loop times out after 100ms without reaching
PSCI_0_2_AFFINITY_LEVEL_OFF, does this function still return 0?

If it returns success while the remote core is still executing, will the
remoteproc core proceed to unmap the IOMMU and free the carveouts, leading to
a use-after-free when the still-running core accesses that memory?

Should it return an error like -ETIMEDOUT instead?

[ ... ]
> @@ -1274,6 +1383,17 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed on imx_rproc_addr_init\n");
>  
> +	ret = of_property_read_u32(dev->of_node, "fsl,cpus-bits", &cpus);
> +	if (ret) {
> +		cpumask_clear(&priv->cpus);
> +	} else {
> +		cpus_bits = cpus;
> +		bitmap_copy(cpumask_bits(&priv->cpus), &cpus_bits,
> +				min((unsigned int)nr_cpumask_bits,
> +				    (unsigned int)sizeof(unsigned long)));

Is the third argument to bitmap_copy intended to be a bit count rather than
a byte count?

Passing sizeof(unsigned long) requests a copy of only 4 or 8 bits. Should
this use BITS_PER_LONG to prevent logical truncation?

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