Re: [PATCH v11 03/16] iommu/arm-smmu-v3: Add arm_smmu_drain_queue() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] iommu: missing final condition check in arm_smmu_drain_queue() polling loop
--

commit d3de0e0ff6c17b4b4b52184a4e8af1a09c29b369
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Add arm_smmu_drain_queue() helper

This commit adds a sleepable, counting-based arm_smmu_drain_queue() helper
to replace queue specific polling loops. Its until_empty mode serves suspend
and runtime PM routines that drain the CMDQ.

> 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 06b7de2e6e4b6..84b56849f6dc4 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c

[ ... ]

> @@ -948,6 +948,87 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,

[ ... ]

> +	while (true) {
> +		/* Accumulate the entries consumed since the last poll */
> +		prev = cons;
> +		cons = readl_relaxed(q->cons_reg);
> +		drained += Q_POS(&q->llq, cons - prev);
> +
> +		prod = readl_relaxed(q->prod_reg);
> +		undrained = Q_POS(&q->llq, prod - cons);
> +
> +		/* Exit on an empty queue, regardless of until_empty */
> +		if (!undrained)
> +			return 0;
> +
> +		/* Snapshot mode: exit once the pending entries are drained */
> +		if (!until_empty && drained >= pending)
> +			return 0;
> +
> +		/*
> +		 * A timeout means the consumer might be stuck. In theory, if it
> +		 * moves 2 * qsize entries or more within a single poll interval
> +		 * Q_POS() would wrap and undercount drained: that could trigger
> +		 * a spurious warning too, if the queue was never once observed
> +		 * empty. Yet, that much consumption in such a short interval is
> +		 * unrealistic. WARN it only, as a stuck consumer is a real bug.
> +		 */
> +		if (WARN_ON(ktime_compare(ktime_get(), timeout) > 0))
> +			break;

[Severity: Medium]
Is there a missing final condition re-evaluation after the timeout check in
arm_smmu_drain_queue()?

If the thread executing this loop is preempted for more than the timeout
duration immediately before the ktime_get() check:

arm_smmu_drain_queue()
    if (!undrained)
        return 0;
    
    [ ... thread preempted by hypervisor for >1 second ... ]
    
    if (WARN_ON(ktime_compare(ktime_get(), timeout) > 0))
        break;
        
Could this sequence result in a spurious timeout?

Even if the queue was fully drained while the thread was scheduled out,
when the thread resumes it will immediately evaluate the timeout as
exceeded without checking the queue state one last time.

This would trigger a WARN_ON stack trace and return -ETIMEDOUT, which could
then cause caller operations like PM suspend to fail.

> +
> +		/* The consumer might be a threaded IRQ handler. Yield to it */
> +		usleep_range(100, 200);
> +	}
> +
> +	dev_warn_ratelimited(smmu->dev,
> +			     "queue drain timed out at prod=0x%x cons=0x%x\n",
> +			     prod, cons);
> +	return -ETIMEDOUT;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929034510.2023173-1-praan@google.com?part=3