Re: [PATCH v4 5/8] iommu/arm-smmu-v3: Add a usage counter for cmdq

[Pranjal Shrivastava <praan@google.com>]

On Tue, Nov 18, 2025 at 11:35:01AM +0530, Sairaj Kodilkar wrote:
> On 11/18/2025 12:44 AM, Pranjal Shrivastava wrote:
> > Introduce a biased counter to track the number of active cmdq owners as
> > a preparatory step for the runtime PM implementation.
> > 
> > The counter will be used to gate command submission, preventing the
> > submission of new commands while the device is suspended and deferring
> > suspend while the command submissions are in-flight.
> > 
> > The counter is biased to a value of 1 during device reset. A cmdq owner
> > or a thread issuing cmds with sync, increment it before accessing HW
> > registers and decrements it with release semantics afterwards.
> > 
> > A value of 1 represents an idle (but active) state. A suspend operation
> > will set it to from 1 -> 0 representing the suspended state.
> > 
> > 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 |  3 +
> >   2 files changed, 61 insertions(+), 11 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 1d6c60bee7dd..d6e75d1646d6 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -794,7 +794,7 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> >   	u64 cmd_sync[CMDQ_ENT_DWORDS];
> >   	u32 prod;
> >   	unsigned long flags;
> > -	bool owner;
> > +	bool owner, has_ref = false;
> >   	struct arm_smmu_ll_queue llq, head;
> >   	int ret = 0;
> > @@ -808,8 +808,15 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> >   		while (!queue_has_space(&llq, n + sync)) {
> >   			local_irq_restore(flags);
> > +
> > +			if (!atomic_inc_not_zero(&smmu->nr_cmdq_users))
> > +				/* Device is suspended, don't wait for space */
> > +				return 0;
> > +
> >   			if (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
> >   				dev_err_ratelimited(smmu->dev, "CMDQ timeout\n");
> > +
> > +			atomic_dec_return_release(&smmu->nr_cmdq_users);
> >   			local_irq_save(flags);
> >   		}
> > @@ -868,10 +875,35 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> >   		arm_smmu_cmdq_poll_valid_map(cmdq, llq.prod, prod);
> >   		/*
> > -		 * d. Advance the hardware prod pointer
> > +		 * d. Advance the hardware prod pointer (if smmu is still active)
> >   		 * Control dependency ordering from the entries becoming valid.
> >   		 */
> > -		writel_relaxed(prod, cmdq->q.prod_reg);
> > +		if (atomic_inc_not_zero(&smmu->nr_cmdq_users)) {
> > +			writel_relaxed(prod, cmdq->q.prod_reg);
> > +
> > +			if (sync) {
> > +				has_ref = true;
> > +			} else {
> > +				/*
> > +				 * Use release semantics to enforce ordering without a full barrier.
> > +				 * This ensures the prior writel_relaxed() is ordered/visible
> > +				 * before the refcount decrement, avoiding the heavy pipeline
> > +				 * stall of a full wmb().
> > +				 *
> > +				 * We need the atomic_dec_return_release() below and the
> > +				 * atomic_set_release() in step (e) below doesn't suffice.
> > +				 *
> > +				 * Specifically, without release semantics on the decrement,
> > +				 * the CPU is free to reorder the independent atomic_dec_relaxed()
> > +				 * before the writel_relaxed().
> > +				 *
> > +				 * If this happens, the refcount could drop to zero, allowing the PM
> > +				 * suspend path (running on another CPU) to disable the SMMU before
> > +				 * the register write completes, resulting in a bus fault.
> > +				 */
> > +				atomic_dec_return_release(&smmu->nr_cmdq_users);
> > +			}
> > +		}
> >   		/*
> >   		 * e. Tell the next owner we're done
> > @@ -883,14 +915,23 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> >   	/* 5. If we are inserting a CMD_SYNC, we must wait for it to complete */
> >   	if (sync) {
> > -		llq.prod = queue_inc_prod_n(&llq, n);
> > -		ret = arm_smmu_cmdq_poll_until_sync(smmu, cmdq, &llq);
> > -		if (ret) {
> > -			dev_err_ratelimited(smmu->dev,
> > -					    "CMD_SYNC timeout at 0x%08x [hwprod 0x%08x, hwcons 0x%08x]\n",
> > -					    llq.prod,
> > -					    readl_relaxed(cmdq->q.prod_reg),
> > -					    readl_relaxed(cmdq->q.cons_reg));
> > +
> > +		/* If we are not the owner, check if we're suspended */
> > +		if (!has_ref) {
> > +			if (atomic_inc_not_zero(&smmu->nr_cmdq_users))
> > +				has_ref = true;
> > +		}
> > +
> > +		if (has_ref) {
> 
> You can merge  above two if condition as follow
> 
> if (has_ref || atomic_inc_not_zero(&smmu->nr_cmdq_users)) {
> 	has_ref = true;
> 	....
> }
> 

Hmm.. yes, I guess redundantly setting has_ref = true again is fine..

> Thanks
> Sairaj
> 

Thanks,
Praan