Subject: Re: [PATCH v7 08/11] iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops

From: Pranjal Shrivastava <[email-masked]>
Date: Thu, 28 May 2026 21:21:22 +0000

On Thu, May 28, 2026 at 12:39:46PM -0700, Nicolin Chen wrote:
> On Wed, May 27, 2026 at 10:14:04PM +0000, Pranjal Shrivastava wrote:
> > +/* Runtime PM helpers */
> > +__maybe_unused static int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
> > +{
> > +	int ret;
> > +
> > +	if (pm_runtime_enabled(smmu->dev)) {
> > +		ret = pm_runtime_resume_and_get(smmu->dev);
> > +		if (ret < 0) {
> > +			dev_err(smmu->dev, "failed to resume device: %d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> 
> Nit: ret is used within the first if.
> 
> Yet, I wouldn't like it to move. So, maybe do an early return:
> 
> 	if (!pm_runtime_enabled(smmu->dev))
> 		return 0;
> 

Ack. I'll add an early return.

> > +__maybe_unused static void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
> > +{
> > +	int ret;
> > +
> > +	if (pm_runtime_enabled(smmu->dev)) {
> > +		ret = pm_runtime_put_autosuspend(smmu->dev);
> > +		if (ret < 0)
> > +			dev_err(smmu->dev, "failed to suspend device: %d\n", ret);
> > +	}
> > +}
> 
> Ditto
> 

Same here.

> > +
> > +static inline u32 arm_smmu_cmdq_owner_prod_idx(struct arm_smmu_cmdq *cmdq)
> > +{
> > +	return atomic_read(&cmdq->owner_prod) & CMDQ_PROD_IDX_MASK;
> > +}
> > +
> >  static void parse_driver_options(struct arm_smmu_device *smmu)
> >  {
> >  	int i = 0;
> > @@ -789,7 +822,8 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> >  		/* b. Stop gathering work by clearing the owned flag */
> >  		prod = atomic_fetch_andnot_relaxed(CMDQ_PROD_OWNED_FLAG,
> >  						   &cmdq->q.llq.atomic.prod);
> > -		prod &= ~CMDQ_PROD_OWNED_FLAG;
> > +		/* Strip all metadata flags */
> > +		prod &= CMDQ_PROD_IDX_MASK;
> 
> Should its prior atomic_fetch_andnot_relaxed() call do something
> about the CMDQ_PROD_STOP_FLAG as well?

Umm.. No, the atomic_fetch_andnot_relaxed() call must leave the STOP_FLAG.
This block is the owner-publish phase, which occurs *after* the Point of
Commitment. If a submission successfully reserved its indices before the
gate closed, it shall be allowed to finish.

If the owner thread cleared the STOP_FLAG here in the global memory, it
would prematurely re-open the gate, allowing new racing submissions to 
leak in during the suspend sequence. 

The algorithm works mainly because the RPM callbacks are the only ones
that are allowed to manipulate this flag.

> 
> > +/*
> > + * Lockless pre-check to elide invalidations if SMMU is suspended.
> > + * Races with concurrent suspend are benign: the cmpxchg loop in
> > + * arm_smmu_cmdq_issue_cmdlist() acts as the true commit point.
> > + * If we lose the race, that loop observes Q_STOP == 1 and safely
> > + * drops the command. If we win, the suspend thread waits for us.
> > + */
> > +static inline bool arm_smmu_can_elide(struct arm_smmu_device *smmu)
> > +{
> > +	return !!Q_STOP(READ_ONCE(smmu->cmdq.q.llq.prod));
> > +}
> 
> arm_smmu_cmdq_can_elide()

Nice name! I'll update it.

> 
> Should it handle a secondary_cmdq?

No, I don't think we need to check secondary queues here. The STOP_FLAG
being set on the primary CMDQ during the suspend should suffice to 
indicate the entire SMMU's power state. That's why the function takes in
the smmu ptr and not the cmdq.

Altough, thanks for discussing this, I noticed that while rebasing I
missed the elision check at one place: arm_smmu_write_ste()
I'll take add a check in that too in the next version.

Did you have another scenario / use-case in mind where this might not
work?

Thanks,
Praan