Re: [PATCH net-next v5 06/12] net: airoha: Move qos_sq_bmap in airoha_gdm_dev struct

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] HTB node modification unconditionally fails with -EBUSY and its error path can silently corrupt global channel ownership.
> - [High] Hardware teardown failures cause a permanent leak of shared QDMA channels.
> --
> 
> commit 1b146a034660fa016267e6a142564dd1ddeb17b1
> 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 configuration overwrites when multiple net_devices share a GDM port.
> It also adds a qos_channel_map bitmap to airoha_qdma to track shared QDMA
> channels.
> 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index f9cb49f0529a0..33a0f7b98d52e 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -2606,19 +2606,28 @@ static int airoha_tc_htb_alloc_leaf_queue(struct net_device *netdev,
> >  	u32 rate = div_u64(opt->rate, 1000) << 3; /* kbps */
> >  	int err, num_tx_queues = netdev->real_num_tx_queues;
> >  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > -	struct airoha_gdm_port *port = dev->port;
> > +	struct airoha_qdma *qdma = dev->qdma;
> >  
> >  	if (opt->parent_classid != TC_HTB_CLASSID_ROOT) {
> >  		NL_SET_ERR_MSG_MOD(opt->extack, "invalid parent classid");
> >  		return -EINVAL;
> >  	}
> >  
> > +	/* Here we need to check the requested QDMA channel is not already
> > +	 * in use by another net_device running on the same QDMA block.
> > +	 */
> > +	if (test_and_set_bit(channel, qdma->qos_channel_map)) {
> > +		NL_SET_ERR_MSG_MOD(opt->extack,
> > +				   "qdma qos channel already in use");
> > +		return -EBUSY;
> > +	}
> > +
> 
> Since this function handles both TC_HTB_LEAF_ALLOC_QUEUE and
> TC_HTB_NODE_MODIFY, won't the bit already be set in qdma->qos_channel_map
> for modify operations?
> 
> If so, will modify operations now always fail with -EBUSY here,
> breaking HTB rate limit updates?

ack, I will fix it in v6

> 
> >  	err = airoha_qdma_set_tx_rate_limit(netdev, channel, rate,
> >  					    opt->quantum);
> >  	if (err) {
> >  		NL_SET_ERR_MSG_MOD(opt->extack,
> >  				   "failed configuring htb offload");
> > -		return err;
> > +		goto error;
> >  	}
> >  
> >  	if (opt->command == TC_HTB_NODE_MODIFY)
> > @@ -2630,13 +2639,17 @@ static int airoha_tc_htb_alloc_leaf_queue(struct net_device *netdev,
> >  					      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;
> >  
> >  	return 0;
> > +error:
> > +	clear_bit(channel, qdma->qos_channel_map);
> > +
> > +	return err;
> >  }
> 
> If a modify operation (TC_HTB_NODE_MODIFY) fails at
> airoha_qdma_set_tx_rate_limit(), it jumps to the error label.
> 
> Won't this unconditionally clear the bit in the shared qdma->qos_channel_map?
> 
> Could this accidentally release ownership of the shared QDMA channel while
> the device is still using it, allowing another net_device to claim it?

ack, I will fix it in v6

> 
> [ ... ]
> 
> > @@ -2814,14 +2827,28 @@ static int airoha_dev_setup_tc_block(struct net_device *dev,
> >  	}
> >  }
> >  
> > -static void airoha_tc_remove_htb_queue(struct net_device *netdev, int queue)
> > +static int 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;
> > +	int err;
> >  
> > -	netif_set_real_num_tx_queues(netdev, netdev->real_num_tx_queues - 1);
> > -	airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0);
> > -	clear_bit(queue, port->qos_sq_bmap);
> > +	err = netif_set_real_num_tx_queues(netdev,
> > +					   netdev->real_num_tx_queues - 1);
> > +	if (err)
> > +		return err;
> > +
> > +	err = airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0);
> > +	if (err) {
> > +		netif_set_real_num_tx_queues(netdev,
> > +					     netdev->real_num_tx_queues + 1);
> > +		return err;
> > +	}
> > +
> > +	clear_bit(queue, qdma->qos_channel_map);
> > +	clear_bit(queue, dev->qos_sq_bmap);
> > +
> > +	return 0;
> >  }
> 
> If a hardware teardown failure occurs and airoha_tc_remove_htb_queue()
> returns early with an error, the bits in qdma->qos_channel_map and
> dev->qos_sq_bmap are never cleared.

ack, I will fix it in v6

> 
> >  static int airoha_tc_htb_destroy(struct net_device *netdev)
> >  {
> >  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> > -	struct airoha_gdm_port *port = dev->port;
> >  	int q;
> >  
> > -	for_each_set_bit(q, port->qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS)
> > -		airoha_tc_remove_htb_queue(netdev, q);
> > +	for_each_set_bit(q, dev->qos_sq_bmap, AIROHA_NUM_QOS_CHANNELS) {
> > +		int err;
> > +
> > +		err = airoha_tc_remove_htb_queue(netdev, q);
> > +		if (err)
> > +			return err;
> > +	}
> >  
> >  	return 0;
> >  }
> 
> In this loop, an error on a single queue immediately aborts the loop, leaving
> any remaining queues unprocessed.
> 
> Since the core networking stack (net/sched/sch_htb.c) ignores the return value
> of TC_HTB_DESTROY and fully destroys the software qdisc regardless, will this
> permanently leak the limited QDMA hardware channels?
> 
> Could these bits remain set forever in the globally shared qdma->qos_channel_map,
> preventing other net_devices from using them until the driver is reloaded?

ack, I will fix it in v6

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260509-airoha-eth-multi-serdes-v5-0-805e38edc2aa@kernel.org?part=6

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