Re: [PATCH RFC 05/13] firmware: qcom: scm: Configure LPAC aperture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional SCM call for LPAC aperture configuration on hardware that does not support it.
- [Medium] Unused mask macro `QCOM_SCM_LPAC_APERTURE_CONTEXT_MASK` and missing `FIELD_PREP` bounds masking in `qcom_scm_set_gpu_smmu_lpac_aperture`.
--

commit 25a397c9a4153cea4348b027438df84a69f95cee
Author: Anna Maniscalco <anna.maniscalco2000@gmail.com>

firmware: qcom: scm: Configure LPAC aperture

In order to be able to update the SMMU registers from CP for page table
updates we need to configure the LPAC aperture to include the context
bank that will be associated with SID 1.

> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 9b06a69d3a6d7..92093399f20d9 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1204,6 +1204,7 @@ int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare)
>  EXPORT_SYMBOL_GPL(qcom_scm_restore_sec_cfg);
>  
>  #define QCOM_SCM_CP_APERTURE_CONTEXT_MASK	GENMASK(7, 0)
> +#define QCOM_SCM_LPAC_APERTURE_CONTEXT_MASK	GENMASK(7, 1)

[Severity: Medium]
This isn't a bug, but QCOM_SCM_LPAC_APERTURE_CONTEXT_MASK is defined here
but left unused. Also, is GENMASK(7, 1) correct, or should it be
GENMASK(7, 0) like the CP mask above?

[ ... ]

> @@ -1229,6 +1230,23 @@ int qcom_scm_set_gpu_smmu_aperture(unsigned int context_bank)
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_set_gpu_smmu_aperture);
>  
> +int qcom_scm_set_gpu_smmu_lpac_aperture(unsigned int context_bank)
> +{
> +	struct qcom_scm_desc desc = {
> +		.svc = QCOM_SCM_SVC_MP,
> +		.cmd = QCOM_SCM_MP_CP_SMMU_APERTURE_ID,
> +		.arginfo = QCOM_SCM_ARGS(4),
> +		.args[0] = 0xffff0000 | (1 << 8 | context_bank),

[Severity: Medium]
Should this use FIELD_PREP() with the QCOM_SCM_LPAC_APERTURE_CONTEXT_MASK
defined above?

Directly ORing context_bank bypasses bounds checking, which could
potentially allow an out-of-bounds context_bank to corrupt adjacent bits
like the LPAC bit (1 << 8).

> +		.args[1] = 0xffffffff,
> +		.args[2] = 0xffffffff,
> +		.args[3] = 0xffffffff,
> +		.owner = ARM_SMCCC_OWNER_SIP
> +	};
> +
> +	return qcom_scm_call(__scm->dev, &desc, NULL);
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_set_gpu_smmu_lpac_aperture);

[ ... ]

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 71ce4cbbf27ae..be374cf209f4f 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -668,6 +668,9 @@ int adreno_hw_init(struct msm_gpu *gpu)
>  		ret = qcom_scm_set_gpu_smmu_aperture(0);
>  		if (ret)
>  			DRM_DEV_ERROR(gpu->dev->dev, "unable to set SMMU aperture: %d\n", ret);
> +		ret = qcom_scm_set_gpu_smmu_lpac_aperture(1);
> +		if (ret)
> +			DRM_DEV_ERROR(gpu->dev->dev, "unable to set lpac SMMU aperture: %d\n", ret);

[Severity: High]
Does this unconditionally call qcom_scm_set_gpu_smmu_lpac_aperture() on
hardware that might not support LPAC?

The outer condition only checks qcom_scm_set_gpu_smmu_aperture_is_available()
which verifies the existence of QCOM_SCM_MP_CP_SMMU_APERTURE_ID. Older
ADRENO_6XX_GEN1+ devices support this command for the CP aperture, but
may not support LPAC.

If older firmware rejects the LPAC command, could it cause boot failures or
log errors on every boot and resume?

Alternatively, if the older firmware doesn't validate bit 8 and simply
masks the lower 8 bits, could it silently overwrite the primary CP aperture
context bank from 0 to 1, causing SMMU translation faults and rendering the
GPU unusable?

Should this be conditionally guarded by checking if LPAC is available,
such as checking if gpu->lpac_pdev is non-NULL?

>  	}
>  
>  	for (int i = 0; i < gpu->nr_rings; i++) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-descriptive-name-lpac-upstream-v1-0-01d50c3e0c99@gmail.com?part=5