Re: [PATCH net v2] net: airoha: fix ETS channel derivation in airoha_tc_setup_qdisc_ets()

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> Derive the hardware QoS channel from opt->parent instead of opt->handle
> in airoha_tc_setup_qdisc_ets(). The ETS qdisc handle is either
> user-specified or auto-allocated by qdisc_alloc_handle() and bears no
> relation to the HTB leaf classid that identifies the hardware channel.
> HTB derives the channel from TC_H_MIN(opt->classid), and ETS is always
> attached as a child of an HTB leaf, so its opt->parent matches that
> classid. Using opt->handle instead can cause two ETS qdiscs on different
> HTB leaves to collide on the same hardware channel, corrupting scheduler
> configuration and stats.
> 
> Fixes: 20bf7d07c956 ("net: airoha: Add sched ETS offload support")
> Reviewed-by: Simon Horman <horms@kernel.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
> Changes in v2:
> - Rebase on top of net main branch
> - Link to v1: https://lore.kernel.org/r/20260704-airoha-ets-handle-fix-v1-1-42516b3549a1@kernel.org
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 59001fd4b6f7..fac2aaefffff 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -2504,8 +2504,7 @@ static int airoha_tc_setup_qdisc_ets(struct net_device *dev,
>  	if (opt->parent == TC_H_ROOT)
>  		return -EINVAL;
>  
> -	channel = TC_H_MAJ(opt->handle) >> 16;
> -	channel = channel % AIROHA_NUM_QOS_CHANNELS;
> +	channel = TC_H_MIN(opt->parent) % AIROHA_NUM_QOS_CHANNELS;
>  
>  	switch (opt->command) {
>  	case TC_ETS_REPLACE:
> 
> ---
> base-commit: e13caf1c26587434f0b768193100440939c0fb91
> change-id: 20260704-airoha-ets-handle-fix-6c166de3d396
> 
> Best regards,
> -- 
> Lorenzo Bianconi <lorenzo@kernel.org>
> 

commenting on sashiko's report:
https://sashiko.dev/#/patchset/20260720-airoha-ets-handle-fix-v2-1-6f7129ddc06f%40kernel.org

- This isn't a bug introduced by this patch, but I noticed a potential
  deadlock issue in airoha_update_hw_stats() in this same file.
  Does the use of a plain spin_lock() here leave the critical section
  vulnerable to softirq re-entrancy?
  - I do not think this is a real issue since airoha_update_hw_stats() can't
    run in softirq context.

Regards,
Lorenzo

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