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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Off-by-one error in airoha_tc_remove_htb_queue when clearing hardware rate limit.
> - [High] Blind increment/decrement of real_num_tx_queues breaks HTB offload for non-sequential queue IDs.
> --
> 
> commit 7088e1abda5f2f9f4a048fc5586684fdaf382845
> Author: Lorenzo Bianconi <lorenzo@kernel.org>
> 
> net: airoha: Move qos_sq_bmap in airoha_gdm_dev struct
> 
> This commit moves the qos_sq_bmap field from airoha_gdm_port to
> airoha_gdm_dev to prevent overwrites when multiple net_devices share a GDM
> port. It also adds qos_channel_map to track QDMA channels in use.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index b90fc1cbfbc48..2a2a30dbad27e 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -2597,44 +2597,61 @@ static int airoha_qdma_set_tx_rate_limit(struct net_device *netdev,
> 
> [ ... ]
> 
> >  	err = netif_set_real_num_tx_queues(netdev, num_tx_queues + 1);
> >  	if (err) {
> >  		airoha_qdma_set_tx_rate_limit(netdev, channel, 0,
> >  					      opt->quantum);
> >  		NL_SET_ERR_MSG_MOD(opt->extack,
> >  				   "failed setting real_num_tx_queues");
> > -		return err;
> > +		goto error;
> >  	}
> >  
> > -	set_bit(channel, port->qos_sq_bmap);
> > +	set_bit(channel, dev->qos_sq_bmap);
> >  	opt->qid = AIROHA_NUM_TX_RING + channel;
> 
> This problem wasn't introduced by this patch, but is a logic flaw that
> breaks the offload functionality.
> 
> When allocating a QoS channel, real_num_tx_queues is blindly incremented by
> 1 while the qid is derived from the channel. If a user allocates a
> non-sequential classid (for example, mapping to channel 3 when it is the
> first allocated queue), real_num_tx_queues will become
> AIROHA_NUM_TX_RING + 1, but qid will be AIROHA_NUM_TX_RING + 3.
> 
> When transmitting, netdev_cap_txqueue() strictly validates that the packet's
> queue_mapping is < real_num_tx_queues.
> 
> Since qid exceeds the blindly incremented count, will the stack log a warning
> and forcefully route the packet to queue 0, silently bypassing the HTB
> hardware rate limit?
> 
> Additionally, does blindly decrementing the count when deleting queues
> immediately invalidate any active higher-indexed queues?

Since this issue has not been introduced by this series, I will fix it with a
dedicated patch.

> 
> > @@ -2815,11 +2832,13 @@ 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);
> >  	airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0);
> 
> This problem wasn't introduced by this patch, but the patch exposes it to
> cross-device corruption.
> 
> In airoha_tc_remove_htb_queue(), the driver attempts to disable the hardware
> rate limit for the removed queue by calling airoha_qdma_set_tx_rate_limit()
> with queue + 1. However, the queue is originally allocated and configured
> using the channel index (where channel equals queue).
> 
> Passing queue + 1 fails to clear the rate limit of the actually deleted queue,
> permanently leaking its hardware state. Furthermore, it incorrectly zeros out
> the rate limit of the adjacent queue.
> 
> Because this patch allows multiple net_devices to share the same QDMA block
> and its channels, could deleting a QoS queue on one interface unexpectedly
> corrupt the QoS hardware state of another interface actively using the
> adjacent channel?

Since this issue has not been introduced by this series, I will fix it with a
dedicated patch.

Regards,
Lorenzo

> 
> > -	clear_bit(queue, port->qos_sq_bmap);
> > +
> > +	clear_bit(queue, qdma->qos_channel_map);
> > +	clear_bit(queue, dev->qos_sq_bmap);
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260516-airoha-eth-multi-serdes-v7-0-99e0093303e2@kernel.org?part=5

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