Re: [PATCH 2/3] firmware: qcom: scm: pass qcom_scm through the exported API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] NULL pointer dereference in fastrpc_rpmsg_probe() when allocating memory for SDSP_DOMAIN_ID on systems without the qcom,vmids property.
- [High] SMP initialization is unconditionally disabled on Qualcomm ARM32 platforms.
- [Low] Unupdated caller of `qcom_scm_io_writel()` in the USB EUD driver.
- [Low] Unupdated caller of `qcom_scm_mem_protect_video_var()` in the Iris media driver.
--

commit 9df6c4c915fd9c1b7019af817a1a3e4285555806
Author: Albert Esteve <aesteve@redhat.com>

firmware: qcom: scm: pass qcom_scm through the exported API

This commit modifies the exported SCM calls to take an explicit struct qcom_scm
handle instead of using a global singleton. Subsystems and device drivers are
updated to recover the instance via dev_get_drvdata() or qcom_scm_get() before
calling SCM APIs.

> diff --git a/arch/arm/mach-qcom/platsmp.c b/arch/arm/mach-qcom/platsmp.c
> index eca2fe0f4314b..c4e7aab77bc99 100644
> --- a/arch/arm/mach-qcom/platsmp.c
> +++ b/arch/arm/mach-qcom/platsmp.c
> @@ -355,9 +355,10 @@ static int kpssv2_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  
>  static void __init qcom_smp_prepare_cpus(unsigned int max_cpus)
>  {
> +	struct qcom_scm *scm = qcom_scm_get();
>  	int cpu;
>  
> -	if (qcom_scm_set_cold_boot_addr(secondary_startup_arm)) {
> +	if (!scm || qcom_scm_set_cold_boot_addr(scm, secondary_startup_arm)) {

[Severity: High]
Does this code unintentionally disable SMP on all Qualcomm ARM32 devices?

Because qcom_smp_prepare_cpus() is called early in the boot process (from 
smp_prepare_cpus()), before do_initcalls() probes the qcom_scm platform driver,
qcom_scm_get() will unconditionally return NULL here.

This causes the (!scm) condition to short-circuit to true, skipping the SCM
call and executing the error path which clears the cpu_present mask for all
secondary CPUs.

[ ... ]
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 90fd669636ec1..e8ff9d4b03957 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
[ ... ]
> @@ -2569,13 +2574,17 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  				"qcom,vmids", &vmids[0], 0, FASTRPC_MAX_VMIDS);
>  	if (vmcount < 0)
>  		vmcount = 0;
> -	else if (!qcom_scm_is_available())
> -		return -EPROBE_DEFER;
> +	else {
> +		scm = qcom_scm_get();
> +		if (!scm)
> +			return -EPROBE_DEFER;
> +	}
>  
>  	data = kzalloc_obj(*data);
>  	if (!data)
>  		return -ENOMEM;
>  
> +	data->scm = scm;
>  	if (vmcount) {
>  		data->vmcount = vmcount;
>  		for (i = 0; i < data->vmcount; i++) {
> @@ -2592,7 +2601,7 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev)
>  		if (!err) {
>  			src_perms = BIT(QCOM_SCM_VMID_HLOS);
>  
> -			err = qcom_scm_assign_mem(res.start, resource_size(&res), &src_perms,
> +			err = qcom_scm_assign_mem(scm, res.start, resource_size(&res), &src_perms,

[Severity: High]
Can this result in a NULL pointer dereference in qcom_scm_assign_mem() when
allocating TZ memory?

If the "qcom,vmids" DT property is missing, vmcount defaults to 0 and the
"else" block above is skipped, leaving the scm pointer as NULL. Later in
fastrpc_rpmsg_probe(), for the SDSP_DOMAIN_ID domain with a reserved memory
region, this code unconditionally passes the NULL scm pointer to
qcom_scm_assign_mem().

Inside qcom_scm_assign_mem(), the NULL pointer is immediately dereferenced
during the memory allocation call: 
qcom_tzmem_alloc(scm->mempool, ...)

[ ... ]
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index 5747bd191bf15..9c513fe6f0ea9 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
[ ... ]
> -int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
> -int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
> +int qcom_scm_io_readl(struct qcom_scm *scm, phys_addr_t addr, unsigned int *val);
> +int qcom_scm_io_writel(struct qcom_scm *scm, phys_addr_t addr, unsigned int val);

[Severity: Low]
Are there unupdated callers of qcom_scm_io_writel() that will cause a build
failure?

The enable_eud() and disable_eud() functions in drivers/usb/misc/qcom_eud.c
appear to call qcom_scm_io_writel() without the newly required struct qcom_scm
pointer as the first argument.

[ ... ]
> -int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
> +int qcom_scm_mem_protect_video_var(struct qcom_scm *scm, u32 cp_start, u32 cp_size,
>  				   u32 cp_nonpixel_start, u32 cp_nonpixel_size);

[Severity: Low]
Could this signature change also cause a build failure?

The iris_set_cp_prot() function in drivers/media/platform/qcom/iris/iris_firmware.c
appears to still call qcom_scm_mem_protect_video_var() without the newly
required struct qcom_scm pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-scm-device-api-v1-0-3573e2596c51@redhat.com?part=2