[PATCH net] net: airoha: Take into account out-of-order tx completions in airoha_dev_xmit()

[PATCH net] net: airoha: Take into account out-of-order tx completions in airoha_dev_xmit()

[Lorenzo Bianconi]

Completion napi can free out-of-order tx descriptors if hw QoS is
enabled and packets with different priority are queued to same DMA ring.
Take into account possible out-of-order reports checking if the tx queue
is full using circular buffer head/tail pointer instead of the number of
queued packets.

Fixes: 23020f0493270 ("net: airoha: Introduce ethernet support for EN7581 SoC")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/airoha/airoha_eth.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index 833dd911980b3f698bd7e5f9fd9e2ce131dd5222..5e2ff52dba03a7323141fe9860fba52806279bd0 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -1873,6 +1873,19 @@ static u32 airoha_get_dsa_tag(struct sk_buff *skb, struct net_device *dev)
 #endif
 }
 
+static bool airoha_dev_is_tx_busy(struct airoha_queue *q, u32 nr_frags)
+{
+	u16 index = (q->head + nr_frags) % q->ndesc;
+
+	/* completion napi can free out-of-order tx descriptors if hw QoS is
+	 * enabled and packets with different priorities are queued to the same
+	 * DMA ring. Take into account possible out-of-order reports checking
+	 * if the tx queue is full using circular buffer head/tail pointers
+	 * instead of the number of queued packets.
+	 */
+	return index >= q->tail && (q->head < q->tail || q->head > index);
+}
+
 static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
 				   struct net_device *dev)
 {
@@ -1926,7 +1939,7 @@ static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
 	txq = netdev_get_tx_queue(dev, qid);
 	nr_frags = 1 + skb_shinfo(skb)->nr_frags;
 
-	if (q->queued + nr_frags > q->ndesc) {
+	if (airoha_dev_is_tx_busy(q, nr_frags)) {
 		/* not enough space in the queue */
 		netif_tx_stop_queue(txq);
 		spin_unlock_bh(&q->lock);

---
base-commit: 18a7e218cfcdca6666e1f7356533e4c988780b57
change-id: 20251010-airoha-tx-busy-queue-bdf11cec83ed

Best regards,
-- 
Lorenzo Bianconi <lorenzo@kernel.org>

Re: [PATCH net] net: airoha: Take into account out-of-order tx completions in airoha_dev_xmit()

[Simon Horman]

On Fri, Oct 10, 2025 at 07:21:43PM +0200, Lorenzo Bianconi wrote:
> Completion napi can free out-of-order tx descriptors if hw QoS is
> enabled and packets with different priority are queued to same DMA ring.
> Take into account possible out-of-order reports checking if the tx queue
> is full using circular buffer head/tail pointer instead of the number of
> queued packets.
> 
> Fixes: 23020f0493270 ("net: airoha: Introduce ethernet support for EN7581 SoC")
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 833dd911980b3f698bd7e5f9fd9e2ce131dd5222..5e2ff52dba03a7323141fe9860fba52806279bd0 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1873,6 +1873,19 @@ static u32 airoha_get_dsa_tag(struct sk_buff *skb, struct net_device *dev)
>  #endif
>  }
>  
> +static bool airoha_dev_is_tx_busy(struct airoha_queue *q, u32 nr_frags)
> +{
> +	u16 index = (q->head + nr_frags) % q->ndesc;
> +
> +	/* completion napi can free out-of-order tx descriptors if hw QoS is
> +	 * enabled and packets with different priorities are queued to the same
> +	 * DMA ring. Take into account possible out-of-order reports checking
> +	 * if the tx queue is full using circular buffer head/tail pointers
> +	 * instead of the number of queued packets.
> +	 */
> +	return index >= q->tail && (q->head < q->tail || q->head > index);

Hi Lorenzo,

I think there is a corner case here.
Perhaps they can't occur, but here goes.

Let us suppose that head is 1.
And the ring is completely full, so tail is 2.

Now, suppose nr_frags is ndesc - 1.
In this case the function above will return false. But the ring is full.

Ok, ndesc is actually 1024 and nfrags should never be close to that.
But the problem is general. And a perhaps more realistic example is:

  ndesc is 1024
  head is 1008
  The ring is full so tail is 1009
  (Or head is any other value that leaves less than 16 slots free)
  nr_frags is 16

airoha_dev_is_tx_busy() returns false, even though the ring is full.

Probably this has it's own problems. But if my reasoning above is correct
(is it?) then the following seems to address it by flattening and extending
the ring. Because what we are about is the relative value of head, index
and tail. Not the slots they occupy in the ring.

N.B: I tetsed the algorirthm with a quick implementation in user-space.
The following is, however, completely untested.

static bool airoha_dev_is_tx_busy(struct airoha_queue *q, u32 nr_frags)
{
	unsigned int tail = q->tail < q->head ? q->tail + q->ndesc : q->tail;
	unsigned int index = q->head + nr_frags;

	return index >= tail;
}

...

Re: [PATCH net] net: airoha: Take into account out-of-order tx completions in airoha_dev_xmit()

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3998 bytes --]

> On Fri, Oct 10, 2025 at 07:21:43PM +0200, Lorenzo Bianconi wrote:
> > Completion napi can free out-of-order tx descriptors if hw QoS is
> > enabled and packets with different priority are queued to same DMA ring.
> > Take into account possible out-of-order reports checking if the tx queue
> > is full using circular buffer head/tail pointer instead of the number of
> > queued packets.
> > 
> > Fixes: 23020f0493270 ("net: airoha: Introduce ethernet support for EN7581 SoC")
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  drivers/net/ethernet/airoha/airoha_eth.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index 833dd911980b3f698bd7e5f9fd9e2ce131dd5222..5e2ff52dba03a7323141fe9860fba52806279bd0 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -1873,6 +1873,19 @@ static u32 airoha_get_dsa_tag(struct sk_buff *skb, struct net_device *dev)
> >  #endif
> >  }
> >  
> > +static bool airoha_dev_is_tx_busy(struct airoha_queue *q, u32 nr_frags)
> > +{
> > +	u16 index = (q->head + nr_frags) % q->ndesc;
> > +
> > +	/* completion napi can free out-of-order tx descriptors if hw QoS is
> > +	 * enabled and packets with different priorities are queued to the same
> > +	 * DMA ring. Take into account possible out-of-order reports checking
> > +	 * if the tx queue is full using circular buffer head/tail pointers
> > +	 * instead of the number of queued packets.
> > +	 */
> > +	return index >= q->tail && (q->head < q->tail || q->head > index);
> 
> Hi Lorenzo,

Hi Simon,

thx for the review.

> 
> I think there is a corner case here.
> Perhaps they can't occur, but here goes.
> 
> Let us suppose that head is 1.
> And the ring is completely full, so tail is 2.
> 
> Now, suppose nr_frags is ndesc - 1.
> In this case the function above will return false. But the ring is full.
> 
> Ok, ndesc is actually 1024 and nfrags should never be close to that.
> But the problem is general. And a perhaps more realistic example is:
> 
>   ndesc is 1024
>   head is 1008
>   The ring is full so tail is 1009
>   (Or head is any other value that leaves less than 16 slots free)
>   nr_frags is 16
> 
> airoha_dev_is_tx_busy() returns false, even though the ring is full.

yes, you are right, this corner case is not properly managed by the proposed
algorithm, thx for pointing this out.

> 
> Probably this has it's own problems. But if my reasoning above is correct
> (is it?) then the following seems to address it by flattening and extending
> the ring. Because what we are about is the relative value of head, index
> and tail. Not the slots they occupy in the ring.
> 
> N.B: I tetsed the algorirthm with a quick implementation in user-space.
> The following is, however, completely untested.
> 
> static bool airoha_dev_is_tx_busy(struct airoha_queue *q, u32 nr_frags)
> {
> 	unsigned int tail = q->tail < q->head ? q->tail + q->ndesc : q->tail;
> 	unsigned int index = q->head + nr_frags;
> 
> 	return index >= tail;
> }

I agree, the algorithm you proposed properly manages the 99% of the cases. The
only case where it fails is when the queue is empty (so tail = head = x,
e.g. x = 0). In this case we would have:

	- q->ndesc = 1024
	- q->tail = q->head = 0
	- tail = 0
	- index = n (e.g. n = 1)
	- index >= tail ==> 1 >= 0 ==> busy (but the queue is actually empty).

I guess we should add a minor change in the tail definition:

	u32 tail = q->tail <= q->head ? q->tail + q->ndesc : q->tail;

so:
	- q->ndesc = 1024
	- q->tail = q->head = 0
	- tail = 1024
	- index = n (e.g. n = 1)
	- index >= tail => 1 < 1024 => OK

Can you spot any downside with this approach?
I tested the proposed approach and it seems to be working fine.

Regards,
Lorenzo

> 
> ...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net] net: airoha: Take into account out-of-order tx completions in airoha_dev_xmit()

[Simon Horman]

On Sat, Oct 11, 2025 at 03:34:41PM +0200, Lorenzo Bianconi wrote:
> > On Fri, Oct 10, 2025 at 07:21:43PM +0200, Lorenzo Bianconi wrote:
> > > Completion napi can free out-of-order tx descriptors if hw QoS is
> > > enabled and packets with different priority are queued to same DMA ring.
> > > Take into account possible out-of-order reports checking if the tx queue
> > > is full using circular buffer head/tail pointer instead of the number of
> > > queued packets.
> > > 
> > > Fixes: 23020f0493270 ("net: airoha: Introduce ethernet support for EN7581 SoC")
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  drivers/net/ethernet/airoha/airoha_eth.c | 15 ++++++++++++++-
> > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > > index 833dd911980b3f698bd7e5f9fd9e2ce131dd5222..5e2ff52dba03a7323141fe9860fba52806279bd0 100644
> > > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > > @@ -1873,6 +1873,19 @@ static u32 airoha_get_dsa_tag(struct sk_buff *skb, struct net_device *dev)
> > >  #endif
> > >  }
> > >  
> > > +static bool airoha_dev_is_tx_busy(struct airoha_queue *q, u32 nr_frags)
> > > +{
> > > +	u16 index = (q->head + nr_frags) % q->ndesc;
> > > +
> > > +	/* completion napi can free out-of-order tx descriptors if hw QoS is
> > > +	 * enabled and packets with different priorities are queued to the same
> > > +	 * DMA ring. Take into account possible out-of-order reports checking
> > > +	 * if the tx queue is full using circular buffer head/tail pointers
> > > +	 * instead of the number of queued packets.
> > > +	 */
> > > +	return index >= q->tail && (q->head < q->tail || q->head > index);
> > 
> > Hi Lorenzo,
> 
> Hi Simon,
> 
> thx for the review.
> 
> > 
> > I think there is a corner case here.
> > Perhaps they can't occur, but here goes.
> > 
> > Let us suppose that head is 1.
> > And the ring is completely full, so tail is 2.
> > 
> > Now, suppose nr_frags is ndesc - 1.
> > In this case the function above will return false. But the ring is full.
> > 
> > Ok, ndesc is actually 1024 and nfrags should never be close to that.
> > But the problem is general. And a perhaps more realistic example is:
> > 
> >   ndesc is 1024
> >   head is 1008
> >   The ring is full so tail is 1009
> >   (Or head is any other value that leaves less than 16 slots free)
> >   nr_frags is 16
> > 
> > airoha_dev_is_tx_busy() returns false, even though the ring is full.
> 
> yes, you are right, this corner case is not properly managed by the proposed
> algorithm, thx for pointing this out.
> 
> > 
> > Probably this has it's own problems. But if my reasoning above is correct
> > (is it?) then the following seems to address it by flattening and extending
> > the ring. Because what we are about is the relative value of head, index
> > and tail. Not the slots they occupy in the ring.
> > 
> > N.B: I tetsed the algorirthm with a quick implementation in user-space.
> > The following is, however, completely untested.
> > 
> > static bool airoha_dev_is_tx_busy(struct airoha_queue *q, u32 nr_frags)
> > {
> > 	unsigned int tail = q->tail < q->head ? q->tail + q->ndesc : q->tail;
> > 	unsigned int index = q->head + nr_frags;
> > 
> > 	return index >= tail;
> > }
> 
> I agree, the algorithm you proposed properly manages the 99% of the cases. The
> only case where it fails is when the queue is empty (so tail = head = x,
> e.g. x = 0). In this case we would have:
> 
> 	- q->ndesc = 1024
> 	- q->tail = q->head = 0
> 	- tail = 0
> 	- index = n (e.g. n = 1)
> 	- index >= tail ==> 1 >= 0 ==> busy (but the queue is actually empty).
> 
> I guess we should add a minor change in the tail definition:
> 
> 	u32 tail = q->tail <= q->head ? q->tail + q->ndesc : q->tail;
> 
> so:
> 	- q->ndesc = 1024
> 	- q->tail = q->head = 0
> 	- tail = 1024
> 	- index = n (e.g. n = 1)
> 	- index >= tail => 1 < 1024 => OK
> 
> Can you spot any downside with this approach?
> I tested the proposed approach and it seems to be working fine.

Thanks, agreed.
Sorry for the out by one error.