Re: [PATCH v9 02/12] iommu/arm-smmu-v3: Add a helper to drain cmd queues

[Pranjal Shrivastava <praan@google.com>]

On Tue, Aug 25, 2026 at 01:36:11PM -0300, Jason Gunthorpe wrote:
> > [ ... 36 lines skipped ... ]
> > +		if (queue_empty(llq))
> 
>  [Severity: High]
>  Does polling for an empty CMDQ actually guarantee that commands have completed?
> 
>  queue_empty() relies on the SMMU consumer register equaling the producer
>  pointer. However, for CMD_SYNC, the SMMU advances the consumer register
>  immediately upon consumption, not upon completion. Suspending the SMMU while
>  commands like ATC_INV are still in-flight could abort them, leaving stale
>  translations in the device ATC and causing submitters to time out.
> 
> This seems broadly wrong, but it doesn't seem explained very well,
> maybe put a comment above the function something like
> 
>  The caller must have ensured that no concurrent command submission
>  can happen. Thus the entry at the PROD must be a CMD_SYNC due to how
>  all submitters generate commands. Waiting for PROD==CONS guarentees
>  completion of the sync and all prior work and thus idle HW.
> 
> I don't think the other substantive remarks are valid.
> 
> The Werror thing is right, every patch should compile alone without
> warnings. Sometimes people add __maybe_unused to accomplish this..
>

Ack. I plan to add __maybe_unused here.

> > [ ... 19 lines skipped ... ]
>  [Severity: High]
>  Does this logic miss commands routed to secondary queues?
> 
>  The driver supports routing commands, specifically ATC_INV, to secondary
>  queues (like the Tegra241 CMDQV virtual queues) via get_secondary_cmdq().
>  By strictly hardcoding the poll to smmu->cmdq.q, we ignore secondary queues.
>  If the SMMU suspends while ATC_INV commands are processing there, they could
>  be dropped.
> 
> Yes, I think the ordering is wrong, to keep them as different patches
> 
> iommu/tegra241-cmdqv: Add a helper to drain VCMDQs
> 
> Should come first, adding the op callback, then this patch would have
> the hunk completing the function so the newly introduced function
> works completely.
> 

Ack, I'll re-order the patches.

> Otherwise the approach looks OK to me
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks,
Praan