Re: [RFC PATCH v3 2/8] iommu/arm-smmu-v3: Add a helper to drain cmd queues

[Pranjal Shrivastava <praan@google.com>]

On Tue, Jul 08, 2025 at 03:32:08PM +0000, Mostafa Saleh wrote:
> On Mon, Jun 16, 2025 at 08:31:43PM +0000, Pranjal Shrivastava wrote:
> > Before we suspend SMMU, we want to ensure that all commands have been
> > flushed by all the queues i.e. the queues are empty. Add a helper
> > function that polls till the queues are dained.
> > 
> > Signed-off-by: Pranjal Shrivastava <praan@google.com>
> > ---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 40 +++++++++++++++++++++
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 ++
> >  2 files changed, 43 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 f19a77708d2c..0df1b17a09cc 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,46 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,
> >  					   cmds->num, true);
> >  }
> >  
> > +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);
> > +	do {
> > +		if (queue_empty(llq))
> > +			break;
> > +
> > +		ret = queue_poll(&qp);
> > +		WRITE_ONCE(llq->cons, readl_relaxed(q->cons_reg));
> > +
> > +	} while (!ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int arm_smmu_drain_queues(struct arm_smmu_device *smmu)
> > +{
> > +	struct arm_smmu_cmdq *cmdq = &smmu->cmdq;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	/*
> > +	 * Since this is only called from the suspend callback, we
> > +	 * should be able to acquire the exclusive lock without failing.
> > +	 */
> 
> I don’t understand the comment, can’t the command queue be busy at this
> point form another unmap context?
> 

The expectation would be that if we entered the suspend callback, all
clients are down.. if there's an interrupt I think we're holding the
power lock during the entirety of this callback, thus the client should
sleep or spin at rpm_get() for the dev?

> > +	arm_smmu_cmdq_exclusive_trylock_irqsave(cmdq, flags);
> > +	ret = arm_smmu_queue_poll_until_empty(smmu, &smmu->cmdq.q);
> > +	arm_smmu_cmdq_exclusive_unlock_irqrestore(cmdq, flags);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> 
> I wonder if that’s enough, I see that if arm_smmu_cmdq_issue_cmdlist()
> is called with sync=false, it won't touch the exclusive lock, so what
> happens if another driver keeps unmapping, that means that this thread
> can loop forever as prod keeps growing?
> 

I'm not sure how can we enter suspend while a client is awake? I'd
expect the devlinks to ensure that suspend isn't called if a client is
awake. If it wakes up while we are *in* the suspend callback, it will
have to wait for the entire callback to execute, i.e. it will spin/sleep
in the rpm_get() call for the client device.

> Thinking out loud about this, if the aim of draining the command queue
> is not TLB invalidation but ATC, maybe we need higher-level
> synchronization between those 2 and keep the command queue out of this.
> (this also applies for the next patch for tegra)
> 

Hmm.. I guess that would mean issuing all ATC invalidations synchronously
We already hold a PM ref while issuing the ATC inv commands, but I think
we'll have to issue all of those with sync (wait till CMD_SYNC is done)

> Thanks,
> Mostafa
> 
> >  static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
> >  				   struct iommu_page_response *resp)
> >  {
> > 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..461f1958e574 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -965,6 +965,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 &
> > -- 
> > 2.50.0.rc2.692.g299adb8693-goog
> >