Re: [PATCH 3/3] remoteproc: qcom: pas: Add Nord ADSP support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The patch introduces a compilation failure by initializing a non-existent `early_boot` member, and omits the "attach path" logic promised in the commit message.
- [Low] The `qcom_pas_of_match` array is not kept in alphabetical order.

Pre-existing issues:
- [High] Missing unroll logic in `qcom_pas_assign_memory_region()` permanently leaks secure memory assignments on error.
--

--- Patch [3]: [PATCH 3/3] remoteproc: qcom: pas: Add Nord ADSP support ---
commit c4d506b746f90f7894797ce51ad96d54c9cd9077
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

remoteproc: qcom: pas: Add Nord ADSP support

The ADSP on Nord SoC is pre-booted by XBL before Linux starts. This patch
adds the ADSP resource descriptor and sets the early_boot flag for the
attach path rather than a cold boot sequence.

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -1354,6 +1354,27 @@ static const struct qcom_pas_data milos_cdsp_resource = {
>  	.smem_host_id = 5,
>  };
>  
> +static const struct qcom_pas_data nord_adsp_resource = {
> +	.crash_reason_smem = 423,
> +	.firmware_name = "adsp.mdt",
> +	.dtb_firmware_name = "adsp_dtb.mbn",
> +	.pas_id = 1,
> +	.dtb_pas_id = 36,
> +	.minidump_id = 5,
> +	.auto_boot = true,
> +	.early_boot = true,

[Severity: High]
Does this cause a build failure? Looking at the definition of
struct qcom_pas_data in drivers/remoteproc/qcom_q6v5_pas.c, it doesn't
appear to have an early_boot member.

Additionally, the commit message mentions setting the early_boot flag for
the attach path rather than a cold boot sequence. If the build is fixed by
simply adding early_boot to the struct, will the probe succeed but fall
back to the cold boot path since the driver appears to be missing the
attach logic?

This could cause the driver to reset and reload firmware on an
already-running DSP.

> +	.proxy_pd_names = (char*[]){
> +		"cx",
> +		"mx",
> +		NULL
> +	},

[ ... ]

> @@ -1578,6 +1599,7 @@ static const struct of_device_id qcom_pas_of_match[] = {
>  	{ .compatible = "qcom,milos-cdsp-pas", .data = &milos_cdsp_resource },
>  	{ .compatible = "qcom,milos-mpss-pas", .data = &sm8450_mpss_resource },
>  	{ .compatible = "qcom,milos-wpss-pas", .data = &sc7280_wpss_resource },
> +	{ .compatible = "qcom,nord-adsp-pas", .data = &nord_adsp_resource },
>  	{ .compatible = "qcom,msm8226-adsp-pil", .data = &msm8996_adsp_resource },

[Severity: Low]
This isn't a bug, but does this break the alphabetical ordering of the
qcom_pas_of_match array? The compatible qcom,nord-adsp-pas is inserted
between qcom,milos-wpss-pas and qcom,msm8226-adsp-pil. Since nord starts
with n, it should likely be placed further down after the msm entries.


[Severity: High]
This is a pre-existing issue, but I noticed a potential leak of secure
memory assignments on error paths:

drivers/remoteproc/qcom_q6v5_pas.c:qcom_pas_assign_memory_region() {
    ...
    if (ret < 0) {
        dev_err(pas->dev, "assign memory %d failed\n", offset);
        return ret;
    }
    ...
}

If an assignment fails at an offset greater than 0, the function returns
an error code without reverting the permissions of regions that were
successfully mapped in prior iterations.

Additionally, in qcom_pas_probe():

drivers/remoteproc/qcom_q6v5_pas.c:qcom_pas_probe() {
    ...
    ret = qcom_pas_assign_memory_region(pas);
    if (ret)
        goto free_rproc;
    ...
}

The probe failure path jumps to free_rproc, bypassing the unassign_mem
label and failing to call qcom_pas_unassign_memory_region().

Does this permanently leak system memory to the secure environment,
rendering it inaccessible to the host OS?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709085149.4072181-1-shengchao.guo@oss.qualcomm.com?part=3