Re: [PATCH RFC v2 07/14] iommu: arm-smmu-qcom: Fixed mapping between sid and cb for gpu and lpac

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iommu/arm-smmu-qcom: unconditional CB 1 reservation for LPAC shifts GMU CB
--

commit 51e5e8227679664f11df23a51921048a054a1310
Author: Anna Maniscalco <anna.maniscalco2000@gmail.com>

iommu: arm-smmu-qcom: Fixed mapping between sid and cb for gpu and lpac

This commit splits context bank allocation and initialization in
arm-smmu-qcom so that the gpu device (stream id 0) is assigned context bank 0
and the lpac device (stream id 1) is assigned context bank 1, while other
devices start allocation from context bank 2.

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index ae6152bddf8fe..9413b748e79d1 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c

[ ... ]

> @@ -313,12 +330,14 @@ static int qcom_adreno_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_doma
>  	 */
>  	if (qcom_adreno_smmu_is_gpu_device(dev)) {
>  		start = 0;
> -		count = 2;
> +		count = start + 1;
> +	} else if (qcom_adreno_smmu_is_lpac_device(dev)) {
> +		start = 1;
> +		count = start + 1;
>  	} else {
>  		start = 2;
>  		count = smmu->num_context_banks;
>  	}
> -
>  	return __arm_smmu_alloc_bitmap(smmu->context_map, start, count);
>  }

[Severity: High]
Does unconditionally reserving context bank 1 for a separate lpac device and
forcing all other devices into the else branch with start = 2 cause a
regression on platforms without a separate qcom,lpac device node?

In drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c,
qcom_adreno_smmu_alloc_context_bank() now assigns only context bank 0 when
qcom_adreno_smmu_is_gpu_device(dev) is true, reserves context bank 1 for a
separate device matching qcom_adreno_smmu_is_lpac_device(dev), and starts all
other allocations at context bank 2.

On platforms that do not have stream id 1 (such as sdm845, sc7180, sm8150, and
sm8250), or on platforms where both stream id 0 and stream id 1 are listed on
the main gpu node rather than a separate qcom,lpac child node (such as
sc8280xp, sm8350, sc7280, sm8450, sm8550, x1e80100, or sm8650 with an older
device tree), the main gpu device matches qcom_adreno_smmu_is_gpu_device(dev)
first and allocates context bank 0, leaving context bank 1 unallocated.

When a6xx_gmu_memory_probe() -> msm_iommu_new() -> iommu_attach_device() ->
arm_smmu_init_domain_context() -> qcom_adreno_smmu_alloc_context_bank()
subsequently allocates a context bank for the qcom,adreno-gmu device (stream
id 5), it falls into the else branch with start = 2 and receives cbndx = 2
instead of cbndx = 1. As noted in commit afc95681c306 ("iommu/arm-smmu-qcom:
Add missing GMU entry to match table"), having the gmu on cbndx = 2 instead of
cbndx = 1 on sc8280xp causes the smmu to wedge on gpu translation faults.

In addition, on platforms with only two context banks available to Linux,
such as qcom,sdm630-smmu-v2:

drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:qcom_adreno_smmuv2_cfg_probe() {
	/* TZ protects several last context banks, hide them from Linux */
	if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm630-smmu-v2") &&
	    smmu->num_context_banks == 5)
		smmu->num_context_banks = 2;
}

entering the else branch passes start = 2 and count = 2 to
__arm_smmu_alloc_bitmap(), which immediately fails even though context bank 1
is free:

drivers/iommu/arm/arm-smmu/arm-smmu.h:__arm_smmu_alloc_bitmap() {
	do {
		idx = find_next_zero_bit(map, end, start);
		if (idx == end)
			return -ENOSPC;
	} while (test_and_set_bit(idx, map));
}

Could context bank 1 only be reserved when a separate lpac device is actually
present, or should the else branch still allow allocating from context bank 1
when lpac is not used?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-descriptive-name-lpac-upstream-v2-0-a21a5c850961@gmail.com?part=7