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

> 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 <[email protected]>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> Changes in v2:
> - Rebase on top of net main branch
> - Link to v1: https://lore.kernel.org/r/[email protected]
> ---
>  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 <[email protected]>
> 

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

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCal/hHAAKCRA6cBh0uS2t
rHpVAQDoSaeJJgiMO+dfAiPzLV/tz/bqTE769wq89vWeXOijVAEA2MDZOX3/wC9U
et9XWnaEulJXCrO58U4dNN9LJToQqQA=
=MDOM
-----END PGP SIGNATURE-----