[PATCH 0/8] iommu/qcom: Misc Fixes

[PATCH 0/8] iommu/qcom: Misc Fixes

[Mukesh Ojha]

Series to address fixes in legacy qcom_iommu driver, it is based on top
of  https://lore.kernel.org/lkml/20260623071245.1985938-1-haoxiang_li2024@163.com/

Mukesh Ojha (8):
  iommu/qcom: Fix inverted fault report check in qcom_iommu_fault()
  iommu/qcom: Fix missing pm_runtime_disable() in
    qcom_iommu_device_remove()
  iommu/qcom: Check pm_runtime_resume_and_get() return in probe
  iommu/qcom: Fix pgtbl_ops leak in qcom_iommu_init_domain() error path
  iommu/qcom: Publish pgtbl_ops before releasing init_mutex
  iommu/qcom: Add NULL ctx check in TLB invalidation paths
  iommu/qcom: Enable clocks before hardware access in
    qcom_iommu_ctx_probe()
  iommu/qcom: Document why sec_ptbl allocated flag needs no locking

 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 45 ++++++++++++++++++-------
 1 file changed, 33 insertions(+), 12 deletions(-)

-- 
2.53.0

[PATCH 1/8] iommu/qcom: Fix inverted fault report check in qcom_iommu_fault()

[Mukesh Ojha]

report_iommu_fault() returns 0 when a fault handler successfully handles
the fault, and -ENOSYS when no handler is installed. The condition
'!report_iommu_fault()' evaluates to true (printing "Unhandled context
fault") precisely when the fault *was* handled, and stays silent when no
handler is present — the opposite of what is intended.

Remove the '!' so the driver logs unhandled faults correctly.

Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 32efef69e72d..09f2ee6be988 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -200,7 +200,7 @@ static irqreturn_t qcom_iommu_fault(int irq, void *dev)
 	fsynr = iommu_readl(ctx, ARM_SMMU_CB_FSYNR0);
 	iova = iommu_readq(ctx, ARM_SMMU_CB_FAR);
 
-	if (!report_iommu_fault(ctx->domain, ctx->dev, iova, 0)) {
+	if (report_iommu_fault(ctx->domain, ctx->dev, iova, 0)) {
 		dev_err_ratelimited(ctx->dev,
 				    "Unhandled context fault: fsr=0x%x, "
 				    "iova=0x%016llx, fsynr=0x%x, cb=%d\n",
-- 
2.53.0

[PATCH 2/8] iommu/qcom: Fix missing pm_runtime_disable() in qcom_iommu_device_remove()

[Mukesh Ojha]

qcom_iommu_device_probe() calls pm_runtime_enable() but
qcom_iommu_device_remove() only calls pm_runtime_force_suspend() without
a matching pm_runtime_disable(). This leaves runtime PM enabled after the
driver unbinds, which can cause issues on rebind or if any code races to
resume the device after removal.

Add pm_runtime_disable() in the remove path to balance the enable in probe.

Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 09f2ee6be988..cb43276f4a39 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -878,6 +878,7 @@ static void qcom_iommu_device_remove(struct platform_device *pdev)
 	struct qcom_iommu_dev *qcom_iommu = platform_get_drvdata(pdev);
 
 	pm_runtime_force_suspend(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	platform_set_drvdata(pdev, NULL);
 	iommu_device_sysfs_remove(&qcom_iommu->iommu);
 	iommu_device_unregister(&qcom_iommu->iommu);
-- 
2.53.0

[PATCH 3/8] iommu/qcom: Check pm_runtime_resume_and_get() return in probe

[Mukesh Ojha]

The SMMU_INTR_SEL_NS register write in qcom_iommu_device_probe() uses
pm_runtime_get_sync() without checking the return value. If runtime
resume fails the subsequent writel_relaxed() would access hardware with
clocks potentially disabled.

Switch to pm_runtime_resume_and_get() which handles the usage-count
cleanup on failure, check the return value, and unwind the already
registered iommu device on error.

Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index cb43276f4a39..4e714a8e1fac 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -859,13 +859,17 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
 	}
 
 	if (qcom_iommu->local_base) {
-		pm_runtime_get_sync(dev);
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret)
+			goto err_iommu_unregister;
 		writel_relaxed(0xffffffff, qcom_iommu->local_base + SMMU_INTR_SEL_NS);
 		pm_runtime_put_sync(dev);
 	}
 
 	return 0;
 
+err_iommu_unregister:
+	iommu_device_unregister(&qcom_iommu->iommu);
 err_sysfs_remove:
 	iommu_device_sysfs_remove(&qcom_iommu->iommu);
 err_pm_disable:
-- 
2.53.0

[PATCH 4/8] iommu/qcom: Fix pgtbl_ops leak in qcom_iommu_init_domain() error path

[Mukesh Ojha]

alloc_io_pgtable_ops() can succeed and then qcom_scm_restore_sec_cfg()
can fail for one of the context banks. The goto out_clear_iommu path
only cleared qcom_domain->iommu; the locally allocated pgtbl_ops was
never freed, leaking it permanently since qcom_domain->pgtbl_ops is only
assigned on the success path.

free_io_pgtable_ops() safely handles a NULL argument (covers the case
where alloc_io_pgtable_ops() itself failed), so add it unconditionally in
the out_clear_iommu handler.

Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 4e714a8e1fac..b6ce85f7f923 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -314,6 +314,7 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 	return 0;
 
 out_clear_iommu:
+	free_io_pgtable_ops(pgtbl_ops);
 	qcom_domain->iommu = NULL;
 out_unlock:
 	mutex_unlock(&qcom_domain->init_mutex);
-- 
2.53.0

[PATCH 5/8] iommu/qcom: Publish pgtbl_ops before releasing init_mutex

[Mukesh Ojha]

qcom_domain->pgtbl_ops was assigned after mutex_unlock(). Another thread
calling qcom_iommu_init_domain() would see qcom_domain->iommu already set
(domain fully initialized) and skip re-initialization under the mutex.
If it then called qcom_iommu_map() before the first thread set pgtbl_ops,
it would observe a NULL ops pointer and return -ENODEV for valid mappings.

Move the assignment to before mutex_unlock() so that once the mutex is
released the domain is fully visible to concurrent operations.

Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index b6ce85f7f923..40fb0408dc07 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -306,13 +306,12 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 		ctx->domain = domain;
 	}
 
-	mutex_unlock(&qcom_domain->init_mutex);
-
 	/* Publish page table ops for map/unmap */
 	qcom_domain->pgtbl_ops = pgtbl_ops;
 
-	return 0;
+	mutex_unlock(&qcom_domain->init_mutex);
 
+	return 0;
 out_clear_iommu:
 	free_io_pgtable_ops(pgtbl_ops);
 	qcom_domain->iommu = NULL;
-- 
2.53.0

[PATCH 6/8] iommu/qcom: Add NULL ctx check in TLB invalidation paths

[Mukesh Ojha]

to_ctx() returns qcom_iommu->ctxs[asid], which can be NULL if the
corresponding context bank failed to probe or was already removed.
qcom_iommu_tlb_sync(), qcom_iommu_tlb_inv_context(), and
qcom_iommu_tlb_inv_range_nosync() all dereference the returned pointer
directly, risking a NULL pointer dereference.

Add WARN_ON(!ctx) guards with continue so TLB operations skip
broken context banks without crashing.

Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 40fb0408dc07..51b60b296bb8 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -121,6 +121,9 @@ static void qcom_iommu_tlb_sync(void *cookie)
 		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
 		unsigned int val, ret;
 
+		if (WARN_ON(!ctx))
+			continue;
+
 		iommu_writel(ctx, ARM_SMMU_CB_TLBSYNC, 0);
 
 		ret = readl_poll_timeout(ctx->base + ARM_SMMU_CB_TLBSTATUS, val,
@@ -138,6 +141,10 @@ static void qcom_iommu_tlb_inv_context(void *cookie)
 
 	for (i = 0; i < fwspec->num_ids; i++) {
 		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
+
+		if (WARN_ON(!ctx))
+			continue;
+
 		iommu_writel(ctx, ARM_SMMU_CB_S1_TLBIASID, ctx->asid);
 	}
 
@@ -157,6 +164,9 @@ static void qcom_iommu_tlb_inv_range_nosync(unsigned long iova, size_t size,
 		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
 		size_t s = size;
 
+		if (WARN_ON(!ctx))
+			continue;
+
 		iova = (iova >> 12) << 12;
 		iova |= ctx->asid;
 		do {
-- 
2.53.0

[PATCH 7/8] iommu/qcom: Enable clocks before hardware access in qcom_iommu_ctx_probe()

[Mukesh Ojha]

qcom_iommu_ctx_probe() reads and writes the CB_FSR register to clear any
stale IRQ left by the bootloader. This happens during
devm_of_platform_populate() which is called from the parent device's
probe before any pm_runtime_get(). The parent's clocks (iface, bus, tbu)
are therefore not guaranteed to be on, making the register access
unreliable on rebind or after a suspend cycle.

Use pm_runtime_resume_and_get() on the parent device to ensure clocks
are enabled before the register access, and release the reference
immediately after.

Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 51b60b296bb8..0df8c2af8eed 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -714,8 +714,13 @@ static int qcom_iommu_ctx_probe(struct platform_device *pdev)
 	/* clear IRQs before registering fault handler, just in case the
 	 * boot-loader left us a surprise:
 	 */
-	if (!ctx->secured_ctx)
+	if (!ctx->secured_ctx) {
+		ret = pm_runtime_resume_and_get(dev->parent);
+		if (ret)
+			return ret;
 		iommu_writel(ctx, ARM_SMMU_CB_FSR, iommu_readl(ctx, ARM_SMMU_CB_FSR));
+		pm_runtime_put_sync(dev->parent);
+	}
 
 	ret = devm_request_irq(dev, irq,
 			       qcom_iommu_fault,
-- 
2.53.0

[PATCH 8/8] iommu/qcom: Document why sec_ptbl allocated flag needs no locking

[Mukesh Ojha]

qcom_iommu_sec_ptbl_init() uses a function-static bool to track whether
the secure page table has been initialized, with no locking around it.

Only one IOMMU device per SoC has secure context banks (the others have
only non-secure context banks), and platform devices probe serially since
the driver does not set PROBE_PREFER_ASYNCHRONOUS. Concurrent calls to
this function are therefore not reachable. Add a comment to make the
absence of locking intentional rather than an oversight.

Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 0df8c2af8eed..bcf5ab049aed 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -624,7 +624,11 @@ static int qcom_iommu_sec_ptbl_init(struct device *dev)
 	void *cpu_addr;
 	dma_addr_t paddr;
 	unsigned long attrs;
-	static bool allocated = false;
+	/*
+	 * Only one IOMMU device per SoC has secure context banks, and
+	 * platform devices probe serially, so no locking is needed here.
+	 */
+	static bool allocated;
 	int ret;
 
 	if (allocated)
@@ -651,15 +655,12 @@ static int qcom_iommu_sec_ptbl_init(struct device *dev)
 	ret = qcom_scm_iommu_secure_ptbl_init(paddr, psize, spare);
 	if (ret) {
 		dev_err(dev, "failed to init iommu pgtable (%d)\n", ret);
-		goto free_mem;
+		dma_free_attrs(dev, psize, cpu_addr, paddr, attrs);
+		return ret;
 	}
 
 	allocated = true;
 	return 0;
-
-free_mem:
-	dma_free_attrs(dev, psize, cpu_addr, paddr, attrs);
-	return ret;
 }
 
 static int get_asid(const struct device_node *np)
-- 
2.53.0

Re: [PATCH 1/8] iommu/qcom: Fix inverted fault report check in qcom_iommu_fault()

[Konrad Dybcio]

On 6/23/26 2:20 PM, Mukesh Ojha wrote:
> report_iommu_fault() returns 0 when a fault handler successfully handles
> the fault, and -ENOSYS when no handler is installed. The condition
> '!report_iommu_fault()' evaluates to true (printing "Unhandled context
> fault") precisely when the fault *was* handled, and stays silent when no
> handler is present — the opposite of what is intended.
> 
> Remove the '!' so the driver logs unhandled faults correctly.
> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
>  drivers/iommu/arm/arm-smmu/qcom_iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 32efef69e72d..09f2ee6be988 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -200,7 +200,7 @@ static irqreturn_t qcom_iommu_fault(int irq, void *dev)
>  	fsynr = iommu_readl(ctx, ARM_SMMU_CB_FSYNR0);
>  	iova = iommu_readq(ctx, ARM_SMMU_CB_FAR);
>  
> -	if (!report_iommu_fault(ctx->domain, ctx->dev, iova, 0)) {
> +	if (report_iommu_fault(ctx->domain, ctx->dev, iova, 0)) {

Absolutely hilarious

Fixes: 049541e178d5 ("iommu: qcom: wire up fault handler")
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

>  		dev_err_ratelimited(ctx->dev,
>  				    "Unhandled context fault: fsr=0x%x, "
>  				    "iova=0x%016llx, fsynr=0x%x, cb=%d\n",

Re: [PATCH 2/8] iommu/qcom: Fix missing pm_runtime_disable() in qcom_iommu_device_remove()

[Konrad Dybcio]

On 6/23/26 2:20 PM, Mukesh Ojha wrote:
> qcom_iommu_device_probe() calls pm_runtime_enable() but
> qcom_iommu_device_remove() only calls pm_runtime_force_suspend() without
> a matching pm_runtime_disable(). This leaves runtime PM enabled after the
> driver unbinds, which can cause issues on rebind or if any code races to
> resume the device after removal.
> 
> Add pm_runtime_disable() in the remove path to balance the enable in probe.
> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
>  drivers/iommu/arm/arm-smmu/qcom_iommu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 09f2ee6be988..cb43276f4a39 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -878,6 +878,7 @@ static void qcom_iommu_device_remove(struct platform_device *pdev)
>  	struct qcom_iommu_dev *qcom_iommu = platform_get_drvdata(pdev);
>  
>  	pm_runtime_force_suspend(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);

devm_ would be neater

Konrad

Re: [PATCH 3/8] iommu/qcom: Check pm_runtime_resume_and_get() return in probe

[Konrad Dybcio]

On 6/23/26 2:20 PM, Mukesh Ojha wrote:
> The SMMU_INTR_SEL_NS register write in qcom_iommu_device_probe() uses
> pm_runtime_get_sync() without checking the return value. If runtime
> resume fails the subsequent writel_relaxed() would access hardware with
> clocks potentially disabled.
> 
> Switch to pm_runtime_resume_and_get() which handles the usage-count
> cleanup on failure, check the return value, and unwind the already
> registered iommu device on error.
> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 4/8] iommu/qcom: Fix pgtbl_ops leak in qcom_iommu_init_domain() error path

[Konrad Dybcio]

On 6/23/26 2:20 PM, Mukesh Ojha wrote:
> alloc_io_pgtable_ops() can succeed and then qcom_scm_restore_sec_cfg()
> can fail for one of the context banks. The goto out_clear_iommu path
> only cleared qcom_domain->iommu; the locally allocated pgtbl_ops was
> never freed, leaking it permanently since qcom_domain->pgtbl_ops is only
> assigned on the success path.
> 
> free_io_pgtable_ops() safely handles a NULL argument (covers the case
> where alloc_io_pgtable_ops() itself failed), so add it unconditionally in
> the out_clear_iommu handler.
> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
>  drivers/iommu/arm/arm-smmu/qcom_iommu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 4e714a8e1fac..b6ce85f7f923 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -314,6 +314,7 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>  	return 0;
>  
>  out_clear_iommu:
> +	free_io_pgtable_ops(pgtbl_ops);


This label also jumped to when alloc_io_pgtable_ops() succeeds,
but there's a nullcheck inside, so i guess it's fine

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 5/8] iommu/qcom: Publish pgtbl_ops before releasing init_mutex

[Konrad Dybcio]

On 6/23/26 2:20 PM, Mukesh Ojha wrote:
> qcom_domain->pgtbl_ops was assigned after mutex_unlock(). Another thread
> calling qcom_iommu_init_domain() would see qcom_domain->iommu already set
> (domain fully initialized) and skip re-initialization under the mutex.
> If it then called qcom_iommu_map() before the first thread set pgtbl_ops,
> it would observe a NULL ops pointer and return -ENODEV for valid mappings.
> 
> Move the assignment to before mutex_unlock() so that once the mutex is
> released the domain is fully visible to concurrent operations.
> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
>  drivers/iommu/arm/arm-smmu/qcom_iommu.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index b6ce85f7f923..40fb0408dc07 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -306,13 +306,12 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>  		ctx->domain = domain;
>  	}
>  
> -	mutex_unlock(&qcom_domain->init_mutex);
> -
>  	/* Publish page table ops for map/unmap */
>  	qcom_domain->pgtbl_ops = pgtbl_ops;
>  
> -	return 0;
> +	mutex_unlock(&qcom_domain->init_mutex);

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

even better, we could probably just wrap this in guard(mutex) now

Konrad

Re: [PATCH 7/8] iommu/qcom: Enable clocks before hardware access in qcom_iommu_ctx_probe()

[Konrad Dybcio]

On 6/23/26 2:20 PM, Mukesh Ojha wrote:
> qcom_iommu_ctx_probe() reads and writes the CB_FSR register to clear any
> stale IRQ left by the bootloader. This happens during
> devm_of_platform_populate() which is called from the parent device's
> probe before any pm_runtime_get(). The parent's clocks (iface, bus, tbu)
> are therefore not guaranteed to be on, making the register access
> unreliable on rebind or after a suspend cycle.
> 
> Use pm_runtime_resume_and_get() on the parent device to ensure clocks
> are enabled before the register access, and release the reference
> immediately after.
> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 8/8] iommu/qcom: Document why sec_ptbl allocated flag needs no locking

[Konrad Dybcio]

On 6/23/26 2:20 PM, Mukesh Ojha wrote:
> qcom_iommu_sec_ptbl_init() uses a function-static bool to track whether
> the secure page table has been initialized, with no locking around it.
> 
> Only one IOMMU device per SoC has secure context banks (the others have
> only non-secure context banks), and platform devices probe serially since
> the driver does not set PROBE_PREFER_ASYNCHRONOUS. Concurrent calls to

This may be influenced by cmdline, so I'd rather not take it for granted

although I'm not sure how this plays out with the context bank device
always being a child of the IOMMU

Konrad

Re: [PATCH 6/8] iommu/qcom: Add NULL ctx check in TLB invalidation paths

[Konrad Dybcio]

On 6/23/26 2:20 PM, Mukesh Ojha wrote:
> to_ctx() returns qcom_iommu->ctxs[asid], which can be NULL if the
> corresponding context bank failed to probe or was already removed.
> qcom_iommu_tlb_sync(), qcom_iommu_tlb_inv_context(), and
> qcom_iommu_tlb_inv_range_nosync() all dereference the returned pointer
> directly, risking a NULL pointer dereference.
> 
> Add WARN_ON(!ctx) guards with continue so TLB operations skip
> broken context banks without crashing.
> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
>  drivers/iommu/arm/arm-smmu/qcom_iommu.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 40fb0408dc07..51b60b296bb8 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -121,6 +121,9 @@ static void qcom_iommu_tlb_sync(void *cookie)
>  		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
>  		unsigned int val, ret;
>  
> +		if (WARN_ON(!ctx))
> +			continue;

I'm rather unamused that we have to deal with this in the first
place.. I don't know if this can be easily reworked to be more
predictable, but this works in the interim

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 6/8] iommu/qcom: Add NULL ctx check in TLB invalidation paths

[Robin Murphy]

On 23/06/2026 1:20 pm, Mukesh Ojha wrote:
> to_ctx() returns qcom_iommu->ctxs[asid], which can be NULL if the
> corresponding context bank failed to probe or was already removed.
> qcom_iommu_tlb_sync(), qcom_iommu_tlb_inv_context(), and
> qcom_iommu_tlb_inv_range_nosync() all dereference the returned pointer
> directly, risking a NULL pointer dereference.

But if there's no context bank, then how has a domain been allocated in 
order to permit io-pgtable operations that would eventually call into 
qcom_flush_ops at all? Can you please clarify whether you've actually 
observed a real-world issue here, and if so how?

Thanks,
Robin.

> Add WARN_ON(!ctx) guards with continue so TLB operations skip
> broken context banks without crashing.
> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
> ---
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 40fb0408dc07..51b60b296bb8 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -121,6 +121,9 @@ static void qcom_iommu_tlb_sync(void *cookie)
>   		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
>   		unsigned int val, ret;
>   
> +		if (WARN_ON(!ctx))
> +			continue;
> +
>   		iommu_writel(ctx, ARM_SMMU_CB_TLBSYNC, 0);
>   
>   		ret = readl_poll_timeout(ctx->base + ARM_SMMU_CB_TLBSTATUS, val,
> @@ -138,6 +141,10 @@ static void qcom_iommu_tlb_inv_context(void *cookie)
>   
>   	for (i = 0; i < fwspec->num_ids; i++) {
>   		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
> +
> +		if (WARN_ON(!ctx))
> +			continue;
> +
>   		iommu_writel(ctx, ARM_SMMU_CB_S1_TLBIASID, ctx->asid);
>   	}
>   
> @@ -157,6 +164,9 @@ static void qcom_iommu_tlb_inv_range_nosync(unsigned long iova, size_t size,
>   		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
>   		size_t s = size;
>   
> +		if (WARN_ON(!ctx))
> +			continue;
> +
>   		iova = (iova >> 12) << 12;
>   		iova |= ctx->asid;
>   		do {

Re: [PATCH 6/8] iommu/qcom: Add NULL ctx check in TLB invalidation paths

[Mukesh Ojha]

On Tue, Jun 23, 2026 at 06:08:08PM +0100, Robin Murphy wrote:
> On 23/06/2026 1:20 pm, Mukesh Ojha wrote:
> > to_ctx() returns qcom_iommu->ctxs[asid], which can be NULL if the
> > corresponding context bank failed to probe or was already removed.
> > qcom_iommu_tlb_sync(), qcom_iommu_tlb_inv_context(), and
> > qcom_iommu_tlb_inv_range_nosync() all dereference the returned pointer
> > directly, risking a NULL pointer dereference.
> 
> But if there's no context bank, then how has a domain been allocated in
> order to permit io-pgtable operations that would eventually call into
> qcom_flush_ops at all? Can you please clarify whether you've actually
> observed a real-world issue here, and if so how?

You're right, I haven't observed a real crash here.
qcom_iommu_of_xlate already rejects any ASID whose ctxs[] slot is
NULL, so a domain can never be attached  and therefore
qcom_flush_ops can never be reached  with a NULL ctx.
I'll drop it.

-- 
-Mukesh Ojha