Message-ID: <20251104102539.00001110@linux.microsoft.com>
Date: Tue, 4 Nov 2025 10:25:39 -0800
From: Jacob Pan <jacob.pan@...ux.microsoft.com>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: <linux-kernel@...r.kernel.org>, "iommu@...ts.linux.dev"
 <iommu@...ts.linux.dev>, Will Deacon <will@...nel.org>, Joerg Roedel
 <joro@...tes.org>, Mostafa Saleh <smostafa@...gle.com>, Jason Gunthorpe
 <jgg@...dia.com>, Robin Murphy <robin.murphy@....com>, Zhang Yu
 <zhangyu1@...ux.microsoft.com>, Jean Philippe-Brucker
 <jean-philippe@...aro.org>, Alexander Grest <Alexander.Grest@...rosoft.com>
Subject: Re: [PATCH v2 1/2] iommu/arm-smmu-v3: Fix CMDQ timeout warning

Hi Nicolin,

On Mon, 3 Nov 2025 17:23:26 -0800
Nicolin Chen <nicolinc@...dia.com> wrote:

> On Mon, Nov 03, 2025 at 03:16:31PM -0800, Jacob Pan wrote:
> > On Thu, 30 Oct 2025 15:41:57 -0700 Nicolin Chen
> > <nicolinc@...dia.com> wrote:  
> > > On Mon, Oct 20, 2025 at 03:43:52PM -0700, Jacob Pan wrote:
> > > I still feel that we could just replace the _until_not_full()
> > > with a _until_has_space()?  
> 
> > Since the current code uses three nested while loops, replacing the
> > inner _until_not_full() function means means retaining all three
> > nested while loops and calling queue_has_space in two places - once
> > in the middle while loop then again in this _until_has_space()
> > function.
> > 
> > I tried to extract the inner loop into a function but it requires
> > passing in irqflags to restore. Not pretty.  
> 
> I think we could do:
> 
> -----------------------------------------------------------------
> 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
> 2a8b46b948f05..1211e087dedca 100644 ---
> a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -138,12 +138,6 @@
> static bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
> return space >= n; }
>  
> -static bool queue_full(struct arm_smmu_ll_queue *q)
> -{
> -	return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
> -	       Q_WRP(q, q->prod) != Q_WRP(q, q->cons);
> -}
> -
>  static bool queue_empty(struct arm_smmu_ll_queue *q)
>  {
>  	return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
> @@ -633,14 +627,13 @@ static void arm_smmu_cmdq_poll_valid_map(struct
> arm_smmu_cmdq *cmdq, __arm_smmu_cmdq_poll_set_valid_map(cmdq, sprod,
> eprod, false); }
>  
> -/* Wait for the command queue to become non-full */
> -static int arm_smmu_cmdq_poll_until_not_full(struct arm_smmu_device
> *smmu,
> -					     struct arm_smmu_cmdq
> *cmdq,
> -					     struct
> arm_smmu_ll_queue *llq) +/* Poll command queue PROD and CONS, using a
> continued timer */ +static inline void arm_smmu_cmdq_poll(struct
> arm_smmu_device *smmu,
> +				      struct arm_smmu_cmdq *cmdq,
> +				      struct arm_smmu_ll_queue *llq,
> +				      struct arm_smmu_queue_poll *qp)
>  {
>  	unsigned long flags;
> -	struct arm_smmu_queue_poll qp;
> -	int ret = 0;
>  
>  	/*
>  	 * Try to update our copy of cons by grabbing exclusive cmdq
> access. If @@ -650,19 +643,18 @@ static int
> arm_smmu_cmdq_poll_until_not_full(struct arm_smmu_device *smmu,
> WRITE_ONCE(cmdq->q.llq.cons, readl_relaxed(cmdq->q.cons_reg));
> arm_smmu_cmdq_exclusive_unlock_irqrestore(cmdq, flags); llq->val =
> READ_ONCE(cmdq->q.llq.val);
> -		return 0;
> +		return;
>  	}
>  
> -	queue_poll_init(smmu, &qp);
> -	do {
> -		llq->val = READ_ONCE(cmdq->q.llq.val);
> -		if (!queue_full(llq))
> -			break;
> -
> -		ret = queue_poll(&qp);
> -	} while (!ret);
> -
> -	return ret;
> +	/* queue_poll() returns 0 or -ETIMEDOUT only */
> +	if (queue_poll(qp)) {
I would still prefer more a defensive approach to prevent future change
of queue_poll returning other error being treated as ETIMEOUT.

> +		dev_err_ratelimited(smmu->dev,
> +				    "CMDQ timeout at prod 0x%08x
> cons 0x%08x\n",
> +				    llq->prod, llq->cons);
> +		/* Restart the timer */
> +		queue_poll_init(smmu, qp);
> +	}
> +	llq->val = READ_ONCE(cmdq->q.llq.val);
>  }
>  
>  /*
> @@ -804,12 +796,13 @@ int arm_smmu_cmdq_issue_cmdlist(struct
> arm_smmu_device *smmu, local_irq_save(flags);
>  	llq.val = READ_ONCE(cmdq->q.llq.val);
>  	do {
> +		struct arm_smmu_queue_poll qp;
>  		u64 old;
>  
> +		queue_poll_init(smmu, &qp);
>  		while (!queue_has_space(&llq, n + sync)) {
>  			local_irq_restore(flags);
> -			if (arm_smmu_cmdq_poll_until_not_full(smmu,
> cmdq, &llq))
> -				dev_err_ratelimited(smmu->dev, "CMDQ
> timeout\n");
> +			arm_smmu_cmdq_poll(smmu, cmdq, &llq, &qp);
>  			local_irq_save(flags);
>  		}
>  
yeah, that should work. it is more readable than open coding.

> -----------------------------------------------------------------
> 
> And the commit message should point out:
> 
> The existing arm_smmu_cmdq_poll_until_not_full() doesn't fit
> efficiently nor ideally to the only caller
> arm_smmu_cmdq_issue_cmdlist():
>  - It uses a new timer at every single call, which fails to limit to
> the preset ARM_SMMU_POLL_TIMEOUT_US per issue.
Not following what you mean.
The original code below does honor the timeout of
ARM_SMMU_POLL_TIMEOUT_US

-       queue_poll_init(smmu, &qp);
-       do {
-               llq->val = READ_ONCE(cmdq->q.llq.val);
-               if (!queue_full(llq))
-                       break;
-
-               ret = queue_poll(&qp);
-       } while (!ret);

>  - It has a redundant internal queue_full(), which doesn't detect
> whether there is a enough space for number of n commands.
will incorporate, though the same points already mentioned in the
current commit message.
 
> So, rework it to be an inline helper to work with the
> queue_has_space().
> 

> Nicolin

Powered by blists - more mailing lists