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

[Jason Gunthorpe]

> [ ... 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..

> [ ... 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.

Otherwise the approach looks OK to me

Reviewed-by: Jason Gunthorpe <jgg at nvidia.com>

-- 
Jason