Re: [PATCH 08/12] iommu: qcom_iommu: halt the micro-MMU while programming context banks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent domain attachments and context programming cause race conditions in the global micro-MMU halt state, leading to premature unhalting and data races on the control register.
- [Medium] Spinning for up to 100ms in a tight loop using `cpu_relax()` risks degrading system responsiveness on hardware timeout.
--

--- Patch [8]: [PATCH 08/12] iommu: qcom_iommu: halt the micro-MMU while programming context banks ---
commit b970b685e94c99403661414d09a198b270daac8a
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

iommu: qcom_iommu: halt the micro-MMU while programming context banks

This commit adds a hardware handshake to halt the micro-MMU before
reprogramming context bank registers. It introduces qcom_iommu_halt()
and qcom_iommu_unhalt() to wrap the context programming sequence.

> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 8e9488c47d5c1..f27f8722ee640 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c

[ ... ]

> @@ -339,9 +349,41 @@ static int qcom_iommu_reset_ns(struct qcom_iommu_dev *qcom_iommu)
>  	return 0;
>  }
>  
> +/*
> + * Halting the micro-MMU quiesces the translation front-end: it stops new
> + * client transactions being accepted and waits for the outstanding ones to
> + * retire, so that the context bank registers can be reprogrammed without
> + * in-flight traffic racing the change.
> + */
> +static void qcom_iommu_halt(struct qcom_iommu_dev *qcom_iommu)
> +{
> +	void __iomem *reg = qcom_iommu->global_base + QCOM_IOMMU_MICRO_MMU_CTRL;
> +	u32 val;
> +
> +	if (!qcom_iommu->cfg || !qcom_iommu->cfg->halt)
> +		return;
> +
> +	writel_relaxed(readl_relaxed(reg) | MICRO_MMU_CTRL_HALT_REQ, reg);

[Severity: High]
Could this read-modify-write on the global MICRO_MMU_CTRL register race if
devices mapped to different IOMMU domains are probing or attaching
concurrently?

> +
> +	if (readl_poll_timeout(reg, val, val & MICRO_MMU_CTRL_IDLE, 0, 100000))

[Severity: Medium]
Does setting the delay parameter to 0 cause this to spin continuously in a
tight cpu_relax() loop for up to 100ms?

If the hardware becomes unresponsive, especially in callers like
qcom_iommu_resume() which loop over multiple context banks, could this block
the CPU for over a second in a non-preemptible kernel?

> +		dev_err(qcom_iommu->dev, "timeout waiting for micro-MMU halt\n");
> +}
> +
> +static void qcom_iommu_unhalt(struct qcom_iommu_dev *qcom_iommu)
> +{
> +	void __iomem *reg = qcom_iommu->global_base + QCOM_IOMMU_MICRO_MMU_CTRL;
> +
> +	if (!qcom_iommu->cfg || !qcom_iommu->cfg->halt)
> +		return;
> +
> +	writel_relaxed(readl_relaxed(reg) & ~MICRO_MMU_CTRL_HALT_REQ, reg);

[Severity: High]
If qcom_iommu_program_ctx() is called concurrently for different domains,
could this unconditional clear prematurely unhalt the micro-MMU while another
thread is still actively programming a different context bank?

Since this modifies the global micro-MMU state without a device-level lock or
reference count, does this risk allowing in-flight traffic to race against the
reconfiguration?

> +}
> +
>  static void qcom_iommu_program_ctx(struct qcom_iommu_dev *qcom_iommu,
>  				   struct qcom_iommu_ctx *ctx)
>  {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809-msm8974-iommu-upstream-v1-0-87f5cd492560@oss.qualcomm.com?part=8