Re: [PATCH v11 11/16] iommu/arm-smmu-v3: Add CMDQ_PROD_STOP_FLAG to gate CMDQ submissions

[Will Deacon]

On Tue, Sep 29, 2026 at 03:45:05AM +0000, Pranjal Shrivastava wrote:
> Introduce a new bit flag, CMDQ_PROD_STOP_FLAG (bit 30), in the command
> queue's producer index to safely gate command submissions during device
> suspension.
> 
> The flag embeds the suspend state directly into the existing global state
> The flag is checked in the cmpxchg loop in arm_smmu_cmdq_issue_cmdlist(),
> which acts as a Point of Commitment, ensuring that no indices are
> reserved or committed once the SMMU begins suspending.

I'm not familiar with the term "Point of Commitment". Is that something
you came up with? Without a general definition, I think I'd prefer to
avoid using it, especially as it sounds like something to do with the
Arm memory system (where we already have architectural terms such as
"Point of Coherency".
 
> This prevents a situation of "abandoned batches" where indices are
> incremented but commands are never written, which would otherwise
> lead to timeout during the drain poll.
> 
> Update queue_inc_prod_n() to preserve this flag during index
> calculations, ensuring that any in-flight commands that successfully
> passed the point of commitment can proceed to completion while the
> flag remains set.
> 
> Suggested-by: Daniel Mentz <danielmentz@xxxxxxxxxx>
> Reviewed-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> Signed-off-by: Pranjal Shrivastava <praan@xxxxxxxxxx>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 35 +++++++++++++++++++--
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  5 +++
>  2 files changed, 38 insertions(+), 2 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 a123810fac57..7f89e2306942 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -218,7 +218,8 @@ static int queue_sync_prod_in(struct arm_smmu_queue *q)
>  static u32 queue_inc_prod_n(struct arm_smmu_ll_queue *q, int n)
>  {
>  	u32 prod = Q_POS(q, q->prod) + n;
> -	return Q_OVF(q->prod) | Q_POS(q, prod);
> +
> +	return Q_OVF(q->prod) | Q_STOP(q->prod) | Q_POS(q, prod);
>  }
>  
>  static void queue_poll_init(struct arm_smmu_device *smmu,
> @@ -726,13 +727,42 @@ int __arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
>  	do {
>  		u64 old;
>  
> +		/*
> +		 * If the SMMU is suspended/suspending, any new CMDs are elided.
> +		 * This loop is the Point of Commitment. If we haven't cmpxchg'd
> +		 * our new indices yet, we can safely bail. Once the indices are
> +		 * committed, we MUST write valid commands to those slots to
> +		 * avoid indefinite polling in the drain function.
> +		 */
> +		if (Q_STOP(llq.prod)) {
> +			local_irq_restore(flags);
> +			return 0;
> +		}
> +
>  		while (!queue_has_space(&llq, n + sync)) {
>  			local_irq_restore(flags);
> +
> +			/* Avoid waiting for space if the SMMU is suspending */
> +			if (Q_STOP(READ_ONCE(cmdq->q.llq.prod)))
> +				return 0;
> +
>  			if (arm_smmu_cmdq_poll_until_not_full(smmu, cmdq, &llq))
>  				dev_err_ratelimited(smmu->dev, "CMDQ timeout\n");

It's a bit grotty to have the extra READ_ONCE() here. Why not make
arm_smmu_cmdq_poll_until_not_full() return if the stop bit is set?

> 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 deefb17e31eb..794b258550dd 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -394,6 +394,11 @@ static inline unsigned int arm_smmu_cdtab_l2_idx(unsigned int ssid)
>  #define CMDQ_ERR_CERROR_ATC_INV_IDX	3
>  
>  #define CMDQ_PROD_OWNED_FLAG		Q_OVERFLOW_FLAG
> +#define CMDQ_PROD_STOP_FLAG		(1U << 30)

I'm unsure about this. Bit 30 of the cmdq prod register is RES0, so it's
plausible that the architecture allocates that bit for something in future.
For example, if the index field was extended, then I think that would
cause problems for the driver.

Do we have to put the stop flag in the producer value, or can it just be
a flag elsewhere in memory?

Will