Re: [PATCH v6 06/16] remoteproc: qcom_q6v5_mss: Switch to generic PAS TZ APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect dependency check replacement leads to potential NULL pointer dereference
- [High] Missing probe deferral for PAS availability causes runtime firmware load failures
- [High] The modified PAS memory setup feature is dead code due to missing initialization
--

commit f2a63e3caa15c50ea25aa5e07776a62f41d75add
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>

remoteproc: qcom_q6v5_mss: Switch to generic PAS TZ APIs

This commit switches the qcom_q6v5_mss client driver over to the generic PAS
TZ APIs to support multiple backends like QTEE based SCM and OP-TEE.

> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index ae78f5c7c1b69..96888007faa88 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
[ ... ]
> @@ -1480,7 +1481,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
>  	}
>  
>  	if (qproc->need_pas_mem_setup) {
> -		ret = qcom_scm_pas_mem_setup(MPSS_PAS_ID, qproc->mpss_phys, qproc->mpss_size);
> +		ret = qcom_pas_mem_setup(MPSS_PAS_ID, qproc->mpss_phys, qproc->mpss_size);

While this isn't a regression from this commit, is qproc->need_pas_mem_setup
ever initialized from desc in probe?

Looking at q6v5_probe(), several flags are copied from the match data to the
device instance:

    qproc->need_mem_protection = desc->need_mem_protection;
    qproc->has_mba_logs = desc->has_mba_logs;

Since qproc is zero-initialized by devm_rproc_alloc(), does the missing
copy of need_pas_mem_setup leave the flag permanently false and make
this modified qcom_pas_mem_setup() block unreachable?

[ ... ]
> @@ -2077,7 +2078,7 @@ static int q6v5_probe(struct platform_device *pdev)
>  	if (!desc)
>  		return -EINVAL;
>  
> -	if (desc->need_mem_protection && !qcom_scm_is_available())
> +	if (desc->need_mem_protection && !qcom_pas_is_available())

Does replacing this probe deferral check cause issues if the generic PAS
backend is available (like OP-TEE) but SCM is not initialized?

The need_mem_protection flag dictates whether q6v5_xfer_mem_ownership() is
called. That function unconditionally relies on qcom_scm_assign_mem() which
directly accesses the SCM backend. If qcom_pas_is_available() allows probe
to succeed without SCM, could this result in a NULL pointer dereference
when qcom_scm_assign_mem() accesses the backend?

Additionally, what happens for platforms where desc->need_mem_protection is
false, but desc->need_pas_mem_setup is true?

It appears probe will skip checking if PAS is available and register the
remoteproc immediately. Later during firmware load, wouldn't
qcom_pas_mem_setup() fail with -ENODEV if the PAS backend has not yet probed?

Should probe also defer when desc->need_pas_mem_setup is true and PAS is
not available?

>  		return -EPROBE_DEFER;
>  
>  	mba_image = desc->hexagon_mba_image;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518072856.22790-1-sumit.garg@kernel.org?part=6