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

[Pranjal Shrivastava <praan@google.com>]

On Tue, Oct 13, 2026 at 12:35:40PM +0100, Robin Murphy wrote:
> 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?
> 

We can do that as well, but it can only be an early-out.. the STOP_FLAG
may not be set when arm_smmu_get_cmdq() looks at it, but get set right
after, before we reserve our slots in arm_smmu_cmdq_issue_cmdlist(). So
we'd still need the check at the point where we actually commit, i.e.
the cmpxchg on llq.prod.

Thanks,
Praan