Re: [PATCH net-next v4 05/10] net: airoha: Move qos_sq_bmap in airoha_qdma struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Pre-existing off-by-one error in hardware queue rate limit clearing during HTB queue removal.
- [High] Pre-existing logic error in `real_num_tx_queues` management breaks hardware QoS offload for out-of-order queue mapping.
- [High] Pre-existing missing resource collision check in HTB queue allocation allows silent hardware state overwrites and resource leaks.
--

commit f382d3d7cd42468f4ce246a12779bca6a0b8a1e1
Author: Lorenzo Bianconi <lorenzo@kernel.org>

net: airoha: Move qos_sq_bmap in airoha_qdma struct

This patch moves the qos_sq_bmap field from the airoha_gdm_port struct to
airoha_qdma to prevent configuration overwrites when multiple net_devices
connected to different QDMA blocks share the same GDM port.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 080705e2f58d7..69a4c2e0d58b7 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -2627,7 +2627,7 @@ static int airoha_tc_htb_alloc_leaf_queue(struct net_device *netdev,
>  		return err;
>  	}
>  
> -	set_bit(channel, port->qos_sq_bmap);
> +	set_bit(channel, qdma->qos_sq_bmap);

This wasn't introduced by this patch, but is it possible for multiple classes
to map to the same channel here since channel is derived via modulo?

If they do, does the code overwrite the hardware rate limit of the first
class and increment real_num_tx_queues again without checking if the bit was
already set in qos_sq_bmap?

>  	opt->qid = AIROHA_NUM_TX_RING + channel;

Also a pre-existing issue: because channel is derived via modulo and can be
sparse, opt->qid can end up being >= netdev->real_num_tx_queues. 

Will netdev_core_pick_tx() see an out-of-bounds queue mapping during
transmission and fall back to standard hashing, bypassing the hardware
offload?

>  
>  	return 0;
>  }
[ ... ]
> @@ -2811,11 +2811,11 @@ static int airoha_dev_setup_tc_block(struct net_device *dev,
>  static void airoha_tc_remove_htb_queue(struct net_device *netdev, int queue)
>  {
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> -	struct airoha_gdm_port *port = dev->port;
> +	struct airoha_qdma *qdma = dev->qdma;
>  
>  	netif_set_real_num_tx_queues(netdev, netdev->real_num_tx_queues - 1);

Following up on the real_num_tx_queues logic above, does blindly decrementing
this by 1 dynamically shrink the queue range and instantly invalidate other
active queues whose qid might now fall out of bounds?

>  	airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0);

Does using queue + 1 here instead of queue leave the deleted channel's rate
limit fully active while incorrectly wiping the rate limit of the adjacent
channel?

If the deleted channel is AIROHA_NUM_QOS_CHANNELS - 1, could this also pass
an out-of-bounds channel to the hardware configuration functions?

> -	clear_bit(queue, port->qos_sq_bmap);
> +	clear_bit(queue, qdma->qos_sq_bmap);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507-airoha-eth-multi-serdes-v4-0-af613b61ae02@kernel.org?part=5