Re: [PATCH net 2/3] net: mvneta: fix the removal of the mqprio offload

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After this patch, an mqprio teardown request (all-zero qopt,
  num_tc == 0) always reaches the device-wide reset in…

--

net: mvneta: fix the removal of the mqprio offload

When an offloaded mqprio qdisc is deleted, mqprio_disable_offload()
passes an all-zero qopt with hw set to 0. As a result,
mvneta_setup_mqprio() used to return before reaching its teardown code.
This patch checks qopt.hw only when num_tc is non-zero, so the teardown
path is reached on removal.

> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 9b9efac2846ef..79711a117c39c 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -5251,11 +5251,12 @@ static int mvneta_setup_mqprio(struct net_device *dev,
>  	int rxq, txq, tc, ret;
>  	u8 num_tc;
>  
> -	if (mqprio->qopt.hw != TC_MQPRIO_HW_OFFLOAD_TCS)
> -		return 0;
> -
>  	num_tc = mqprio->qopt.num_tc;
>  
> +	/* On removal the qdisc passes an all-zero qopt, with hw 0 */
> +	if (num_tc && mqprio->qopt.hw != TC_MQPRIO_HW_OFFLOAD_TCS)
> +		return 0;
> +

[Severity: Medium]
Can a root qdisc replace now wipe out the configuration that the new
mqprio qdisc has just offloaded?

With this change, every all-zero teardown request reaches the reset that
covers the whole device, further down in mvneta_setup_mqprio():

	mvneta_clear_rx_prio_map(pp);

	if (!num_tc) {
		mvneta_disable_per_queue_rate_limit(pp);
		netdev_reset_tc(dev);
		return 0;
	}

Take the case where offloaded mqprio 1: is installed and the admin runs
something like "tc qdisc replace dev X root handle 2: mqprio ... hw 1".
__tc_modify_qdisc() does not find handle 2:, so it goes to create_n_graft.
The new qdisc is then initialised before the old one is destroyed:

__tc_modify_qdisc()
  qdisc_create()
    mqprio_init()
      mqprio_enable_offload()
        mvneta_setup_mqprio(num_tc = N, hw = 1)
          programs TC map, VLAN prio to RXQ map, per-queue rate limits
  qdisc_graft()
    notify_and_destroy(old)
      qdisc_put(old)
        mqprio_destroy(old)
          mqprio_disable_offload()
            mvneta_setup_mqprio(num_tc = 0)
              clears RX prio map, rate limits, netdev TC state
    new->ops->attach(new)
      mqprio_attach()

mqprio_destroy() takes this path because the old qdisc has hw_offload
set:

	if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc)
		mqprio_disable_offload(sch);

mqprio_attach() only grafts the child qdiscs and does not reapply the
configuration. The replace returns success and the new qdisc still
reports hw_offload, but its rate limits, RX steering and netdev TC
mapping are gone.

Before this patch, the hw != TC_MQPRIO_HW_OFFLOAD_TCS check turned the old
qdisc's teardown into a no-op, so the new qdisc's configuration was kept.

The same init-before-destroy ordering already resets state in software
mode, since mqprio_destroy() calls netdev_set_num_tc(dev, 0). It also
affects other drivers that reset on num_tc == 0, and mvneta behaved this
way before e7ca75fe6662. On mvneta, though, this patch is what makes the
case reachable again.

>  	if (num_tc > rxq_number)
>  		return -EINVAL;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102112816.12322-1-pepe.schlehofer%40gmail.com