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

[sashiko-bot@kernel.org]

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?

>  	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?

[ ... ]

> @@ -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.

>  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?

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