[PATCH net] net: airoha: Fix TX scheduler queue mask loop upper bound

[PATCH net] net: airoha: Fix TX scheduler queue mask loop upper bound

[Wayen Yan]

In airoha_qdma_set_chan_tx_sched(), the loop clearing queue mask was
using AIROHA_NUM_TX_RING (32) instead of AIROHA_NUM_QOS_QUEUES (8).

Each channel has 8 queues, and TXQ_DISABLE_CHAN_QUEUE_MASK(channel, i)
computes BIT(i + (channel * 8)). With i ranging 0..31, this causes:
- channel 0: clears bit 0..31 (all 4 channels) instead of 0..7
- channel 1: clears bit 8..31 (channels 1-3) instead of 8..15
- channel 2: clears bit 16..31 (channels 2-3) instead of 16..23
- channel 3: clears bit 24..31 (channel 3 only) - correct by accident

While BIT(32+) on arm64 produces 64-bit values truncated to 0 in u32
mask parameter, the loop still incorrectly clears queues within the
same channel beyond queue 7.

Fix by using AIROHA_NUM_QOS_QUEUES (8) as the loop upper bound.

Fixes: ef1ca9271313 ("net: airoha: Add sched HTB offload support")
Signed-off-by: Wayen Yan <win847@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index 31cdb11cd7..a1eda13400 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -2217,7 +2217,7 @@ static int airoha_qdma_set_chan_tx_sched(struct net_device *dev,
 	struct airoha_gdm_port *port = netdev_priv(dev);
 	int i;
 
-	for (i = 0; i < AIROHA_NUM_TX_RING; i++)
+	for (i = 0; i < AIROHA_NUM_QOS_QUEUES; i++)
 		airoha_qdma_clear(port->qdma, REG_QUEUE_CLOSE_CFG(channel),
 				  TXQ_DISABLE_CHAN_QUEUE_MASK(channel, i));
 
-- 
2.51.0

Re: [PATCH net] net: airoha: Fix TX scheduler queue mask loop upper bound

[Lorenzo Bianconi]

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

> In airoha_qdma_set_chan_tx_sched(), the loop clearing queue mask was
> using AIROHA_NUM_TX_RING (32) instead of AIROHA_NUM_QOS_QUEUES (8).
> 
> Each channel has 8 queues, and TXQ_DISABLE_CHAN_QUEUE_MASK(channel, i)
> computes BIT(i + (channel * 8)). With i ranging 0..31, this causes:
> - channel 0: clears bit 0..31 (all 4 channels) instead of 0..7
> - channel 1: clears bit 8..31 (channels 1-3) instead of 8..15
> - channel 2: clears bit 16..31 (channels 2-3) instead of 16..23
> - channel 3: clears bit 24..31 (channel 3 only) - correct by accident
> 
> While BIT(32+) on arm64 produces 64-bit values truncated to 0 in u32
> mask parameter, the loop still incorrectly clears queues within the
> same channel beyond queue 7.
> 
> Fix by using AIROHA_NUM_QOS_QUEUES (8) as the loop upper bound.
> 
> Fixes: ef1ca9271313 ("net: airoha: Add sched HTB offload support")
> Signed-off-by: Wayen Yan <win847@gmail.com>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 31cdb11cd7..a1eda13400 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -2217,7 +2217,7 @@ static int airoha_qdma_set_chan_tx_sched(struct net_device *dev,
>  	struct airoha_gdm_port *port = netdev_priv(dev);
>  	int i;
>  
> -	for (i = 0; i < AIROHA_NUM_TX_RING; i++)
> +	for (i = 0; i < AIROHA_NUM_QOS_QUEUES; i++)
>  		airoha_qdma_clear(port->qdma, REG_QUEUE_CLOSE_CFG(channel),
>  				  TXQ_DISABLE_CHAN_QUEUE_MASK(channel, i));

Even if the current codebase supports just AIROHA_NUM_QOS_CHANNEL (4), the hw
exposes 32 hw QoS channels (AIROHA_NUM_TX_RING). Here we are just clearing the
configuration, so I guess the current implementation is correct.

Regards,
Lorenzo

>  
> -- 
> 2.51.0
> 
> 

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

Re: [PATCH net] net: airoha: Fix TX scheduler queue mask loop upper bound

[Wayen Yan]

On Tue, Jun 17, 2026, Lorenzo Bianconi wrote:
> Even if the current codebase supports just AIROHA_NUM_QOS_CHANNEL (4), the hw
> exposes 32 hw QoS channels (AIROHA_NUM_TX_RING). Here we are just clearing the
> configuration, so I guess the current implementation is correct.

Hi Lorenzo,

You are right that there is no functional impact, and I agree this
should not go to net. Let me explain the register layout I was worried
about, and you can decide whether it is worth a net-next cleanup or
should just be dropped.

The two macros are:

	REG_QUEUE_CLOSE_CFG(_n)             = 0x00a0 + ((_n) & 0xfc)
	TXQ_DISABLE_CHAN_QUEUE_MASK(_n, _m) = BIT((_m) + (((_n) & 0x3) << 3))

REG_QUEUE_CLOSE_CFG() masks the channel with 0xfc, and the bit macro
folds the channel with & 0x3 (mod 4) shifted by 3. So one 32-bit
register holds 4 channels x 8 queues, 8 queue bits per channel:

	channel 0 -> reg 0x00a0, bits  0..7
	channel 1 -> reg 0x00a0, bits  8..15
	channel 2 -> reg 0x00a0, bits 16..23
	channel 3 -> reg 0x00a0, bits 24..31
	channel 4 -> reg 0x00a4, bits  0..7
	...

In airoha_qdma_set_chan_tx_sched() the loop variable 'i' is passed as
the *queue* argument _m, not as a channel:

	for (i = 0; i < AIROHA_NUM_TX_RING; i++)   // i = 0..31
		airoha_qdma_clear(qdma, REG_QUEUE_CLOSE_CFG(channel),
				  TXQ_DISABLE_CHAN_QUEUE_MASK(channel, i));

Since each channel only has AIROHA_NUM_QOS_QUEUES (8) queues, the correct
logic is to clear the 8 queue bits belonging to 'channel'. With i running
up to 31 the BIT() shift instead walks past those 8 bits and into the bit
ranges of the other channels folded into the same register. For channel 0
the accumulated mask becomes 0xffffffff, i.e. it touches channels 1..3 as
well.

This is harmless today only because REG_QUEUE_CLOSE_CFG is written
exclusively here, via airoha_qdma_clear() (RMW clear), and the register
resets to 0 and is never set anywhere -- so clearing extra bits is a
no-op. Functionally the current code is fine, as you say.

The point is just the loop-bound semantics: 'i' is a per-channel queue
index, so the bound should be AIROHA_NUM_QOS_QUEUES (8), not
AIROHA_NUM_TX_RING (32). The two happen to be related (32 == 4 channels *
8 queues) but mean different things.

Since there is no functional change, feel free to drop this if you would
rather not carry a cosmetic patch. If you think the clarity is worth it I
can resend against net-next without the Fixes tag.

Thanks,
Wayen

Re: [PATCH net] net: airoha: Fix TX scheduler queue mask loop upper bound

[Lorenzo Bianconi]

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

> On Tue, Jun 17, 2026, Lorenzo Bianconi wrote:
> > Even if the current codebase supports just AIROHA_NUM_QOS_CHANNEL (4), the hw
> > exposes 32 hw QoS channels (AIROHA_NUM_TX_RING). Here we are just clearing the
> > configuration, so I guess the current implementation is correct.
> 
> Hi Lorenzo,
> 
> You are right that there is no functional impact, and I agree this
> should not go to net. Let me explain the register layout I was worried
> about, and you can decide whether it is worth a net-next cleanup or
> should just be dropped.
> 
> The two macros are:
> 
> 	REG_QUEUE_CLOSE_CFG(_n)             = 0x00a0 + ((_n) & 0xfc)
> 	TXQ_DISABLE_CHAN_QUEUE_MASK(_n, _m) = BIT((_m) + (((_n) & 0x3) << 3))
> 
> REG_QUEUE_CLOSE_CFG() masks the channel with 0xfc, and the bit macro
> folds the channel with & 0x3 (mod 4) shifted by 3. So one 32-bit
> register holds 4 channels x 8 queues, 8 queue bits per channel:
> 
> 	channel 0 -> reg 0x00a0, bits  0..7
> 	channel 1 -> reg 0x00a0, bits  8..15
> 	channel 2 -> reg 0x00a0, bits 16..23
> 	channel 3 -> reg 0x00a0, bits 24..31
> 	channel 4 -> reg 0x00a4, bits  0..7
> 	...
> 
> In airoha_qdma_set_chan_tx_sched() the loop variable 'i' is passed as
> the *queue* argument _m, not as a channel:
> 
> 	for (i = 0; i < AIROHA_NUM_TX_RING; i++)   // i = 0..31
> 		airoha_qdma_clear(qdma, REG_QUEUE_CLOSE_CFG(channel),
> 				  TXQ_DISABLE_CHAN_QUEUE_MASK(channel, i));
> 
> Since each channel only has AIROHA_NUM_QOS_QUEUES (8) queues, the correct
> logic is to clear the 8 queue bits belonging to 'channel'. With i running
> up to 31 the BIT() shift instead walks past those 8 bits and into the bit
> ranges of the other channels folded into the same register. For channel 0
> the accumulated mask becomes 0xffffffff, i.e. it touches channels 1..3 as
> well.
> 
> This is harmless today only because REG_QUEUE_CLOSE_CFG is written
> exclusively here, via airoha_qdma_clear() (RMW clear), and the register
> resets to 0 and is never set anywhere -- so clearing extra bits is a
> no-op. Functionally the current code is fine, as you say.
> 
> The point is just the loop-bound semantics: 'i' is a per-channel queue
> index, so the bound should be AIROHA_NUM_QOS_QUEUES (8), not
> AIROHA_NUM_TX_RING (32). The two happen to be related (32 == 4 channels *
> 8 queues) but mean different things.
> 
> Since there is no functional change, feel free to drop this if you would
> rather not carry a cosmetic patch. If you think the clarity is worth it I
> can resend against net-next without the Fixes tag.
> 
> Thanks,
> Wayen
> 

Sorry you are right, I misread the code, your patch is correct. Since as you
pointed out REG_QUEUE_CLOSE_CFG() is actually never set at the moment and the
default register value is 0, I would repost this patch for net-next as soon as
it is opened (this will avoid merge conflicts).

Regards,
Lorenzo

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