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

[Sairaj Kodilkar <sarunkod@amd.com>]

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;
	....
}

Thanks
Sairaj