Re: [PATCH net v3 2/2] net: stmmac: preserve real_num_tx_queues on mqprio setup failure

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

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

> With the FPE preemption-class mapping error now propagated from
> stmmac_fpe_map_preemption_class(), tc_setup_dwmac510_mqprio() can fail
> on the mapping step. The error path used to call stmmac_reset_tc_mqprio(),
> which resets the number of real TX queues to priv->plat->tx_queues_to_use
> (the platform maximum), overwriting the value that was active before the
> offload was attempted (for example a lower count left over from a previous
> mqprio configuration).
> 
> The issue can be triggered using the following configuration:
> 
>   # First mqprio config lowers the hw queue count below the platform
>   # default (e.g. 8 TX queues).
>   $tc qdisc add dev eth0 root handle 1: mqprio queues 2@0 2@2
> 
>   # Replace mqprio configuration with a second one that fails FPE
>   # preemption-class mapping. stmmac driver resets the real_num_tx_queues
>   # to the platform maximum, losing the previous configuration.
>   $tc qdisc replace dev eth0 root handle 2: mqprio queues 2@0 2@2 fp E P
> 
> Save ndev->real_num_tx_queues before lowering it and restore it,
> together with the TC-to-queue and priority-to-TC mappings, when the FPE
> preemption-class mapping fails, instead of resetting the queue count to
> the platform maximum.
> 
> Note that a failed setup makes the qdisc layer run mqprio_destroy() on
> the new qdisc. Because priv->hw_offload is only assigned after
> ndo_setup_tc() succeeds, mqprio_destroy() calls netdev_set_num_tc(dev, 0),
> so dev->num_tc ends up 0 regardless of the driver-side restore and the
> previous qdisc is not reactivated. The restore is still needed to keep
> real_num_tx_queues and to avoid leaving the failed configuration's
> TC-to-queue and priority-to-TC mappings in place.
> 
> Fixes: 195e4f409a40 ("net: stmmac: support fp parameter of tc-mqprio")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 82 +++++++++++++++++++------
>  1 file changed, 62 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c

commenting on sashiko's report:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260911-stmmac-tc_setup_dwmac510_mqprio-error-path-v3-0-a76b1e2547c1%40oss.qualcomm.com

[...]

> -	err = netif_set_real_num_tx_queues(ndev, num_stack_tx_queues);
> +	err = stmmac_set_ndev_tcs(ndev, qopt->num_tc, tc_to_txq);
> +	if (err)
> +		goto error_reset_tc;
> +
> +	ndev_num_tx_queues = ndev->real_num_tx_queues;
> +	err = netif_set_real_num_tx_queues(ndev, num_tx_queues);
>  	if (err)
> -		goto err_reset_tc;
> +		goto error_reset_tc;
>  
>  	err = stmmac_fpe_map_preemption_class(priv, ndev, extack,
>  					      mqprio->preemptible_tcs);
>  	if (err)
> -		goto err_reset_tc;
> +		goto error_reset_num_tx_queues;
>  
>  	return 0;
>  
> -err_reset_tc:
> -	stmmac_reset_tc_mqprio(ndev, extack);
> +error_reset_num_tx_queues:
> +	if (netif_set_real_num_tx_queues(ndev, ndev_num_tx_queues))
> +		netdev_warn(ndev, "Failed to restore %u TX queues\n",
> +			    ndev_num_tx_queues);
> +error_reset_tc:
> +	stmmac_set_ndev_tcs(ndev, ndev_ntc, ndev_tc_to_txq);
> +	for (i = 0; i < ARRAY_SIZE(ndev_prio_tc_map); i++)
> +		netdev_set_prio_tc_map(ndev, i, ndev_prio_tc_map[i]);

- Is the loss of the FPE reprogramming step on these two labels intentional?
  The old err_reset_tc path went through stmmac_reset_tc_mqprio(), which ends
  with:
  	return stmmac_fpe_map_preemption_class(priv, ndev, extack, 0);
  The new error_reset_num_tx_queues / error_reset_tc labels only touch netdev
  software state (stmmac_set_ndev_tcs() -> netdev_reset_tc() /
  netdev_set_num_tc() / netdev_set_tc_queue(), plus
  netdev_set_prio_tc_map()), so the hardware mapping is never rewritten.
  - I guess we have already discussed about it. If qdisc replace fails, I think
    the driver should restore the previous offloaded hw configuration. It is then
    up to sch_mqprio qdisc to properly restore the logic. Here sch_mqprio does
    not run mqprio_disable_offload() and this one seems a sch_mqprio bug to me.

Regards,
Lorenzo


>  
>  	return err;
>  }
> 
> -- 
> 2.55.0
> 

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