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

[Robin Murphy <robin.murphy@arm.com>]

On 07/10/2026 3:08 pm, Will Deacon wrote:
> 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@google.com>
>> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>> Signed-off-by: Pranjal Shrivastava <praan@google.com>
>> ---
>>   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?

Logically, it feels like this should happen earlier in the process 
anyway - if we're entering a state where we don't want to accept _any_ 
new command submissions, then surely arm_smmu_get_cmdq() should just 
start saying "No, there is no valid queue to submit this command." to 
everything?

Thanks,
Robin.