[RFC PATCH v2 00/10] iommu/arm-smmu-v3: Implement Runtime/System Sleep ops

[RFC PATCH v2 00/10] iommu/arm-smmu-v3: Implement Runtime/System Sleep ops

[Pranjal Shrivastava]

As arm-smmu-v3 rapidly finds its way into SoCs designed for hand-held
devices, power management capabilities, similar to its predecessors, are
crucial for these applications. This series introduces power management
support for the arm-smmu-v3 driver.

Design
=======
The arm-smmu-v3 primarily operates with in-memory data structures
through HW registers pointing to these data structures in some fashion.
The proposed design tries to make use of this fact for implementing the
suspend and resume ops.

1. Suspend / Resume
The idea for the "suspend" op is to wait till all the queues are flushed
before disabling the SMMU through CR0. In order to avoid mis-use or
spurious transactions (b/w SMMU disable -> power-down), the GBPA register
is configured to abort all transactions.

The resume operation uses the `arm_smmu_device_reset` function which
re-initializes the HW using the SW-copies maintained by the driver. For
example, prod/cons for queues, base addresses for queues & tables. The
arm_smmu_device_reset also clears the TLBs and config caches.

2. Interrupt Re-config
a. Wired irqs: The series refactors the `arm_smmu_setup_irqs` to be
able to enable/disable irqs and install their handlers separately to
help with the re-initialization of the interrupts correctly.

b. MSIs: The series relies on caching the msi_msg and retrieving it
through a newly introduced helper `arm_smmu_resume_msis()` which
re-configures the *_IRQ_CFGn registers via writing back cached msi_msgs.

3. Handling user-space attachments
This version of the series relies on the user-space paths invoking the
iommu_*_claim_dma_owner API. The existing IOMMU API 
iommu_group_dma_owner_claimed() checks if DMA ownership for a group has
been claimed. However, it does NOT distinguish between kernel clients 
and user-space clients. This version of the series introduce a new helper
iommu_group_dma_owner_user() which allows the callers to specifically
determine if a iommu_group is owned by the user-space.

The function checks the group->owner field which is exclusively managed
by the iommu_group_claim_dma_owner() OR iommu_device_claim_dma_owner(),
instead of relying on the iommu_group->owner_cnt. Both the
*_claim_dma_owner APIs are ONLY called by vfio and iommufd at the moment
indicating user-space's DMA ownership for the said iommu_group.

Based on this new API, the arm-smmu-v3 driver tracks all the non-trusted
attachments in a list and keeps deferring suspend till the list of
non-trusted devices is empty, i.e. no non-trusted attachment exists.

SVA: The binding driver would either be in user-space or kernel-space.
For the latter, devlinks would suffice and for the former, we won't
suspend until all user-space attachments are removed.

4. Invoking runtime_pm_get/put
Given that most of the configuration done by arm-smmu-v3 is stored in
memory, the idea in this version of the series is to elide all TLBIs and
CFGIs if the SMMU is suspended and only go ahead with ATC invalidations.

Thus, for most calls, the SMMU driver would make the required changes to
the in-memory data structures, but elide all TLBIs, CFGIs and prefetches
This is done by introducing another runtime PM helper based on
pm_runtime_get_if_active.

Only places where the driver does a pm_runtime_resume_and_get is where
touching the HW is unavoidable and important commands like ATC_INV and
other commands resuming stalled transactions.

Future Work / Potential Improvements
Draining queues could be slightly optimized based on the feature-set
supported by the smmu, for e.g. if STALL isn't supported or ATS is
disabled, we could avoid draining evtq and priq respectivey.

Additionally, for tracking insecure attachments, an rbtee/hashtable
could be used to improve search times, subject to finding a way to
generate IDs / keys for pointers.

Call for review
Any insights/comments on the proposed changes are appreciated,
especially in areas related to locking, atomic contexts, early resume, 
PCIe-related considerations etc. or any other potential optimizations.

Note: The series isn't tested with MSIs and weakly tested for PCIe
clients. The same holds true for tegra241_cmdv changes. Any help in
reviewing and testing these parts is appreciated.

Changelog:

[v2]
 - Introduced `arm_smmu_rpm_get_if_active` for eliding TLBIs & CFGIs
 - Updated the rpm helper invocation strategy.
 - Drained all queues in suspend callback (including tegra241-cmdv)
 - Cache and restore msi_msg instead of free-ing realloc-ing on resume
 - Added support to identify and track user-space attachments
 - Fixed the setup_irqs as per Nicolin & Mostafa's suggestions
 - Used force_runtime_suspend/resume instead as per Mostafa's suggestion.
 - Added "Reviewed-by" line from Mostafa on an unchanged patch

[v1]
 - https://lore.kernel.org/all/20250319004254.2547950-1-praan@google.com/

Pranjal Shrivastava (10):
  iommu/arm-smmu-v3: Refactor arm_smmu_setup_irqs
  iommu/arm-smmu-v3: Add a helper to drain all queues
  iommu/tegra241-cmdqv: Add a helper to drain VCMDQs
  iommu/arm-smmu-v3: Cache and restore MSI config
  iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops
  iommu: Add a helper to check for user ownership
  iommu/arm-smmu-v3: Track masters with user-owned groups
  iommu/arm-smmu-v3: Avoid suspend when user owns DMA
  iommu/arm-smmu-v3: Enable pm_runtime and setup devlinks
  iommu/arm-smmu-v3: Invoke pm_runtime before hw access

 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     |  11 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 468 ++++++++++++++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  19 +
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    |  28 ++
 drivers/iommu/iommu.c                         |  19 +
 include/linux/iommu.h                         |   1 +
 6 files changed, 513 insertions(+), 33 deletions(-)

-- 
2.49.0.805.g082f7c87e0-goog

[RFC PATCH v2 01/10] iommu/arm-smmu-v3: Refactor arm_smmu_setup_irqs

[Pranjal Shrivastava]

Refactor arm_smmu_setup_irqs by splitting it into two parts, one for
registering interrupt handlers and the other one for enabling interrupt
generation in the hardware. This refactor helps in re-initialization of
hardware interrupts as part of a subsequent patch that enables runtime
power management for the arm-smmu-v3 driver.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 54 ++++++++++++++-------
 1 file changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index b4c21aaed126..c295837b20a1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -4009,12 +4009,24 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
 	}
 }
 
-static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
+static void arm_smmu_enable_irqs(struct arm_smmu_device *smmu)
 {
-	int ret, irq;
+	int ret;
 	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
 
-	/* Disable IRQs first */
+	if (smmu->features & ARM_SMMU_FEAT_PRI)
+		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
+
+	ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
+				      ARM_SMMU_IRQ_CTRL, ARM_SMMU_IRQ_CTRLACK);
+	if (ret)
+		dev_warn(smmu->dev, "failed to enable irqs\n");
+}
+
+static int arm_smmu_disable_irqs(struct arm_smmu_device *smmu)
+{
+	int ret;
+
 	ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
 				      ARM_SMMU_IRQ_CTRLACK);
 	if (ret) {
@@ -4022,6 +4034,19 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 		return ret;
 	}
 
+	return 0;
+}
+
+static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
+{
+	int ret, irq;
+
+	/* Disable IRQs first */
+	ret = arm_smmu_disable_irqs(smmu);
+
+	if (ret)
+		return ret;
+
 	irq = smmu->combined_irq;
 	if (irq) {
 		/*
@@ -4038,15 +4063,6 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 	} else
 		arm_smmu_setup_unique_irqs(smmu);
 
-	if (smmu->features & ARM_SMMU_FEAT_PRI)
-		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
-
-	/* Enable interrupt generation on the SMMU */
-	ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
-				      ARM_SMMU_IRQ_CTRL, ARM_SMMU_IRQ_CTRLACK);
-	if (ret)
-		dev_warn(smmu->dev, "failed to enable irqs\n");
-
 	return 0;
 }
 
@@ -4189,11 +4205,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
 		}
 	}
 
-	ret = arm_smmu_setup_irqs(smmu);
-	if (ret) {
-		dev_err(smmu->dev, "failed to setup irqs\n");
-		return ret;
-	}
+	/* Enable interrupt generation on the SMMU */
+	arm_smmu_enable_irqs(smmu);
 
 	if (is_kdump_kernel())
 		enables &= ~(CR0_EVTQEN | CR0_PRIQEN);
@@ -4778,6 +4791,13 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	/* Check for RMRs and install bypass STEs if any */
 	arm_smmu_rmr_install_bypass_ste(smmu);
 
+	/* Setup interrupt handlers */
+	ret = arm_smmu_setup_irqs(smmu);
+	if (ret) {
+		dev_err(smmu->dev, "failed to setup irqs\n");
+		goto err_free_iopf;
+	}
+
 	/* Reset the device */
 	ret = arm_smmu_device_reset(smmu);
 	if (ret)
-- 
2.49.0.805.g082f7c87e0-goog

Re: [RFC PATCH v2 01/10] iommu/arm-smmu-v3: Refactor arm_smmu_setup_irqs

[Nicolin Chen]

On Fri, Apr 18, 2025 at 11:34:00PM +0000, Pranjal Shrivastava wrote:
> Refactor arm_smmu_setup_irqs by splitting it into two parts, one for
> registering interrupt handlers and the other one for enabling interrupt
> generation in the hardware. This refactor helps in re-initialization of
> hardware interrupts as part of a subsequent patch that enables runtime
> power management for the arm-smmu-v3 driver.
> 
> Signed-off-by: Pranjal Shrivastava <praan@google.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

> +static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> +{
> +	int ret, irq;
> +
> +	/* Disable IRQs first */
> +	ret = arm_smmu_disable_irqs(smmu);
> +
> +	if (ret)

Could remove the extra line?

[RFC PATCH v2 02/10] iommu/arm-smmu-v3: Add a helper to drain all queues

[Pranjal Shrivastava]

Before we suspend SMMU, we want to ensure that all commands, events and
paging requests have been flushed by all the queues i.e. the queues are
empty. Add a helper function that waits till the queues are dained.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 46 +++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index c295837b20a1..58bfcfb0d04c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -981,6 +981,52 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,
 					   cmds->num, true);
 }
 
+static int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
+					   struct arm_smmu_queue *q)
+{
+	struct arm_smmu_queue_poll qp;
+	struct arm_smmu_ll_queue *llq = &q->llq;
+	int ret = 0;
+
+	queue_poll_init(smmu, &qp);
+	llq->val = READ_ONCE(q->llq.val);
+	do {
+		if (queue_empty(llq))
+			break;
+
+		ret = queue_poll(&qp);
+		llq->cons = readl(q->cons_reg);
+	} while (!ret);
+
+	return ret;
+}
+
+static int arm_smmu_drain_queues(struct arm_smmu_device *smmu)
+{
+	int ret;
+
+	/* cmdq */
+	arm_smmu_cmdq_shared_lock(&smmu->cmdq);
+	ret = arm_smmu_queue_poll_until_empty(smmu, &smmu->cmdq.q);
+	arm_smmu_cmdq_shared_unlock(&smmu->cmdq);
+	if (ret)
+		return ret;
+
+	/* evtq */
+	ret = arm_smmu_queue_poll_until_empty(smmu, &smmu->evtq.q);
+	if (ret)
+		return ret;
+
+	/* priq */
+	if ((smmu->features & ARM_SMMU_FEAT_PRI)) {
+		ret = arm_smmu_queue_poll_until_empty(smmu, &smmu->priq.q);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
 				   struct iommu_page_response *resp)
 {
-- 
2.49.0.805.g082f7c87e0-goog

Re: [RFC PATCH v2 02/10] iommu/arm-smmu-v3: Add a helper to drain all queues

[Nicolin Chen]

On Fri, Apr 18, 2025 at 11:34:01PM +0000, Pranjal Shrivastava wrote:
> +static int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
> +					   struct arm_smmu_queue *q)
> +{
> +	struct arm_smmu_queue_poll qp;
> +	struct arm_smmu_ll_queue *llq = &q->llq;
> +	int ret = 0;
> +
> +	queue_poll_init(smmu, &qp);
> +	llq->val = READ_ONCE(q->llq.val);

llq points to q->llq, but here it overwrites the value from itself?

Nicolin

Re: [RFC PATCH v2 02/10] iommu/arm-smmu-v3: Add a helper to drain all queues

[Pranjal Shrivastava]

On Fri, May 02, 2025 at 12:32:35PM -0700, Nicolin Chen wrote:
> On Fri, Apr 18, 2025 at 11:34:01PM +0000, Pranjal Shrivastava wrote:
> > +static int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
> > +					   struct arm_smmu_queue *q)
> > +{
> > +	struct arm_smmu_queue_poll qp;
> > +	struct arm_smmu_ll_queue *llq = &q->llq;
> > +	int ret = 0;
> > +
> > +	queue_poll_init(smmu, &qp);
> > +	llq->val = READ_ONCE(q->llq.val);
> 
> llq points to q->llq, but here it overwrites the value from itself?

Ack. This seems redundant, will remove.

> 
> Nicolin

Thanks
Praan

Re: [RFC PATCH v2 02/10] iommu/arm-smmu-v3: Add a helper to drain all queues

[Daniel Mentz]

On Fri, Apr 18, 2025 at 4:34 PM Pranjal Shrivastava <praan@google.com> wrote:
> +static int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
> +                                          struct arm_smmu_queue *q)
> +{
> +       struct arm_smmu_queue_poll qp;
> +       struct arm_smmu_ll_queue *llq = &q->llq;
> +       int ret = 0;
> +
> +       queue_poll_init(smmu, &qp);
> +       llq->val = READ_ONCE(q->llq.val);
> +       do {
> +               if (queue_empty(llq))
> +                       break;
> +
> +               ret = queue_poll(&qp);
> +               llq->cons = readl(q->cons_reg);

 A readl_relaxed() is probably sufficient. Also, in other places, they
use WRITE_ONCE to write to llq->cons. Compare with

WRITE_ONCE(cmdq->q.llq.cons, readl_relaxed(cmdq->q.cons_reg));

in arm_smmu_cmdq_poll_until_not_full.

> +       } while (!ret);
> +
> +       return ret;
> +}
> +
> +static int arm_smmu_drain_queues(struct arm_smmu_device *smmu)
> +{
> +       int ret;
> +
> +       /* cmdq */
> +       arm_smmu_cmdq_shared_lock(&smmu->cmdq);

I'm afraid this might not be a correct usage
arm_smmu_cmdq_shared_lock. I understand that acquiring the shared lock
means that you want to prevent any other thread from updating
cmdq->q.llq.cons, which I believe is not what you're looking for here.
If you want to participate in this locking protocol, I believe you
need to acquire the exclusive lock.

This being said, I believe you need some other mechanism to ensure
that no new commands are submitted to the command queue. Otherwise,
the command queue could become non-empty after you return from
arm_smmu_queue_poll_until_empty.

>
> +       ret = arm_smmu_queue_poll_until_empty(smmu, &smmu->cmdq.q);
> +       arm_smmu_cmdq_shared_unlock(&smmu->cmdq);
> +       if (ret)
> +               return ret;

This is basically a command queue timeout. Consider printing an error
message similar to "CMD_SYNC timeout at [...]" in
arm_smmu_cmdq_issue_cmdlist.

> +
> +       /* evtq */
> +       ret = arm_smmu_queue_poll_until_empty(smmu, &smmu->evtq.q);

I'm not sure if you can use your arm_smmu_queue_poll_until_empty
function on the event queue. I understand that in the case of the
event queue where the driver is the consumer, the driver's version of
llq->cons is always more up-to-date than the SMMU_EVTQ_CONS register.
In fact, I'm afraid you might move the consumer index backwards with
"llq->cons = readl(q->cons_reg);", because llq->cons is more current
than SMMU_EVTQ_CONS.

I'm thinking about the following proposal:
 * Drain only the command queue. Use a local copy for llq->cons. Don't
write to cmdq->q.cons_reg
 * Don't bother draining the other queues
 * Backup consumer and producer indices *after* disabling the queues
in SMMU_CR0. This would involve writing to cmdq->q.cons_reg

Re: [RFC PATCH v2 02/10] iommu/arm-smmu-v3: Add a helper to drain all queues

[Pranjal Shrivastava]

On Sun, May 04, 2025 at 01:28:13PM -0700, Daniel Mentz wrote:
> On Fri, Apr 18, 2025 at 4:34 PM Pranjal Shrivastava <praan@google.com> wrote:
> > +static int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
> > +                                          struct arm_smmu_queue *q)
> > +{
> > +       struct arm_smmu_queue_poll qp;
> > +       struct arm_smmu_ll_queue *llq = &q->llq;
> > +       int ret = 0;
> > +
> > +       queue_poll_init(smmu, &qp);
> > +       llq->val = READ_ONCE(q->llq.val);
> > +       do {
> > +               if (queue_empty(llq))
> > +                       break;
> > +
> > +               ret = queue_poll(&qp);
> > +               llq->cons = readl(q->cons_reg);
> 
>  A readl_relaxed() is probably sufficient. 

Ack. I referred to __arm_smmu_cmdq_poll_until_consumed() which uses
readl, but I agree that this can be _relaxed as for the former we had to
handle a case where multiple threads are polling on their own CMD_SYNCs.

> Also, in other places, they
> use WRITE_ONCE to write to llq->cons. Compare with
> 
> WRITE_ONCE(cmdq->q.llq.cons, readl_relaxed(cmdq->q.cons_reg));
> 
> in arm_smmu_cmdq_poll_until_not_full.
> 

Yes, I believe that's to avoid store-tearing and re-ordering when we
have multiple threads potentially polling on the llq.cons

However, I don't think that's going to be the case in the suspend
callback as we'd atmost have the page_response responding to a stall
event and issuing a CMD_RESUME (without a sync) which won't poll.

Although, if we're looking to use this function in other context apart
from suspend, I guess there's not much of a harm in using WRITE_ONCE()..

> > +       } while (!ret);
> > +
> > +       return ret;
> > +}
> > +
> > +static int arm_smmu_drain_queues(struct arm_smmu_device *smmu)
> > +{
> > +       int ret;
> > +
> > +       /* cmdq */
> > +       arm_smmu_cmdq_shared_lock(&smmu->cmdq);
> 
> I'm afraid this might not be a correct usage
> arm_smmu_cmdq_shared_lock. I understand that acquiring the shared lock
> means that you want to prevent any other thread from updating
> cmdq->q.llq.cons, which I believe is not what you're looking for here.
> If you want to participate in this locking protocol, I believe you
> need to acquire the exclusive lock.
> 

Ack. 
However, I believe there's no such need to acquire any lock here given
the fact that `arm_smmu_drain_queues` isn't expected to be called from
anywhere else except the suspend callback... What do you think ?

The only case that I can think of that could possibly contend with this
is the evtq_thread handling a stalled transaction with a CMD_RESUME
which could be handled by first draining the evtq.

The callers which plan to use arm_smmu_cmdq_poll_until_empty apart from
this could use the appropriate locking, I can add a comment about that.

> This being said, I believe you need some other mechanism to ensure
> that no new commands are submitted to the command queue. Otherwise,
> the command queue could become non-empty after you return from
> arm_smmu_queue_poll_until_empty.
> 

I think the fact that we're calling this from the suspend callback
itself ensures that there aren't any clients actively issuing commands?

The only exception to this could be the other two queues having events
which require to be responded with either CMD_RESUME or CMD_PRI_RESP.
For example: a stall event that requires us to respond with a CMD_RESUME.

We could drain the other queues before the cmdq in that case.

> >
> > +       ret = arm_smmu_queue_poll_until_empty(smmu, &smmu->cmdq.q);
> > +       arm_smmu_cmdq_shared_unlock(&smmu->cmdq);
> > +       if (ret)
> > +               return ret;
> 
> This is basically a command queue timeout. Consider printing an error
> message similar to "CMD_SYNC timeout at [...]" in
> arm_smmu_cmdq_issue_cmdlist.
> 

We're already printing that in the arm_smmu_runtime_suspend:
	dev_err(smmu->dev, "Draining queues timed-out.. retry later\n");

Are you suggesting to print it specifically for cmdq with the prod/cons?

> > +
> > +       /* evtq */
> > +       ret = arm_smmu_queue_poll_until_empty(smmu, &smmu->evtq.q);
> 
> I'm not sure if you can use your arm_smmu_queue_poll_until_empty
> function on the event queue. I understand that in the case of the
> event queue where the driver is the consumer, the driver's version of
> llq->cons is always more up-to-date than the SMMU_EVTQ_CONS register.
> In fact, I'm afraid you might move the consumer index backwards with
> "llq->cons = readl(q->cons_reg);", because llq->cons is more current
> than SMMU_EVTQ_CONS.
> 

That's a good catch! Thanks! This also true for priq, I think we can
have a dedicated function for draining the cmdq as above and another one
for draining priq and evtq.

> I'm thinking about the following proposal:
>  * Drain only the command queue. Use a local copy for llq->cons. Don't
> write to cmdq->q.cons_reg

We're not writing to cmdq->q.cons_reg anywhere? And we can't rely on the
llq->cons copy since it's ONLY updated when the last command was issued
with a "sync" which isn't guaranteed to be the case always.

>  * Don't bother draining the other queues

I don't think that would be appropriate, we can't leave a stalled
transaction before suspending and should handle all Paging requests via
PRI. In fact in the v1, Robin mentioned[1] quite the opposite. 

>  * Backup consumer and producer indices *after* disabling the queues
> in SMMU_CR0. This would involve writing to cmdq->q.cons_reg

So... basically handle the pending stalls and paging requests when we
resume later? 

Also, why would we write to the cmdq->q.cons_reg if it's going to be
lost with the power-down? If at all, we'd have to read the prod/cons_reg
values back into the SW copies. Am I missing something here?


Thanks,
Praan

[1]
https://lore.kernel.org/all/20250319004254.2547950-4-praan@google.com/T/#m6f479d70d666252a657fb3ff57ff1764267a892d

[RFC PATCH v2 03/10] iommu/tegra241-cmdqv: Add a helper to drain VCMDQs

[Pranjal Shrivastava]

The tegra241-cmdqv driver supports vCMDQs which need to be drained
before suspending the SMMU. The current driver implementation only uses
VINTF0 for vCMDQs owned by the kernel which need to be drained. Add a
helper that drains all the enabled vCMDQs under VINTF0.

Add another function ptr to arm_smmu_impl_ops to drain implementation
specified queues and call it within `arm_smmu_drain_queues`

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 10 +++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  4 +++
 .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 28 +++++++++++++++++++
 3 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 58bfcfb0d04c..7f70e0f4f158 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -981,8 +981,8 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,
 					   cmds->num, true);
 }
 
-static int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
-					   struct arm_smmu_queue *q)
+int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
+				    struct arm_smmu_queue *q)
 {
 	struct arm_smmu_queue_poll qp;
 	struct arm_smmu_ll_queue *llq = &q->llq;
@@ -1024,7 +1024,11 @@ static int arm_smmu_drain_queues(struct arm_smmu_device *smmu)
 			return ret;
 	}
 
-	return 0;
+	/* Drain all implementation-specific queues */
+	if (smmu->impl_ops && smmu->impl_ops->drain_queues)
+		ret = smmu->impl_ops->drain_queues(smmu);
+
+	return ret;
 }
 
 static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index dd1ad56ce863..a950da2e7ed9 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -720,6 +720,7 @@ struct arm_smmu_impl_ops {
 	int (*init_structures)(struct arm_smmu_device *smmu);
 	struct arm_smmu_cmdq *(*get_secondary_cmdq)(
 		struct arm_smmu_device *smmu, struct arm_smmu_cmdq_ent *ent);
+	int (*drain_queues)(struct arm_smmu_device *smmu);
 };
 
 /* An SMMUv3 instance */
@@ -965,6 +966,9 @@ int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
 int arm_smmu_cmdq_init(struct arm_smmu_device *smmu,
 		       struct arm_smmu_cmdq *cmdq);
 
+int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
+				    struct arm_smmu_queue *q);
+
 static inline bool arm_smmu_master_canwbs(struct arm_smmu_master *master)
 {
 	return dev_iommu_fwspec_get(master->dev)->flags &
diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
index dd7d030d2e89..767275e10d43 100644
--- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
+++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
@@ -349,6 +349,33 @@ tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu,
 	return &vcmdq->cmdq;
 }
 
+static int tegra241_cmdqv_drain_hyp_vcmdqs(struct arm_smmu_device *smmu)
+{
+	struct tegra241_cmdqv *cmdqv =
+		container_of(smmu, struct tegra241_cmdqv, smmu);
+	struct tegra241_vintf *vintf = cmdqv->vintfs[0];
+	struct tegra241_vcmdq *vcmdq;
+	int ret = 0;
+	u16 i;
+
+	/* Kernel only uses VINTF0 return if it's disabled */
+	if (!READ_ONCE(vintf->enabled))
+		return 0;
+
+	for (i = 0; i < cmdqv->num_lvcmdqs_per_vintf; i++) {
+		vcmdq = vintf->lvcmdqs[i];
+
+		if (!vcmdq || !READ_ONCE(vcmdq->enabled))
+			continue;
+
+		ret = arm_smmu_queue_poll_until_empty(smmu, &vcmdq->cmdq.q);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
 /* HW Reset Functions */
 
 static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq)
@@ -681,6 +708,7 @@ static struct arm_smmu_impl_ops tegra241_cmdqv_impl_ops = {
 	.get_secondary_cmdq = tegra241_cmdqv_get_cmdq,
 	.device_reset = tegra241_cmdqv_hw_reset,
 	.device_remove = tegra241_cmdqv_remove,
+	.drain_queues = tegra241_cmdqv_drain_hyp_vcmdqs,
 };
 
 /* Probe Functions */
-- 
2.49.0.805.g082f7c87e0-goog

Re: [RFC PATCH v2 03/10] iommu/tegra241-cmdqv: Add a helper to drain VCMDQs

[Nicolin Chen]

On Fri, Apr 18, 2025 at 11:34:02PM +0000, Pranjal Shrivastava wrote:
> The tegra241-cmdqv driver supports vCMDQs which need to be drained
> before suspending the SMMU. The current driver implementation only uses
> VINTF0 for vCMDQs owned by the kernel which need to be drained. Add a
> helper that drains all the enabled vCMDQs under VINTF0.
> 
> Add another function ptr to arm_smmu_impl_ops to drain implementation
> specified queues and call it within `arm_smmu_drain_queues`
> 
> Signed-off-by: Pranjal Shrivastava <praan@google.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

Some nitpickings below. No need to reply unless you disagree :)

> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 10 +++++--
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  4 +++
>  .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c    | 28 +++++++++++++++++++
>  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 58bfcfb0d04c..7f70e0f4f158 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -981,8 +981,8 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,
>  					   cmds->num, true);
>  }
>  
> -static int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
> -					   struct arm_smmu_queue *q)
> +int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
> +				    struct arm_smmu_queue *q)
> @@ -965,6 +966,9 @@ int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>  int arm_smmu_cmdq_init(struct arm_smmu_device *smmu,
>  		       struct arm_smmu_cmdq *cmdq);
>  
> +int arm_smmu_queue_poll_until_empty(struct arm_smmu_device *smmu,
> +				    struct arm_smmu_queue *q);
> +

I'd personally prefer these to stay in the change that introduces
the function, since it knew that a following patch wants it to be
shared in the header. But either way should be fine.

>  static inline bool arm_smmu_master_canwbs(struct arm_smmu_master *master)
>  {
>  	return dev_iommu_fwspec_get(master->dev)->flags &
> diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> index dd7d030d2e89..767275e10d43 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
> @@ -349,6 +349,33 @@ tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu,
>  	return &vcmdq->cmdq;
>  }
>  
> +static int tegra241_cmdqv_drain_hyp_vcmdqs(struct arm_smmu_device *smmu)

tegra241_cmdqv_drain_vintf0_lvcmdqs

> +{
> +	struct tegra241_cmdqv *cmdqv =
> +		container_of(smmu, struct tegra241_cmdqv, smmu);
> +	struct tegra241_vintf *vintf = cmdqv->vintfs[0];
> +	struct tegra241_vcmdq *vcmdq;

Let's move into the for loop.

> +	int ret = 0;
> +	u16 i;

u16 lidx;

> +
> +	/* Kernel only uses VINTF0 return if it's disabled */

	/* Kernel only uses VINTF0. Return if it's disabled */

> +	if (!READ_ONCE(vintf->enabled))
> +		return 0;
> +
> +	for (i = 0; i < cmdqv->num_lvcmdqs_per_vintf; i++) {
> +		vcmdq = vintf->lvcmdqs[i];

	for (lidx = 0; lidx < cmdqv->num_lvcmdqs_per_vintf; lidx++) {
		struct tegra241_vcmdq *vcmdq = vintf->lvcmdqs[i];

[RFC PATCH v2 04/10] iommu/arm-smmu-v3: Cache and restore MSI config

[Pranjal Shrivastava]

The SMMU's MSI configuration registers (*_IRQ_CFGn) containing target
address, data and memory attributes lose their state when the SMMU is
powered down. We'll need to cache and restore their contents to ensure
that MSIs work after the system resumes.

To address this, cache the original `msi_msg` within the `msi_desc`
when the configuration is first written by `arm_smmu_write_msi_msg`.
This primarily includes the target address and data since the memory
attributes are fixed.

Introduce a new helper `arm_smmu_resume_msis` which will later be called
during the driver's resume callback. The helper would retrieve the
cached MSI message for each relevant interrupt (evtq, gerr, priq) via
get_cached_msi_msg & re-config the registers via arm_smmu_write_msi_msg

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 41 +++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 7f70e0f4f158..7990865059e4 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3968,6 +3968,9 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
 	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
 	phys_addr_t *cfg = arm_smmu_msi_cfg[desc->msi_index];
 
+	/* Cache the msi_msg for resume */
+	desc->msg = *msg;
+
 	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
 	doorbell &= MSI_CFG0_ADDR_MASK;
 
@@ -3976,6 +3979,44 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
 	writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
 }
 
+static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
+{
+	struct msi_desc *desc;
+	struct msi_msg msg;
+
+	if (!(smmu->features & ARM_SMMU_FEAT_MSI))
+		return;
+
+	if (!smmu->dev->msi.domain)
+		return;
+
+	desc = irq_get_msi_desc(smmu->evtq.q.irq);
+	if (desc) {
+		get_cached_msi_msg(smmu->evtq.q.irq, &msg);
+		arm_smmu_write_msi_msg(desc, &msg);
+	} else {
+		dev_err(smmu->dev, "Failed to resume evtq msi");
+	}
+
+	desc = irq_get_msi_desc(smmu->gerr_irq);
+	if (desc) {
+		get_cached_msi_msg(smmu->gerr_irq, &msg);
+		arm_smmu_write_msi_msg(desc, &msg);
+	} else {
+		dev_err(smmu->dev, "Failed to resume gerror msi");
+	}
+
+	if (smmu->features & ARM_SMMU_FEAT_PRI) {
+		desc = irq_get_msi_desc(smmu->priq.q.irq);
+		if (desc) {
+			get_cached_msi_msg(smmu->priq.q.irq, &msg);
+			arm_smmu_write_msi_msg(desc, &msg);
+		} else {
+			dev_err(smmu->dev, "Failed to resume priq msi");
+		}
+	}
+}
+
 static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
 {
 	int ret, nvec = ARM_SMMU_MAX_MSIS;
-- 
2.49.0.805.g082f7c87e0-goog

Re: [RFC PATCH v2 04/10] iommu/arm-smmu-v3: Cache and restore MSI config

[Nicolin Chen]

On Fri, Apr 18, 2025 at 11:34:03PM +0000, Pranjal Shrivastava wrote:
> +	desc = irq_get_msi_desc(smmu->evtq.q.irq);
> +	if (desc) {
> +		get_cached_msi_msg(smmu->evtq.q.irq, &msg);
> +		arm_smmu_write_msi_msg(desc, &msg);
> +	} else {
> +		dev_err(smmu->dev, "Failed to resume evtq msi");
> +	}

Do we need to check if (smmu->evtq.q.irq) at the top?

And same question to the other irqs.

Thanks
Nicolin

Re: [RFC PATCH v2 04/10] iommu/arm-smmu-v3: Cache and restore MSI config

[Pranjal Shrivastava]

On Fri, May 02, 2025 at 12:43:03PM -0700, Nicolin Chen wrote:
> On Fri, Apr 18, 2025 at 11:34:03PM +0000, Pranjal Shrivastava wrote:
> > +	desc = irq_get_msi_desc(smmu->evtq.q.irq);
> > +	if (desc) {
> > +		get_cached_msi_msg(smmu->evtq.q.irq, &msg);
> > +		arm_smmu_write_msi_msg(desc, &msg);
> > +	} else {
> > +		dev_err(smmu->dev, "Failed to resume evtq msi");
> > +	}
> 
> Do we need to check if (smmu->evtq.q.irq) at the top?
> 
> And same question to the other irqs.
> 

Yes, that's right, we should check if the irqs are assigned. Thanks!

> Thanks
> Nicolin

Praan

[RFC PATCH v2 05/10] iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops

[Pranjal Shrivastava]

Implement pm_runtime and system sleep ops for arm-smmu-v3. The smmu is
disabled as part of the suspend callbacks after ensuring the completion
of all pending commands and is configured to abort any transactions that
happen after disabling the smmu. The smmu shall be reinitialized in the
resume callback by invoking the `arm_smmu_device_reset` helper.

The MSIs are freed as part of the suspend and are re-allocated during
the resume operation.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 93 +++++++++++++++++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 +
 2 files changed, 95 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 7990865059e4..3052ce1a419a 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -26,6 +26,7 @@
 #include <linux/pci.h>
 #include <linux/pci-ats.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/string_choices.h>
 #include <kunit/visibility.h>
 #include <uapi/linux/iommufd.h>
@@ -108,6 +109,41 @@ static const char * const event_class_str[] = {
 
 static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
 
+/* Runtime PM helpers */
+static int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
+{
+	int ret;
+
+	if (pm_runtime_enabled(smmu->dev)) {
+		ret = pm_runtime_resume_and_get(smmu->dev);
+		if (ret < 0) {
+			dev_err(smmu->dev, "Failed to resume device: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int arm_smmu_rpm_get_if_active(struct arm_smmu_device *smmu)
+{
+	if (pm_runtime_enabled(smmu->dev))
+		return pm_runtime_get_if_in_use(smmu->dev);
+
+	return 0;
+}
+
+static void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
+{
+	int ret;
+
+	if (pm_runtime_enabled(smmu->dev)) {
+		ret = pm_runtime_put_autosuspend(smmu->dev);
+		if (ret < 0)
+			dev_err(smmu->dev, "Failed to suspend device: %d\n", ret);
+	}
+}
+
 static void parse_driver_options(struct arm_smmu_device *smmu)
 {
 	int i = 0;
@@ -4935,6 +4971,62 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
 	arm_smmu_device_disable(smmu);
 }
 
+static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
+{
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+	int ret;
+
+	/*
+	 * Since suspend is invoked when all clients have been suspended,
+	 * we don't expect more cmds or events to be added to the queues.
+	 * Wait for all queues to be drained.
+	 */
+	ret = arm_smmu_drain_queues(smmu);
+	if (ret) {
+		dev_err(smmu->dev, "Draining queues timed-out.. retry later\n");
+		return -EAGAIN;
+	}
+
+	/* Disable all queues */
+	arm_smmu_device_disable(smmu);
+
+	/* Abort all transactions to avoid spurious bypass */
+	arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);
+
+	dev_dbg(dev, "Suspending smmu\n");
+	return 0;
+}
+
+static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
+{
+	int ret;
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "Resuming device\n");
+
+	/* Re-configure MSIs */
+	arm_smmu_resume_msis(smmu);
+
+	/*
+	 * The reset will re-initialize all the base addresses, queues,
+	 * prod and cons maintained within struct arm_smmu_device as well as
+	 * re-enable the interrupts.
+	 */
+	ret = arm_smmu_device_reset(smmu);
+
+	if (ret)
+		dev_err(dev, "Failed to reset during resume operation: %d\n", ret);
+
+	return ret;
+}
+
+static const struct dev_pm_ops arm_smmu_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(arm_smmu_runtime_suspend,
+			   arm_smmu_runtime_resume, NULL)
+};
+
 static const struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v3", },
 	{ },
@@ -4951,6 +5043,7 @@ static struct platform_driver arm_smmu_driver = {
 	.driver	= {
 		.name			= "arm-smmu-v3",
 		.of_match_table		= arm_smmu_of_match,
+		.pm                     = &arm_smmu_pm_ops,
 		.suppress_bind_attrs	= true,
 	},
 	.probe	= arm_smmu_device_probe,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index a950da2e7ed9..d4ac6adc6a30 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -506,6 +506,8 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
 #define MSI_IOVA_BASE			0x8000000
 #define MSI_IOVA_LENGTH			0x100000
 
+#define RPM_AUTOSUSPEND_DELAY_MS	15
+
 enum pri_resp {
 	PRI_RESP_DENY = 0,
 	PRI_RESP_FAIL = 1,
-- 
2.49.0.805.g082f7c87e0-goog

Re: [RFC PATCH v2 05/10] iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops

[Daniel Mentz]

On Fri, Apr 18, 2025 at 4:35 PM Pranjal Shrivastava <praan@google.com> wrote:
> +static int arm_smmu_rpm_get_if_active(struct arm_smmu_device *smmu)
> +{
> +       if (pm_runtime_enabled(smmu->dev))
> +               return pm_runtime_get_if_in_use(smmu->dev);

You named this function arm_smmu_rpm_get_if_active but then call into
pm_runtime_get_if_in_use (There's also a pm_runtime_get_if_active).
Shouldn't this function be called arm_smmu_rpm_get_if_in_use?

> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -506,6 +506,8 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
>  #define MSI_IOVA_BASE                  0x8000000
>  #define MSI_IOVA_LENGTH                        0x100000
>
> +#define RPM_AUTOSUSPEND_DELAY_MS       15

Consider moving this to  "iommu/arm-smmu-v3: Enable pm_runtime and
setup devlinks" where it's actually used for the first time.

Re: [RFC PATCH v2 05/10] iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops

[Pranjal Shrivastava]

On Sun, May 04, 2025 at 01:29:36PM -0700, Daniel Mentz wrote:
> On Fri, Apr 18, 2025 at 4:35 PM Pranjal Shrivastava <praan@google.com> wrote:
> > +static int arm_smmu_rpm_get_if_active(struct arm_smmu_device *smmu)
> > +{
> > +       if (pm_runtime_enabled(smmu->dev))
> > +               return pm_runtime_get_if_in_use(smmu->dev);
> 
> You named this function arm_smmu_rpm_get_if_active but then call into
> pm_runtime_get_if_in_use (There's also a pm_runtime_get_if_active).
> Shouldn't this function be called arm_smmu_rpm_get_if_in_use?
> 

Ack, I'll rename the function to arm_smmu_rpm_get_if_in_use to be more
clear.

> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -506,6 +506,8 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
> >  #define MSI_IOVA_BASE                  0x8000000
> >  #define MSI_IOVA_LENGTH                        0x100000
> >
> > +#define RPM_AUTOSUSPEND_DELAY_MS       15
> 
> Consider moving this to  "iommu/arm-smmu-v3: Enable pm_runtime and
> setup devlinks" where it's actually used for the first time.

Ack.

Thanks
Praan

[RFC PATCH v2 06/10] iommu: Add a helper to check for user ownership

[Pranjal Shrivastava]

The existing function iommu_group_dma_owner_claimed() checks if DMA
ownership for a group has been claimed, but it cannot distinguish
between kernel clients and user-space clients like VFIO. This is because
its underlying mechanism (based on group->owner_cnt) can reflect claims
from both user-space (e.g. iommu_group_claim_dma_owner()) and kernel
clients like iommu_device_use_default_domain() during really_probe.

Introduce a new helper iommu_group_dma_owner_user() which allows the
callers to specifically determine if a iommu_group is owned by the
user-space.

The function checks the group->owner field which is exclusively managed
by the iommu_group_claim_dma_owner() OR iommu_device_claim_dma_owner(),
instead of relying on the iommu_group->owner_cnt. Both the
*_claim_dma_owner APIs are ONLY called by vfio and iommufd, indicating
user-space's DMA ownership for the said iommu_group.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/iommu.c | 19 +++++++++++++++++++
 include/linux/iommu.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4f91a740c15f..91d520d76e0d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3348,6 +3348,25 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
 }
 EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
 
+/**
+ * iommu_group_dma_owner_kernel() - Query if kernel owns the group dma
+ * @group: The group.
+ *
+ * This provides status query on a given group. It is racy and only for
+ * non-binding status reporting.
+ */
+bool iommu_group_dma_owner_user(struct iommu_group *group)
+{
+	void *user;
+
+	mutex_lock(&group->mutex);
+	user = group->owner;
+	mutex_unlock(&group->mutex);
+
+	return user ? true : false;
+}
+EXPORT_SYMBOL_GPL(iommu_group_dma_owner_user);
+
 static void iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 				   struct iommu_domain *domain)
 {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ccce8a751e2a..57b27c912a9c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1132,6 +1132,7 @@ void iommu_device_unuse_default_domain(struct device *dev);
 int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner);
 void iommu_group_release_dma_owner(struct iommu_group *group);
 bool iommu_group_dma_owner_claimed(struct iommu_group *group);
+bool iommu_group_dma_owner_user(struct iommu_group *group);
 
 int iommu_device_claim_dma_owner(struct device *dev, void *owner);
 void iommu_device_release_dma_owner(struct device *dev);
-- 
2.49.0.805.g082f7c87e0-goog

Re: [RFC PATCH v2 06/10] iommu: Add a helper to check for user ownership

[kernel test robot]

Hi Pranjal,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on arm-perf/for-next/perf]
[also build test WARNING on linus/master v6.15-rc2 next-20250417]
[cannot apply to joro-iommu/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Pranjal-Shrivastava/iommu-arm-smmu-v3-Refactor-arm_smmu_setup_irqs/20250419-073729
base:   https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git for-next/perf
patch link:    https://lore.kernel.org/r/20250418233409.3926715-7-praan%40google.com
patch subject: [RFC PATCH v2 06/10] iommu: Add a helper to check for user ownership
config: csky-randconfig-002-20250419 (https://download.01.org/0day-ci/archive/20250419/202504192106.ned5rxJr-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 11.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250419/202504192106.ned5rxJr-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504192106.ned5rxJr-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iommu/iommu.c:3355: warning: expecting prototype for iommu_group_dma_owner_kernel(). Prototype was for iommu_group_dma_owner_user() instead


vim +3355 drivers/iommu/iommu.c

  3346	
  3347	/**
  3348	 * iommu_group_dma_owner_kernel() - Query if kernel owns the group dma
  3349	 * @group: The group.
  3350	 *
  3351	 * This provides status query on a given group. It is racy and only for
  3352	 * non-binding status reporting.
  3353	 */
  3354	bool iommu_group_dma_owner_user(struct iommu_group *group)
> 3355	{
  3356		void *user;
  3357	
  3358		mutex_lock(&group->mutex);
  3359		user = group->owner;
  3360		mutex_unlock(&group->mutex);
  3361	
  3362		return user ? true : false;
  3363	}
  3364	EXPORT_SYMBOL_GPL(iommu_group_dma_owner_user);
  3365	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[RFC PATCH v2 07/10] iommu/arm-smmu-v3: Track masters with user-owned groups

[Pranjal Shrivastava]

Track smmu masters (client devices) which belong to user-owned IOMMU
groups as determined by `iommu_group_dma_owner_user`. This allows the
driver to identify attachments where DMA is controlled by user-space.

This tracking provides the necessary infrastructure for setting up
power management policies to help decide if it's safe to power-down the
smmu if an attachment belongs to a user-space controlled group.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 69 +++++++++++++++++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 10 +++
 2 files changed, 79 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 3052ce1a419a..2b51665d5ca6 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3018,6 +3018,62 @@ void arm_smmu_attach_commit(struct arm_smmu_attach_state *state)
 	arm_smmu_remove_master_domain(master, state->old_domain, state->ssid);
 }
 
+static int arm_smmu_insert_insecure_master(struct arm_smmu_device *smmu,
+					   struct arm_smmu_master *master)
+{
+	struct arm_smmu_unsafe_master *umaster;
+	unsigned long flags;
+
+	umaster = kzalloc(sizeof(umaster), GFP_KERNEL);
+	if (!umaster)
+		return -ENOMEM;
+	umaster->master = master;
+
+	spin_lock_irqsave(&smmu->attach_lock, flags);
+	list_add_tail(&umaster->masters, &smmu->insecure_attachments);
+	spin_unlock_irqrestore(&smmu->attach_lock, flags);
+
+	return 0;
+}
+
+static struct arm_smmu_master *
+arm_smmu_find_insecure_master(struct arm_smmu_device *smmu,
+			      struct arm_smmu_master *master)
+{
+	struct arm_smmu_unsafe_master *iter, *umaster = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&smmu->attach_lock, flags);
+	list_for_each_entry(iter, &smmu->insecure_attachments, masters) {
+		if (iter->master == master) {
+			umaster = iter;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&smmu->attach_lock, flags);
+
+	return umaster ? umaster->master : NULL;
+}
+
+static void arm_smmu_remove_insecure_master(struct arm_smmu_device *smmu,
+					    struct arm_smmu_master *master)
+{
+	struct arm_smmu_unsafe_master *iter, *tmp, *node_to_free = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&smmu->attach_lock, flags);
+
+	list_for_each_entry_safe(iter, tmp, &smmu->insecure_attachments, masters) {
+		if (iter->master == master) {
+			list_del(&iter->masters);
+			node_to_free = iter;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&smmu->attach_lock, flags);
+	kfree(node_to_free);
+}
+
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
 	int ret = 0;
@@ -3041,6 +3097,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	if (smmu_domain->smmu != smmu)
 		return ret;
 
+	/* Check if the master is owned by user */
+	if (iommu_group_dma_owner_user(dev->iommu_group)) {
+		if (!arm_smmu_find_insecure_master(smmu, master))
+			arm_smmu_insert_insecure_master(smmu, master);
+
+	} else if (arm_smmu_find_insecure_master(smmu, master)) {
+		/* Remove if the master isn't user-owned anymore */
+		arm_smmu_remove_insecure_master(smmu, master);
+	}
+
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
 		cdptr = arm_smmu_alloc_cd_ptr(master, IOMMU_NO_PASID);
 		if (!cdptr)
@@ -3944,6 +4010,9 @@ static int arm_smmu_init_structures(struct arm_smmu_device *smmu)
 	mutex_init(&smmu->streams_mutex);
 	smmu->streams = RB_ROOT;
 
+	INIT_LIST_HEAD(&smmu->insecure_attachments);
+	spin_lock_init(&smmu->attach_lock);
+
 	ret = arm_smmu_init_queues(smmu);
 	if (ret)
 		return ret;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index d4ac6adc6a30..5ab60ae7ff91 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -795,6 +795,11 @@ struct arm_smmu_device {
 
 	struct rb_root			streams;
 	struct mutex			streams_mutex;
+
+	/* Insecure Attach handles */
+	struct list_head		insecure_attachments;
+	/* Lock for the list */
+	spinlock_t			attach_lock;
 };
 
 struct arm_smmu_stream {
@@ -803,6 +808,11 @@ struct arm_smmu_stream {
 	struct rb_node			node;
 };
 
+struct arm_smmu_unsafe_master {
+	struct list_head		masters;
+	struct arm_smmu_master		*master;
+};
+
 struct arm_smmu_vmaster {
 	struct arm_vsmmu		*vsmmu;
 	unsigned long			vsid;
-- 
2.49.0.805.g082f7c87e0-goog

[RFC PATCH v2 08/10] iommu/arm-smmu-v3: Avoid suspend when user owns DMA

[Pranjal Shrivastava]

Suspending the smmu is unsafe if any attached devices belong
to iommu groups claimed by user-space (via VFIO etc.), as a DMA may
be ongoing or initiated at any time. The smmu must remain powered ON to
handle potential DMA requests originating from the user-space context.

Introduce a helper `arm_smmu_suspend_is_safe()` using the previously
added `insecure_attachments` list. If the check fails (list is not empty),
defer the suspend ensuring the SMMU remains active while it's potentially
needed by user-space for DMA.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 ++++++++++++++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 2b51665d5ca6..7099c0cbf5fe 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -5040,11 +5040,29 @@ static void arm_smmu_device_shutdown(struct platform_device *pdev)
 	arm_smmu_device_disable(smmu);
 }
 
+static bool arm_smmu_suspend_is_safe(struct arm_smmu_device *smmu)
+{
+	unsigned long flags;
+	bool is_empty;
+
+	spin_lock_irqsave(&smmu->attach_lock, flags);
+	is_empty = list_empty(&smmu->insecure_attachments);
+	spin_unlock_irqrestore(&smmu->attach_lock, flags);
+
+	return is_empty;
+}
+
 static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
 {
 	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
 	int ret;
 
+	/* Refuse to suspend if user-space controls DMA */
+	if (!arm_smmu_suspend_is_safe(smmu)) {
+		dev_err(smmu->dev, "Suspend deferred due to unsafe DMA owners\n");
+		return -EAGAIN;
+	}
+
 	/*
 	 * Since suspend is invoked when all clients have been suspended,
 	 * we don't expect more cmds or events to be added to the queues.
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 5ab60ae7ff91..28fddc07ab49 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -796,7 +796,7 @@ struct arm_smmu_device {
 	struct rb_root			streams;
 	struct mutex			streams_mutex;
 
-	/* Insecure Attach handles */
+	/* Insecure attach handles */
 	struct list_head		insecure_attachments;
 	/* Lock for the list */
 	spinlock_t			attach_lock;
-- 
2.49.0.805.g082f7c87e0-goog

Re: [RFC PATCH v2 08/10] iommu/arm-smmu-v3: Avoid suspend when user owns DMA

[Daniel Mentz]

On Fri, Apr 18, 2025 at 4:35 PM Pranjal Shrivastava <praan@google.com> wrote:
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -796,7 +796,7 @@ struct arm_smmu_device {
>         struct rb_root                  streams;
>         struct mutex                    streams_mutex;
>
> -       /* Insecure Attach handles */
> +       /* Insecure attach handles */

Move this to the original commit that introduced this comment.



>         struct list_head                insecure_attachments;
>         /* Lock for the list */
>         spinlock_t                      attach_lock;
> --
> 2.49.0.805.g082f7c87e0-goog
>

Re: [RFC PATCH v2 08/10] iommu/arm-smmu-v3: Avoid suspend when user owns DMA

[Pranjal Shrivastava]

On Sun, May 04, 2025 at 01:28:26PM -0700, Daniel Mentz wrote:
> On Fri, Apr 18, 2025 at 4:35 PM Pranjal Shrivastava <praan@google.com> wrote:
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -796,7 +796,7 @@ struct arm_smmu_device {
> >         struct rb_root                  streams;
> >         struct mutex                    streams_mutex;
> >
> > -       /* Insecure Attach handles */
> > +       /* Insecure attach handles */
> 
> Move this to the original commit that introduced this comment.
> 

Ack.

> 
> 
> >         struct list_head                insecure_attachments;
> >         /* Lock for the list */
> >         spinlock_t                      attach_lock;
> > --
> > 2.49.0.805.g082f7c87e0-goog
> >

Thanks
Praan

[RFC PATCH v2 09/10] iommu/arm-smmu-v3: Enable pm_runtime and setup devlinks

[Pranjal Shrivastava]

Enable PM runtime for SMMUs having a power-domain during smmu probe.
Add a devlink between the clients and SMMU device. The absence of a
power domain effectively disables runtime power management.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
Reviewed-by: Mostafa Saleh <smostafa@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 7099c0cbf5fe..d78c6ab24eb2 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3640,6 +3640,9 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 		pci_prepare_ats(to_pci_dev(dev), stu);
 	}
 
+	device_link_add(dev, smmu->dev,
+			DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER);
+
 	return &smmu->iommu;
 
 err_free_master:
@@ -4999,11 +5002,18 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_disable;
 
+	if (dev->pm_domain) {
+		pm_runtime_set_active(dev);
+		pm_runtime_use_autosuspend(dev);
+		pm_runtime_set_autosuspend_delay(dev, RPM_AUTOSUSPEND_DELAY_MS);
+		pm_runtime_enable(dev);
+	}
+
 	/* And we're up. Go go go! */
 	ret = iommu_device_sysfs_add(&smmu->iommu, dev, NULL,
 				     "smmu3.%pa", &ioaddr);
 	if (ret)
-		goto err_disable;
+		goto err_rpm_disable;
 
 	ret = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
 	if (ret) {
@@ -5015,6 +5025,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 
 err_free_sysfs:
 	iommu_device_sysfs_remove(&smmu->iommu);
+err_rpm_disable:
+	pm_runtime_disable(dev);
 err_disable:
 	arm_smmu_device_disable(smmu);
 err_free_iopf:
-- 
2.49.0.805.g082f7c87e0-goog

[RFC PATCH v2 10/10] iommu/arm-smmu-v3: Invoke pm_runtime before hw access

[Pranjal Shrivastava]

Invoke the pm_runtime helpers at all places before accessing the hw.
The idea is to invoke runtime_pm helpers at common points which are
used by exposed ops or interrupt handlers. Elide all TLB and CFG
invalidations if the smmu is suspended but not  ATC invalidations.

Signed-off-by: Pranjal Shrivastava <praan@google.com>
---
 .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c     |  11 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 135 +++++++++++++++---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |   3 +
 3 files changed, 131 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index e4fd8d522af8..817d384ae8e4 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -13,10 +13,17 @@ void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type)
 	struct iommu_hw_info_arm_smmuv3 *info;
 	u32 __iomem *base_idr;
 	unsigned int i;
+	int ret;
+
+	ret = arm_smmu_rpm_get(master->smmu);
+	if (ret < 0)
+		return ERR_PTR(-EIO);
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (!info)
+	if (!info) {
+		arm_smmu_rpm_put(master->smmu);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	base_idr = master->smmu->base + ARM_SMMU_IDR0;
 	for (i = 0; i <= 5; i++)
@@ -27,6 +34,7 @@ void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type)
 	*length = sizeof(*info);
 	*type = IOMMU_HW_INFO_TYPE_ARM_SMMUV3;
 
+	arm_smmu_rpm_put(master->smmu);
 	return info;
 }
 
@@ -139,6 +147,7 @@ static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
 		.old_domain = iommu_get_domain_for_dev(dev),
 		.ssid = IOMMU_NO_PASID,
 	};
+	struct arm_smmu_device *smmu = master->smmu;
 	struct arm_smmu_ste ste;
 	int ret;
 
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index d78c6ab24eb2..9319df173cba 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -110,7 +110,7 @@ static const char * const event_class_str[] = {
 static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
 
 /* Runtime PM helpers */
-static int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
+int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
 {
 	int ret;
 
@@ -130,10 +130,10 @@ static int arm_smmu_rpm_get_if_active(struct arm_smmu_device *smmu)
 	if (pm_runtime_enabled(smmu->dev))
 		return pm_runtime_get_if_in_use(smmu->dev);
 
-	return 0;
+	return 1;
 }
 
-static void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
+void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
 {
 	int ret;
 
@@ -1072,7 +1072,9 @@ static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused
 {
 	struct arm_smmu_cmdq_ent cmd = {0};
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct arm_smmu_device *smmu = master->smmu;
 	int sid = master->streams[0].id;
+	int ret;
 
 	if (WARN_ON(!master->stall_enabled))
 		return;
@@ -1092,6 +1094,10 @@ static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused
 		break;
 	}
 
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret < 0)
+		return;
+
 	arm_smmu_cmdq_issue_cmd(master->smmu, &cmd);
 	/*
 	 * Don't send a SYNC, it doesn't do anything for RESUME or PRI_RESP.
@@ -1099,11 +1105,16 @@ static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused
 	 * terminated... at some point in the future. PRI_RESP is fire and
 	 * forget.
 	 */
+	arm_smmu_rpm_put(smmu);
 }
 
 /* Context descriptor manipulation functions */
 void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid)
 {
+	/* No need to invalidate TLBs if smmu is suspended */
+	if (!arm_smmu_rpm_get_if_active(smmu))
+		return;
+
 	struct arm_smmu_cmdq_ent cmd = {
 		.opcode	= smmu->features & ARM_SMMU_FEAT_E2H ?
 			CMDQ_OP_TLBI_EL2_ASID : CMDQ_OP_TLBI_NH_ASID,
@@ -1111,6 +1122,7 @@ void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid)
 	};
 
 	arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
+	arm_smmu_rpm_put(smmu);
 }
 
 /*
@@ -1317,6 +1329,10 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master,
 		},
 	};
 
+	/* No need to invalidate if smmu is suspended */
+	if (!arm_smmu_rpm_get_if_active(smmu))
+		return;
+
 	arm_smmu_cmdq_batch_init(smmu, &cmds, &cmd);
 	for (i = 0; i < master->num_streams; i++) {
 		cmd.cfgi.sid = master->streams[i].id;
@@ -1324,6 +1340,7 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master,
 	}
 
 	arm_smmu_cmdq_batch_submit(smmu, &cmds);
+	arm_smmu_rpm_put(smmu);
 }
 
 static void arm_smmu_write_cd_l1_desc(struct arm_smmu_cdtab_l1 *dst,
@@ -1613,6 +1630,10 @@ struct arm_smmu_ste_writer {
 
 static void arm_smmu_ste_writer_sync_entry(struct arm_smmu_entry_writer *writer)
 {
+	/* No need to invalidate if smmu is suspended */
+	if (!arm_smmu_rpm_get_if_active(writer->master->smmu))
+		return;
+
 	struct arm_smmu_ste_writer *ste_writer =
 		container_of(writer, struct arm_smmu_ste_writer, writer);
 	struct arm_smmu_cmdq_ent cmd = {
@@ -1624,6 +1645,7 @@ static void arm_smmu_ste_writer_sync_entry(struct arm_smmu_entry_writer *writer)
 	};
 
 	arm_smmu_cmdq_issue_cmd_with_sync(writer->master->smmu, &cmd);
+	arm_smmu_rpm_put(writer->master->smmu);
 }
 
 static const struct arm_smmu_entry_writer_ops arm_smmu_ste_writer_ops = {
@@ -1648,6 +1670,10 @@ static void arm_smmu_write_ste(struct arm_smmu_master *master, u32 sid,
 
 	/* It's likely that we'll want to use the new STE soon */
 	if (!(smmu->options & ARM_SMMU_OPT_SKIP_PREFETCH)) {
+		/* No need to prefech if smmu is suspended */
+		if (!arm_smmu_rpm_get_if_active(smmu))
+			return;
+
 		struct arm_smmu_cmdq_ent
 			prefetch_cmd = { .opcode = CMDQ_OP_PREFETCH_CFG,
 					 .prefetch = {
@@ -1655,6 +1681,7 @@ static void arm_smmu_write_ste(struct arm_smmu_master *master, u32 sid,
 					 } };
 
 		arm_smmu_cmdq_issue_cmd(smmu, &prefetch_cmd);
+		arm_smmu_rpm_put(smmu);
 	}
 }
 
@@ -2027,6 +2054,7 @@ static void arm_smmu_dump_event(struct arm_smmu_device *smmu, u64 *raw,
 
 static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 {
+	int ret;
 	u64 evt[EVTQ_ENT_DWORDS];
 	struct arm_smmu_event event = {0};
 	struct arm_smmu_device *smmu = dev;
@@ -2035,6 +2063,10 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
 				      DEFAULT_RATELIMIT_BURST);
 
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret < 0)
+		return IRQ_NONE;
+
 	do {
 		while (!queue_remove_raw(q, evt)) {
 			arm_smmu_decode_event(smmu, evt, &event);
@@ -2055,6 +2087,7 @@ static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev)
 
 	/* Sync our overflow flag, as we believe we're up to speed */
 	queue_sync_cons_ovf(q);
+	arm_smmu_rpm_put(smmu);
 	return IRQ_HANDLED;
 }
 
@@ -2102,6 +2135,11 @@ static irqreturn_t arm_smmu_priq_thread(int irq, void *dev)
 	struct arm_smmu_queue *q = &smmu->priq.q;
 	struct arm_smmu_ll_queue *llq = &q->llq;
 	u64 evt[PRIQ_ENT_DWORDS];
+	int ret;
+
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret < 0)
+		return IRQ_NONE;
 
 	do {
 		while (!queue_remove_raw(q, evt))
@@ -2113,6 +2151,7 @@ static irqreturn_t arm_smmu_priq_thread(int irq, void *dev)
 
 	/* Sync our overflow flag, as we believe we're up to speed */
 	queue_sync_cons_ovf(q);
+	arm_smmu_rpm_put(smmu);
 	return IRQ_HANDLED;
 }
 
@@ -2122,13 +2161,24 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev)
 {
 	u32 gerror, gerrorn, active;
 	struct arm_smmu_device *smmu = dev;
+	int ret;
+
+	if (pm_runtime_enabled(smmu->dev)) {
+		ret = pm_runtime_get_if_active(smmu->dev);
+		if (ret == 0) {
+			dev_err(smmu->dev, "Ignoring gerror interrupt because device isn't rpm active\n");
+			return IRQ_NONE;
+		}
+	}
 
 	gerror = readl_relaxed(smmu->base + ARM_SMMU_GERROR);
 	gerrorn = readl_relaxed(smmu->base + ARM_SMMU_GERRORN);
 
 	active = gerror ^ gerrorn;
-	if (!(active & GERROR_ERR_MASK))
+	if (!(active & GERROR_ERR_MASK)) {
+		arm_smmu_rpm_put(smmu);
 		return IRQ_NONE; /* No errors pending */
+	}
 
 	dev_warn(smmu->dev,
 		 "unexpected global error reported (0x%08x), this could be serious\n",
@@ -2161,6 +2211,7 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev)
 		arm_smmu_cmdq_skip_err(smmu);
 
 	writel(gerror, smmu->base + ARM_SMMU_GERRORN);
+	arm_smmu_rpm_put(smmu);
 	return IRQ_HANDLED;
 }
 
@@ -2251,26 +2302,33 @@ arm_smmu_atc_inv_to_cmd(int ssid, unsigned long iova, size_t size,
 static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,
 				   ioasid_t ssid)
 {
-	int i;
+	int i, ret;
 	struct arm_smmu_cmdq_ent cmd;
 	struct arm_smmu_cmdq_batch cmds;
 
 	arm_smmu_atc_inv_to_cmd(ssid, 0, 0, &cmd);
 
+	/* ATC invalidations shouldn't be elided */
+	ret = arm_smmu_rpm_get(master->smmu);
+	if (ret < 0)
+		return ret;
+
 	arm_smmu_cmdq_batch_init(master->smmu, &cmds, &cmd);
 	for (i = 0; i < master->num_streams; i++) {
 		cmd.atc.sid = master->streams[i].id;
 		arm_smmu_cmdq_batch_add(master->smmu, &cmds, &cmd);
 	}
 
-	return arm_smmu_cmdq_batch_submit(master->smmu, &cmds);
+	ret = arm_smmu_cmdq_batch_submit(master->smmu, &cmds);
+	arm_smmu_rpm_put(master->smmu);
+	return ret;
 }
 
 int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 			    unsigned long iova, size_t size)
 {
 	struct arm_smmu_master_domain *master_domain;
-	int i;
+	int i, ret;
 	unsigned long flags;
 	struct arm_smmu_cmdq_ent cmd = {
 		.opcode = CMDQ_OP_ATC_INV,
@@ -2297,6 +2355,11 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 	if (!atomic_read(&smmu_domain->nr_ats_masters))
 		return 0;
 
+	/* ATC invalidations shouldn't be elided */
+	ret = arm_smmu_rpm_get(smmu_domain->smmu);
+	if (ret < 0)
+		return ret;
+
 	arm_smmu_cmdq_batch_init(smmu_domain->smmu, &cmds, &cmd);
 
 	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
@@ -2325,7 +2388,9 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
 	}
 	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 
-	return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds);
+	ret = arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds);
+	arm_smmu_rpm_put(smmu_domain->smmu);
+	return ret;
 }
 
 /* IO_PGTABLE API */
@@ -2347,8 +2412,15 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 	} else {
 		cmd.opcode	= CMDQ_OP_TLBI_S12_VMALL;
 		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
+
+		/* No need to invalidate TLBs if smmu is suspended */
+		if (!arm_smmu_rpm_get_if_active(smmu))
+			goto atc_inv;
+
 		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
+		arm_smmu_rpm_put(smmu);
 	}
+atc_inv:
 	arm_smmu_atc_inv_domain(smmu_domain, 0, 0);
 }
 
@@ -2365,6 +2437,10 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
 	if (!size)
 		return;
 
+	/* No need to invalidate TLBs if smmu is suspended */
+	if (!arm_smmu_rpm_get_if_active(smmu))
+		return;
+
 	if (smmu->features & ARM_SMMU_FEAT_RANGE_INV) {
 		/* Get the leaf page size */
 		tg = __ffs(smmu_domain->domain.pgsize_bitmap);
@@ -2421,18 +2497,24 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
 		iova += inv_range;
 	}
 	arm_smmu_cmdq_batch_submit(smmu, &cmds);
+	arm_smmu_rpm_put(smmu);
 }
 
 static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
 					  size_t granule, bool leaf,
 					  struct arm_smmu_domain *smmu_domain)
 {
+	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_cmdq_ent cmd = {
 		.tlbi = {
 			.leaf	= leaf,
 		},
 	};
 
+	/* No need to invalidate TLBs if smmu is suspended */
+	if (!arm_smmu_rpm_get_if_active(smmu))
+		goto atc_inv;
+
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
 		cmd.opcode	= smmu_domain->smmu->features & ARM_SMMU_FEAT_E2H ?
 				  CMDQ_OP_TLBI_EL2_VA : CMDQ_OP_TLBI_NH_VA;
@@ -2441,6 +2523,7 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
 		cmd.opcode	= CMDQ_OP_TLBI_S2_IPA;
 		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
 	}
+
 	__arm_smmu_tlb_inv_range(&cmd, iova, size, granule, smmu_domain);
 
 	if (smmu_domain->nest_parent) {
@@ -2452,6 +2535,8 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
 		arm_smmu_cmdq_issue_cmd_with_sync(smmu_domain->smmu, &cmd);
 	}
 
+	arm_smmu_rpm_put(smmu);
+atc_inv:
 	/*
 	 * Unfortunately, this can't be leaf-only since we may have
 	 * zapped an entire table.
@@ -3278,13 +3363,14 @@ static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,
 		    sid_domain->type == IOMMU_DOMAIN_BLOCKED)
 			sid_domain->ops->attach_dev(sid_domain, dev);
 	}
+
 	return 0;
 }
 
-static void arm_smmu_attach_dev_ste(struct iommu_domain *domain,
-				    struct device *dev,
-				    struct arm_smmu_ste *ste,
-				    unsigned int s1dss)
+static int arm_smmu_attach_dev_ste(struct iommu_domain *domain,
+				   struct device *dev,
+				   struct arm_smmu_ste *ste,
+				   unsigned int s1dss)
 {
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 	struct arm_smmu_attach_state state = {
@@ -3327,6 +3413,7 @@ static void arm_smmu_attach_dev_ste(struct iommu_domain *domain,
 	 * descriptor from arm_smmu_share_asid().
 	 */
 	arm_smmu_clear_cd(master, IOMMU_NO_PASID);
+	return 0;
 }
 
 static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
@@ -3337,8 +3424,8 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
 
 	arm_smmu_master_clear_vmaster(master);
 	arm_smmu_make_bypass_ste(master->smmu, &ste);
-	arm_smmu_attach_dev_ste(domain, dev, &ste, STRTAB_STE_1_S1DSS_BYPASS);
-	return 0;
+	return arm_smmu_attach_dev_ste(domain, dev, &ste,
+				       STRTAB_STE_1_S1DSS_BYPASS);
 }
 
 static const struct iommu_domain_ops arm_smmu_identity_ops = {
@@ -3358,9 +3445,8 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
 
 	arm_smmu_master_clear_vmaster(master);
 	arm_smmu_make_abort_ste(&ste);
-	arm_smmu_attach_dev_ste(domain, dev, &ste,
-				STRTAB_STE_1_S1DSS_TERMINATE);
-	return 0;
+	return arm_smmu_attach_dev_ste(domain, dev, &ste,
+				       STRTAB_STE_1_S1DSS_TERMINATE);
 }
 
 static const struct iommu_domain_ops arm_smmu_blocked_ops = {
@@ -5037,10 +5123,19 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 static void arm_smmu_device_remove(struct platform_device *pdev)
 {
 	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
+	int ret;
 
 	iommu_device_unregister(&smmu->iommu);
 	iommu_device_sysfs_remove(&smmu->iommu);
+
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret <  0)
+		goto free_iopf;
+
 	arm_smmu_device_disable(smmu);
+	arm_smmu_rpm_put(smmu);
+
+free_iopf:
 	iopf_queue_free(smmu->evtq.iopf);
 	ida_destroy(&smmu->vmid_map);
 }
@@ -5048,8 +5143,14 @@ static void arm_smmu_device_remove(struct platform_device *pdev)
 static void arm_smmu_device_shutdown(struct platform_device *pdev)
 {
 	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret < 0)
+		return;
 
 	arm_smmu_device_disable(smmu);
+	arm_smmu_rpm_put(smmu);
 }
 
 static bool arm_smmu_suspend_is_safe(struct arm_smmu_device *smmu)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 28fddc07ab49..7be08347b012 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -1009,6 +1009,9 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
 				struct arm_smmu_cmdq *cmdq, u64 *cmds, int n,
 				bool sync);
 
+int arm_smmu_rpm_get(struct arm_smmu_device *smmu);
+void arm_smmu_rpm_put(struct arm_smmu_device *smmu);
+
 #ifdef CONFIG_ARM_SMMU_V3_SVA
 bool arm_smmu_sva_supported(struct arm_smmu_device *smmu);
 bool arm_smmu_master_sva_supported(struct arm_smmu_master *master);
-- 
2.49.0.805.g082f7c87e0-goog

Re: [RFC PATCH v2 10/10] iommu/arm-smmu-v3: Invoke pm_runtime before hw access

[kernel test robot]

Hi Pranjal,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on arm-perf/for-next/perf]
[also build test WARNING on linus/master v6.15-rc2 next-20250417]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Pranjal-Shrivastava/iommu-arm-smmu-v3-Refactor-arm_smmu_setup_irqs/20250419-073729
base:   https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git for-next/perf
patch link:    https://lore.kernel.org/r/20250418233409.3926715-11-praan%40google.com
patch subject: [RFC PATCH v2 10/10] iommu/arm-smmu-v3: Invoke pm_runtime before hw access
config: arm64-randconfig-001-20250419 (https://download.01.org/0day-ci/archive/20250419/202504192248.FUDmPe37-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250419/202504192248.FUDmPe37-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504192248.FUDmPe37-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c: In function 'arm_smmu_attach_dev_nested':
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c:150:26: warning: unused variable 'smmu' [-Wunused-variable]
     struct arm_smmu_device *smmu = master->smmu;
                             ^~~~


vim +/smmu +150 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c

   138	
   139	static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
   140					      struct device *dev)
   141	{
   142		struct arm_smmu_nested_domain *nested_domain =
   143			to_smmu_nested_domain(domain);
   144		struct arm_smmu_master *master = dev_iommu_priv_get(dev);
   145		struct arm_smmu_attach_state state = {
   146			.master = master,
   147			.old_domain = iommu_get_domain_for_dev(dev),
   148			.ssid = IOMMU_NO_PASID,
   149		};
 > 150		struct arm_smmu_device *smmu = master->smmu;
   151		struct arm_smmu_ste ste;
   152		int ret;
   153	
   154		if (nested_domain->vsmmu->smmu != master->smmu)
   155			return -EINVAL;
   156		if (arm_smmu_ssids_in_use(&master->cd_table))
   157			return -EBUSY;
   158	
   159		mutex_lock(&arm_smmu_asid_lock);
   160		/*
   161		 * The VM has to control the actual ATS state at the PCI device because
   162		 * we forward the invalidations directly from the VM. If the VM doesn't
   163		 * think ATS is on it will not generate ATC flushes and the ATC will
   164		 * become incoherent. Since we can't access the actual virtual PCI ATS
   165		 * config bit here base this off the EATS value in the STE. If the EATS
   166		 * is set then the VM must generate ATC flushes.
   167		 */
   168		state.disable_ats = !nested_domain->enable_ats;
   169		ret = arm_smmu_attach_prepare(&state, domain);
   170		if (ret) {
   171			mutex_unlock(&arm_smmu_asid_lock);
   172			return ret;
   173		}
   174	
   175		arm_smmu_make_nested_domain_ste(&ste, master, nested_domain,
   176						state.ats_enabled);
   177		arm_smmu_install_ste_for_dev(master, &ste);
   178		arm_smmu_attach_commit(&state);
   179		mutex_unlock(&arm_smmu_asid_lock);
   180		return 0;
   181	}
   182	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH v2 10/10] iommu/arm-smmu-v3: Invoke pm_runtime before hw access

[Daniel Mentz]

On Fri, Apr 18, 2025 at 4:35 PM Pranjal Shrivastava <praan@google.com> wrote:
> @@ -139,6 +147,7 @@ static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
>                 .old_domain = iommu_get_domain_for_dev(dev),
>                 .ssid = IOMMU_NO_PASID,
>         };
> +       struct arm_smmu_device *smmu = master->smmu;

Appears unused.

> @@ -130,10 +130,10 @@ static int arm_smmu_rpm_get_if_active(struct arm_smmu_device *smmu)
>         if (pm_runtime_enabled(smmu->dev))
>                 return pm_runtime_get_if_in_use(smmu->dev);
>
> -       return 0;
> +       return 1;

Can we fix this in the commit that added this function?

> @@ -2365,6 +2437,10 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>         if (!size)
>                 return;
>
> +       /* No need to invalidate TLBs if smmu is suspended */

The code doesn't check if the SMMU is suspended but rather whether the
usage count is zero which is different. I'm concerned about the
following sequence of events:

 * usage count drops to 0
 * TLB invalidations are elided, stale TLB entries remain
 * usage count becomes non-zero

This would result in stale TLB entries staying permanently. I propose
adding a new member like "is_suspended" to struct arm_smmu_device that
stores whether the SMMU device is suspended. If this value reads true
(i.e. SMMU is suspended) then you can rely on the driver issuing
TLBI_NSNH_ALL before re-enabling SMMU. Use proper acquire and release
semantics when accessing this variable.

Re: [RFC PATCH v2 10/10] iommu/arm-smmu-v3: Invoke pm_runtime before hw access

[Pranjal Shrivastava]

On Sun, May 04, 2025 at 01:29:04PM -0700, Daniel Mentz wrote:
> On Fri, Apr 18, 2025 at 4:35 PM Pranjal Shrivastava <praan@google.com> wrote:
> > @@ -139,6 +147,7 @@ static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
> >                 .old_domain = iommu_get_domain_for_dev(dev),
> >                 .ssid = IOMMU_NO_PASID,
> >         };
> > +       struct arm_smmu_device *smmu = master->smmu;
> 
> Appears unused.
> 

Yes, left it by mistake, the kernel test robot complains about it too,
will remove this.

> > @@ -130,10 +130,10 @@ static int arm_smmu_rpm_get_if_active(struct arm_smmu_device *smmu)
> >         if (pm_runtime_enabled(smmu->dev))
> >                 return pm_runtime_get_if_in_use(smmu->dev);
> >
> > -       return 0;
> > +       return 1;
> 
> Can we fix this in the commit that added this function?
> 

Ugh, yes, my bad. Will move it to the appropriate commit.

> > @@ -2365,6 +2437,10 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
> >         if (!size)
> >                 return;
> >
> > +       /* No need to invalidate TLBs if smmu is suspended */
> 
> The code doesn't check if the SMMU is suspended but rather whether the
> usage count is zero which is different. I'm concerned about the
> following sequence of events:
> 
>  * usage count drops to 0
>  * TLB invalidations are elided, stale TLB entries remain
>  * usage count becomes non-zero
> 
> This would result in stale TLB entries staying permanently. I propose
> adding a new member like "is_suspended" to struct arm_smmu_device that
> stores whether the SMMU device is suspended. If this value reads true
> (i.e. SMMU is suspended) then you can rely on the driver issuing
> TLBI_NSNH_ALL before re-enabling SMMU. Use proper acquire and release
> semantics when accessing this variable.

Hmm.. we aren't relying on the refcount alone but the rpm state as well.
Pasting the following lines from pm_runtime_get_if_in_use's doc strings:

 * Increment the runtime PM usage counter of @dev if its runtime PM
 * status is  %RPM_ACTIVE and its runtime PM usage counter is greater
 * than 0, in which case it returns 1.

Thus, we aren't only eliding the invalidations when the refcount == 0
but also when rpm state != RPM_ACTIVE which is checked first[1].

I believe you're suggesting a case where the rpm_state == RPM_ACTIVE
but the refcount bounces between 0 & 1 without changing the rpm state?

Skimming through the core runtime pm code[2], it seems it does nothing
to resume if current_state and/or last_state were RPM_ACTIVE. 

Let me dig deeper into the core runtime code to see if we're missing
something here.. otherwise, I agree with your suggestion to have a bool
to check if the smmu is really suspended.


Thanks,
Praan

[1]
https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/base/power/runtime.c#L1217

[2]
https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/base/power/runtime.c#L784