Re: [PATCH net v3 3/5] net: mvneta: unwind a failed mqprio setup

[netdev-bot+sashiko]

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] In drivers/net/ethernet/marvell/mvneta.c, the new `err:` label
  in mvneta_setup_mqprio() does not put back the hardware state that…

--

net: mvneta: unwind a failed mqprio setup

This commit adds an err: label to mvneta_setup_mqprio(). When a mqprio
offload setup fails, the driver now clears the VLAN priority to RX queue
map, disables the per-queue TX rate limits and resets the netdev traffic
classes, so no partial configuration is left in hardware.

> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 79711a117c39c..171111d7fe57d 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c

[ ... ]

> @@ -5289,29 +5291,39 @@ static int mvneta_setup_mqprio(struct net_device *dev,

[ ... ]

>  			ret = mvneta_setup_queue_rates(pp, txq,
>  						       mqprio->min_rate[tc],
>  						       mqprio->max_rate[tc]);
>  			if (ret)
> -				return ret;
> +				goto err;
>  		}
>  	}
>  
>  	return 0;
> +
> +err:
> +	mvneta_clear_rx_prio_map(pp);
> +	mvneta_disable_per_queue_rate_limit(pp);
> +	netdev_reset_tc(dev);
> +	return ret;
>  }

[Severity: Medium]
Can this unwind also remove the TX rate limits of an mqprio qdisc that is
already attached, when replacing it fails?

For a failed fresh attach, going back to "no offload" is right. Now take
a shaped, offloaded mqprio that is root as 1:. It gets replaced under a
new handle with a bad parameter, for example:

  tc qdisc replace dev eth0 root handle 2: mqprio ... hw 1 mode channel \
     shaper bw_rate max_rate 10001kbit ...

The new qdisc is initialised while 1: is still grafted:

tc_modify_qdisc()
  create_n_graft:
  qdisc_create()
    mqprio_init()
      mqprio_enable_offload()
        mvneta_setup_mqprio()
          mvneta_setup_queue_rates() fails
          goto err
            mvneta_disable_per_queue_rate_limit()

qdisc_create() then calls mqprio_destroy() on the new qdisc.
mqprio_enable_offload() sets priv->hw_offload only after ndo_setup_tc
succeeds, so the driver is not called again and 1: stays root.

Before this patch, these failures returned before any of 1:'s per-queue
TX rates were overwritten:

  - the RX queue range check
  - num_tc > txq_number
  - mvneta_enable_per_queue_rate_limit()
  - the first TX queue

Now every one of those paths goes through err: and refills all TX buckets
at full rate. Meanwhile tc qdisc show still lists 1: with its max_rate
values.

The replace case was already partly inconsistent before this patch, since
the RX prio map is cleared before validation. Losing 1:'s TX rate limits
is new, though.

"net: mvneta: lift the per-queue TX rate limits", earlier in the series,
made mvneta_disable_per_queue_rate_limit() also clear
pp->txqs[].bucket_refill:

	for (queue = 0; queue < txq_number; queue++) {
		pp->txqs[queue].bucket_refill = 0;
		mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);
	}

Because of that, mvneta_restore_per_queue_rate_limit() will not bring 1:'s
limits back on the next open either.

The earlier series commit "net: mvneta: fix the removal of the mqprio
offload" says that "A replacement qdisc is set up before the old one is
torn down". This commit message only covers the fresh attach case.

Would it work to validate the whole configuration before touching the
hardware? That would cover the RX/TX queue ranges, num_tc against
txq_number, min_rate, and the resolution and range of each max_rate. It
would also need to happen before the mvneta_clear_rx_prio_map() and
netdev_set_num_tc() calls at the top of mvneta_setup_mqprio(). A rejected
configuration would then leave the previous offload in place.

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