[PATCH net v2 0/5] net: mvneta: fix the TX queue arbitration and the mqprio offload

[PATCH net v2 0/5] net: mvneta: fix the TX queue arbitration and the mqprio offload

[Josef Schlehofer]

The NETA TX arbiter is left at its reset default, which serves the TX
queues in fixed priority. Under load a busy queue starves the others.
OpenWrt has worked around this since 2017 by sending all TX traffic
of its 32-bit boards through queue 0 [1].

Patch 3 serves the queues in weighted round robin. While working on it,
four older bugs in the mqprio offload showed up. Turning the shaper off
leaves the per-queue rate limits in place (patch 1). Deleting the qdisc
never reaches the driver (patch 2). A failed setup is not undone
(patch 4). Opening the interface or changing its MTU sets every rate
limit to 10.23 Mbit/s (patch 5).

The series was tested on a Turris Omnia (Armada 385). The other NETA
variants were not tested, but the driver already programs registers of
the same block (MVNETA_TX_MTU and the token sizes) on all of them.
Suspend and resume were not tested.

With patch 2, replacing an offloaded mqprio qdisc with another mqprio
under a new handle loses the new configuration. The new qdisc is set up
before the old one is torn down, and the driver now acts on that
teardown. Software mqprio loses the configuration in the same way.

Changes in v2:
- Add patches 4 and 5. They fix the pre-existing issues that the
  Sashiko review pointed out.
- Patch 1: say that deleting the qdisc reaches the new code only with
  patch 2. When MVNETA_REFILL_NUM_CLK_REG was never programmed, the
  full rate refill keeps the refill period that the queues have after
  mvneta_txq_hw_init() and only raises the tokens, so it is not slower
  than before.
- Patch 2: describe the replace case. The v1 cover letter called it an
  existing limitation, but before this patch the driver kept the new
  configuration.
- Patch 3: add a missing blank line in the commit message.
- No code change in patches 1 to 3. Rebased on current net/main.
- v1: https://lore.kernel.org/netdev/20261102112816.12322-1-pepe.schlehofer@gmail.com/

[1] https://github.com/openwrt/openwrt/blob/a5e96c837ead549820867bb83c1ad471e39f5ace/target/linux/mvebu/patches-6.18/700-mvneta-tx-queue-workaround.patch

Josef Schlehofer (5):
  net: mvneta: lift the per-queue TX rate limits
  net: mvneta: fix the removal of the mqprio offload
  net: mvneta: serve the TX queues in weighted round robin
  net: mvneta: unwind a failed mqprio setup
  net: mvneta: keep the TX rate limits across open and MTU change

 drivers/net/ethernet/marvell/mvneta.c | 91 +++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 11 deletions(-)


base-commit: f56daaadfecae1f760d0fb34385b256d3af68970
-- 
2.54.0 (Apple Git-157)

[PATCH net v2 1/5] net: mvneta: lift the per-queue TX rate limits

[Josef Schlehofer]

The per-queue rate limits set by mqprio stay in effect after the
shaper is turned off.

mvneta_disable_per_queue_rate_limit() only changes the mode bits in
MVNETA_TXQ_CMD1_REG. The token buckets keep limiting the TX queues
with that mode as well, and their refill rate still holds the old
limit.

Refill the token buckets of all TX queues at full rate when the rate
limit is disabled. Deleting the qdisc reaches this code only with the
next patch.

Tested on a Turris Omnia (Armada 385) with 10 Mbit/s limits on a
100 Mbit/s link. When mqprio was set up again without a shaper, each
TX queue stayed at 10 Mbit/s. With this patch the link reaches
94 Mbit/s.

Fixes: 2551dc9e398c ("net: mvneta: Add TC traffic shaping offload")
Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index ea755cf05b9e..9b9efac2846e 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -5204,9 +5204,16 @@ static int mvneta_enable_per_queue_rate_limit(struct mvneta_port *pp)
 static void mvneta_disable_per_queue_rate_limit(struct mvneta_port *pp)
 {
 	u32 val = mvreg_read(pp, MVNETA_TXQ_CMD1_REG);
+	int queue;
 
 	val |= (MVNETA_TXQ_CMD1_BW_LIM_SEL_V1 | MVNETA_TXQ_CMD1_BW_LIM_EN);
 	mvreg_write(pp, MVNETA_TXQ_CMD1_REG, val);
+
+	/* The token buckets still limit the queues, refill them at full rate */
+	val = (1 << MVNETA_TXQ_BUCKET_REFILL_PERIOD_SHIFT) |
+	      MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX;
+	for (queue = 0; queue < txq_number; queue++)
+		mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);
 }
 
 static int mvneta_setup_queue_rates(struct mvneta_port *pp, int queue,
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v2 1/5] net: mvneta: lift the per-queue TX rate limits

[Maxime Chevallier]

Hi,

On 11/3/26 13:50, Josef Schlehofer wrote:
> The per-queue rate limits set by mqprio stay in effect after the
> shaper is turned off.
> 
> mvneta_disable_per_queue_rate_limit() only changes the mode bits in
> MVNETA_TXQ_CMD1_REG. The token buckets keep limiting the TX queues
> with that mode as well, and their refill rate still holds the old
> limit.
> 
> Refill the token buckets of all TX queues at full rate when the rate
> limit is disabled. Deleting the qdisc reaches this code only with the
> next patch.
> 
> Tested on a Turris Omnia (Armada 385) with 10 Mbit/s limits on a
> 100 Mbit/s link. When mqprio was set up again without a shaper, each
> TX queue stayed at 10 Mbit/s. With this patch the link reaches
> 94 Mbit/s.
> 
> Fixes: 2551dc9e398c ("net: mvneta: Add TC traffic shaping offload")
> Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks for addressing this,

Maxime

> ---
>  drivers/net/ethernet/marvell/mvneta.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index ea755cf05b9e..9b9efac2846e 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -5204,9 +5204,16 @@ static int mvneta_enable_per_queue_rate_limit(struct mvneta_port *pp)
>  static void mvneta_disable_per_queue_rate_limit(struct mvneta_port *pp)
>  {
>  	u32 val = mvreg_read(pp, MVNETA_TXQ_CMD1_REG);
> +	int queue;
>  
>  	val |= (MVNETA_TXQ_CMD1_BW_LIM_SEL_V1 | MVNETA_TXQ_CMD1_BW_LIM_EN);
>  	mvreg_write(pp, MVNETA_TXQ_CMD1_REG, val);
> +
> +	/* The token buckets still limit the queues, refill them at full rate */
> +	val = (1 << MVNETA_TXQ_BUCKET_REFILL_PERIOD_SHIFT) |
> +	      MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX;
> +	for (queue = 0; queue < txq_number; queue++)
> +		mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);
>  }
>  
>  static int mvneta_setup_queue_rates(struct mvneta_port *pp, int queue,

Re: [PATCH net v2 1/5] net: mvneta: lift the per-queue TX rate limits

[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 code in
  mvneta_disable_per_queue_rate_limit() writes refill period 1 and…

--

net: mvneta: lift the per-queue TX rate limits

After this patch, mvneta_disable_per_queue_rate_limit() also writes refill
period 1 and the maximum refill value into the token bucket refill register
of every TX queue. Before, it only changed the mode bits in
MVNETA_TXQ_CMD1_REG. The goal is to stop the old mqprio per-queue limits
from staying in effect once the shaper is turned off.

> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index ea755cf05b9ef..9b9efac2846ef 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -5204,9 +5204,16 @@ static int mvneta_enable_per_queue_rate_limit(struct mvneta_port *pp)
>  static void mvneta_disable_per_queue_rate_limit(struct mvneta_port *pp)
>  {
>  	u32 val = mvreg_read(pp, MVNETA_TXQ_CMD1_REG);
> +	int queue;
>  
>  	val |= (MVNETA_TXQ_CMD1_BW_LIM_SEL_V1 | MVNETA_TXQ_CMD1_BW_LIM_EN);
>  	mvreg_write(pp, MVNETA_TXQ_CMD1_REG, val);
> +
> +	/* The token buckets still limit the queues, refill them at full rate */
> +	val = (1 << MVNETA_TXQ_BUCKET_REFILL_PERIOD_SHIFT) |
> +	      MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX;
> +	for (queue = 0; queue < txq_number; queue++)
> +		mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);

[Severity: Medium]
Is "full rate" guaranteed here when MVNETA_REFILL_NUM_CLK_REG has never
been programmed?

The period field is counted in units of the base period:

/* Number of Base Period to wait between each bucket refill */
#define MVNETA_TXQ_BUCKET_REFILL_PERIOD	1000

The only place the driver writes that base period is
mvneta_enable_per_queue_rate_limit():

	/* Set the base refill rate */
	mvreg_write(pp, MVNETA_REFILL_NUM_CLK_REG, refill_cycles);

Neither mvneta_defaults_set() nor probe write it. Enable also returns
before this write if clk_get_rate() gives 0 or refill_cycles is greater
than MVNETA_REFILL_MAX_NUM_CLK.

On a fresh boot, adding an offloaded mqprio without a shaper goes
straight to the disable path, and no enable has run before it:

mvneta_setup_tc()->mvneta_setup_mqprio():

	if (mqprio->shaper != TC_MQPRIO_SHAPER_BW_RATE) {
		mvneta_disable_per_queue_rate_limit(pp);
		return 0;
	}

Before this patch, the hardware's reset-default bucket values were left
alone on this path. Now they are overwritten. The resulting rate then
depends on the unknown reset value of MVNETA_REFILL_NUM_CLK_REG and on
MVNETA_TXQ_TOKEN_SIZE_REG(q). The driver only guarantees the token size
is at least MTU * 8 bits, via mvneta_txq_max_tx_size_set().

The units of the base period register are documented for V3 mode. This
write happens right after the port is switched to V1 mode
(SEL_V1 | BW_LIM_EN). Does the period field mean the same thing there?

The test in the commit message covers the case where an earlier enable
had already set the base period to about 100 ns. Could the base period
also be programmed here? The comparable mvpp2 code writes
MVPP2_TXP_SCHED_PERIOD_REG before it sets refill period 1 and the maximum
tokens.

Later patches in the series also reach this function from qdisc deletion
and from the error unwind, and the base period is still not set there.

>  }
>  
>  static int mvneta_setup_queue_rates(struct mvneta_port *pp, int queue,

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

[PATCH net v2 2/5] net: mvneta: fix the removal of the mqprio offload

[Josef Schlehofer]

Deleting an offloaded mqprio qdisc leaves its traffic classes and
per-queue rate limits in place.

mqprio_disable_offload() passes an all-zero configuration, so qopt.hw
is 0 and mvneta_setup_mqprio() returns before it reaches the teardown
code.

Check qopt.hw only when traffic classes are set up.

A replacement qdisc is set up before the old one is torn down.
Replacing an offloaded mqprio qdisc with another mqprio under a new
handle therefore loses the new configuration, as with software mqprio.

Tested on a Turris Omnia (Armada 385) with 10 Mbit/s limits on a
100 Mbit/s link. After the qdisc was deleted, only TX queues 0-3 were
used and the link carried 19 Mbit/s. With this patch and the previous
one, the queues above 3 are used again and the link reaches 94 Mbit/s.

Fixes: e7ca75fe6662 ("net: mvneta: Don't force-set the offloading flag")
Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 9b9efac2846e..79711a117c39 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;
+
 	if (num_tc > rxq_number)
 		return -EINVAL;
 
-- 
2.54.0 (Apple Git-157)

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

[Maxime Chevallier]

Hi,

On 11/3/26 13:50, Josef Schlehofer wrote:
> Deleting an offloaded mqprio qdisc leaves its traffic classes and
> per-queue rate limits in place.
> 
> mqprio_disable_offload() passes an all-zero configuration, so qopt.hw
> is 0 and mvneta_setup_mqprio() returns before it reaches the teardown
> code.
> 
> Check qopt.hw only when traffic classes are set up.
> 
> A replacement qdisc is set up before the old one is torn down.
> Replacing an offloaded mqprio qdisc with another mqprio under a new
> handle therefore loses the new configuration, as with software mqprio.
> 
> Tested on a Turris Omnia (Armada 385) with 10 Mbit/s limits on a
> 100 Mbit/s link. After the qdisc was deleted, only TX queues 0-3 were
> used and the link carried 19 Mbit/s. With this patch and the previous
> one, the queues above 3 are used again and the link reaches 94 Mbit/s.
> 
> Fixes: e7ca75fe6662 ("net: mvneta: Don't force-set the offloading flag")
> Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
>  drivers/net/ethernet/marvell/mvneta.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 9b9efac2846e..79711a117c39 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;
> +
>  	if (num_tc > rxq_number)
>  		return -EINVAL;
>  

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message of "net: mvneta: fix the removal of the mqprio
  offload" leaves out several things that…

Pre-existing issues:
- [Medium] In this patch's tree, mvneta_setup_mqprio()
  (drivers/net/ethernet/marvell/mvneta.c) returns -EINVAL or ret part-…

--

net: mvneta: fix the removal of the mqprio offload

This patch moves the qopt.hw check in mvneta_setup_mqprio() so that it
only applies when num_tc is non-zero. The all-zero configuration passed
by mqprio_disable_offload() can then reach the teardown path. That path
clears the RX priority map, disables the per-queue rate limits and resets
the traffic classes.

> Tested on a Turris Omnia (Armada 385) with 10 Mbit/s limits on a
> 100 Mbit/s link. After the qdisc was deleted, only TX queues 0-3 were
> used and the link carried 19 Mbit/s. With this patch and the previous
> one, the queues above 3 are used again and the link reaches 94 Mbit/s.

[Severity: Low]
This isn't a bug, but the bug fixes section of
Documentation/process/maintainer-netdev.rst says:

  All bug fixes must state how the issue was discovered and validated,
  and how the fix was tested.

Could the commit message say how the problem was found? For example,
was it seen in real use, or found by code inspection?

The same section asks for reproduction steps to be included when they
are short (roughly 10 lines or fewer). Could the tc commands be added?
That would be adding an mqprio qdisc with hw 1, shaper bw_rlimit and
max_rate, followed by tc qdisc del.

It also asks whether the fix was tested against the reproducer and
against kernel selftests. The hardware test is described, but selftests
are not mentioned.

> 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;
> +
>  	if (num_tc > rxq_number)
>  		return -EINVAL;
>  

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. While
looking at the teardown path, though: what happens to the hardware state
when mvneta_setup_mqprio() fails part-way through?

Later in the same function:

	ret = mvneta_enable_per_queue_rate_limit(pp);
	if (ret)
		return ret;

	for (tc = 0; tc < mqprio->qopt.num_tc; tc++) {
		for (txq = mqprio->qopt.offset[tc];
		     ...
			ret = mvneta_setup_queue_rates(pp, txq,
						       mqprio->min_rate[tc],
						       mqprio->max_rate[tc]);
			if (ret)
				return ret;
		}
	}

By the time these returns run, the function has already called:

  netdev_set_num_tc()
  netdev_set_tc_queue()
  mvneta_map_vlan_prio_to_rxq()
  mvneta_enable_per_queue_rate_limit()

It may also have written MVNETA_TXQ_BUCKET_REFILL_REG() for earlier
queues. The rxq >= rxq_number and txq >= txq_number checks also return
-EINVAL without undoing any of this.

On the qdisc side, mqprio_enable_offload() sets hw_offload only on
success:

	err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQPRIO,
					    &mqprio);
	if (err)
		return err;

	priv->hw_offload = mqprio.qopt.hw;

So mqprio_destroy() takes the netdev_set_num_tc(dev, 0) branch and never
calls mqprio_disable_offload(). As a result, the num_tc == 0 teardown in
the driver never runs for a failed setup.

Take two TCs where tc1's max_rate is not a multiple of
MVNETA_TXQ_RATE_LIMIT_RESOLUTION, or where tc1 has a non-zero min_rate.
mvneta_setup_queue_rates() then fails for tc1 after tc0's queues have
already been programmed.

Would tc0's TX queues then stay rate-limited, with the VLAN priority to
RXQ map still programmed, and no mqprio qdisc attached?

This looks like it is fixed later in this series by "net: mvneta: unwind
a failed mqprio setup". That patch turns these returns into goto err. The
err label then calls mvneta_clear_rx_prio_map(),
mvneta_disable_per_queue_rate_limit() and netdev_reset_tc().

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

[PATCH net v2 3/5] net: mvneta: serve the TX queues in weighted round robin

[Josef Schlehofer]

Under load, some TX queues get no service at all. With four TCP streams
on a 100 Mbit/s link, up to three streams sent nothing for 20 seconds.

The TX queue arbiter is left at its reset default, which serves all
queues in fixed priority. The stack spreads the flows over the queues,
so a busy queue starves the others.

Give the TX queues the same WRR weight and clear the fixed priority.
Like the Marvell reference driver, use the smallest weight that is
larger than the largest packet.

The mqprio offload never configured the TX scheduling, so its traffic
classes lose the fixed priority of the reset default as well.

Tested on a Turris Omnia (Armada 385). With this patch each busy TX
queue gets an equal share of the link and no stream stalls.

Fixes: ee40a116ebf1 ("net: mvneta: fix improper tx queue usage in mvneta_tx()")
Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 79711a117c39..75d46349df37 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -253,6 +253,7 @@
 #define MVNETA_TXQ_CMD1_REG			 0x3e00
 #define      MVNETA_TXQ_CMD1_BW_LIM_SEL_V1	 BIT(3)
 #define      MVNETA_TXQ_CMD1_BW_LIM_EN		 BIT(0)
+#define MVNETA_TXQ_FIXED_PRIO_REG		 0x3e04
 #define MVNETA_REFILL_NUM_CLK_REG		 0x3e08
 #define      MVNETA_REFILL_MAX_NUM_CLK		 0x0000ffff
 #define MVNETA_TX_MTU                            0x3e0c
@@ -264,6 +265,8 @@
 #define      MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX	 0x0007ffff
 #define MVNETA_TXQ_TOKEN_SIZE_REG(q)             (0x3e40 + ((q) << 2))
 #define      MVNETA_TXQ_TOKEN_SIZE_MAX           0x7fffffff
+#define MVNETA_TXQ_WRR_ARB_REG(q)		 (0x3e80 + ((q) << 2))
+#define      MVNETA_TXQ_WRR_WEIGHT_MASK		 0xff
 
 /* The values of the bucket refill base period and refill period are taken from
  * the reference manual, and adds up to a base resolution of 10Kbps. This allows
@@ -1629,6 +1632,24 @@ static void mvneta_txq_max_tx_size_set(struct mvneta_port *pp, int max_tx_size)
 	}
 }
 
+/* With the default fixed priority, a busy queue starves the others */
+static void mvneta_txq_wrr_set(struct mvneta_port *pp, int max_tx_size)
+{
+	/* In units of 256 bytes, must exceed the largest packet */
+	u32 weight = max_tx_size / 256 + 1;
+	int queue;
+	u32 val;
+
+	for (queue = 0; queue < txq_number; queue++) {
+		val = mvreg_read(pp, MVNETA_TXQ_WRR_ARB_REG(queue));
+		val &= ~MVNETA_TXQ_WRR_WEIGHT_MASK;
+		val |= weight;
+		mvreg_write(pp, MVNETA_TXQ_WRR_ARB_REG(queue), val);
+	}
+
+	mvreg_write(pp, MVNETA_TXQ_FIXED_PRIO_REG, 0);
+}
+
 /* Set unicast address */
 static void mvneta_set_ucast_addr(struct mvneta_port *pp, u8 last_nibble,
 				  int queue)
@@ -3760,6 +3781,7 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 
 	mvneta_max_rx_size_set(pp, pp->pkt_size);
 	mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
+	mvneta_txq_wrr_set(pp, pp->pkt_size);
 
 	/* start the Rx/Tx activity */
 	mvneta_port_enable(pp);
-- 
2.54.0 (Apple Git-157)

[PATCH net v2 4/5] net: mvneta: unwind a failed mqprio setup

[Josef Schlehofer]

A failed mqprio offload setup leaves part of its configuration in the
hardware. For example, when one max_rate is not a multiple of
10 kbit/s, the command fails but the TX queues set up before it stay
rate limited.

mvneta_setup_mqprio() returns on an invalid queue range or rate after
it has already mapped RX queues, switched the rate limit mode and set
the rates of some TX queues. The qdisc then fails to attach and the
driver is not called again, so nothing removes them.

Undo the RX queue mapping, the rate limits and the traffic classes
when the setup fails.

Tested on a Turris Omnia (Armada 385). After mqprio failed on a
max_rate of 10001kbit, TX queues 0-3 stayed limited to 10 Mbit/s and
the VLAN priority map stayed set. With this patch neither is left
behind.

Fixes: e9f7099d0730 ("net: mvneta: Allow having more than one queue per TC")
Fixes: 2551dc9e398c ("net: mvneta: Add TC traffic shaping offload")
Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 28 +++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 75d46349df37..d3a6bf8f383e 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -5299,8 +5299,10 @@ static int mvneta_setup_mqprio(struct net_device *dev,
 		for (rxq = mqprio->qopt.offset[tc];
 		     rxq < mqprio->qopt.count[tc] + mqprio->qopt.offset[tc];
 		     rxq++) {
-			if (rxq >= rxq_number)
-				return -EINVAL;
+			if (rxq >= rxq_number) {
+				ret = -EINVAL;
+				goto err;
+			}
 
 			mvneta_map_vlan_prio_to_rxq(pp, tc, rxq);
 		}
@@ -5311,29 +5313,39 @@ static int mvneta_setup_mqprio(struct net_device *dev,
 		return 0;
 	}
 
-	if (mqprio->qopt.num_tc > txq_number)
-		return -EINVAL;
+	if (mqprio->qopt.num_tc > txq_number) {
+		ret = -EINVAL;
+		goto err;
+	}
 
 	ret = mvneta_enable_per_queue_rate_limit(pp);
 	if (ret)
-		return ret;
+		goto err;
 
 	for (tc = 0; tc < mqprio->qopt.num_tc; tc++) {
 		for (txq = mqprio->qopt.offset[tc];
 		     txq < mqprio->qopt.count[tc] + mqprio->qopt.offset[tc];
 		     txq++) {
-			if (txq >= txq_number)
-				return -EINVAL;
+			if (txq >= txq_number) {
+				ret = -EINVAL;
+				goto err;
+			}
 
 			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;
 }
 
 static int mvneta_setup_tc(struct net_device *dev, enum tc_setup_type type,
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v2 4/5] net: mvneta: unwind a failed mqprio setup

[Maxime Chevallier]

Hi,

On 11/3/26 13:50, Josef Schlehofer wrote:
> A failed mqprio offload setup leaves part of its configuration in the
> hardware. For example, when one max_rate is not a multiple of
> 10 kbit/s, the command fails but the TX queues set up before it stay
> rate limited.
> 
> mvneta_setup_mqprio() returns on an invalid queue range or rate after
> it has already mapped RX queues, switched the rate limit mode and set
> the rates of some TX queues. The qdisc then fails to attach and the
> driver is not called again, so nothing removes them.
> 
> Undo the RX queue mapping, the rate limits and the traffic classes
> when the setup fails.
> 
> Tested on a Turris Omnia (Armada 385). After mqprio failed on a
> max_rate of 10001kbit, TX queues 0-3 stayed limited to 10 Mbit/s and
> the VLAN priority map stayed set. With this patch neither is left
> behind.
> 
> Fixes: e9f7099d0730 ("net: mvneta: Allow having more than one queue per TC")
> Fixes: 2551dc9e398c ("net: mvneta: Add TC traffic shaping offload")
> Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
>  drivers/net/ethernet/marvell/mvneta.c | 28 +++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 75d46349df37..d3a6bf8f383e 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -5299,8 +5299,10 @@ static int mvneta_setup_mqprio(struct net_device *dev,
>  		for (rxq = mqprio->qopt.offset[tc];
>  		     rxq < mqprio->qopt.count[tc] + mqprio->qopt.offset[tc];
>  		     rxq++) {
> -			if (rxq >= rxq_number)
> -				return -EINVAL;
> +			if (rxq >= rxq_number) {
> +				ret = -EINVAL;
> +				goto err;
> +			}
>  
>  			mvneta_map_vlan_prio_to_rxq(pp, tc, rxq);
>  		}
> @@ -5311,29 +5313,39 @@ static int mvneta_setup_mqprio(struct net_device *dev,
>  		return 0;
>  	}
>  
> -	if (mqprio->qopt.num_tc > txq_number)
> -		return -EINVAL;
> +	if (mqprio->qopt.num_tc > txq_number) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
>  
>  	ret = mvneta_enable_per_queue_rate_limit(pp);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
>  	for (tc = 0; tc < mqprio->qopt.num_tc; tc++) {
>  		for (txq = mqprio->qopt.offset[tc];
>  		     txq < mqprio->qopt.count[tc] + mqprio->qopt.offset[tc];
>  		     txq++) {
> -			if (txq >= txq_number)
> -				return -EINVAL;
> +			if (txq >= txq_number) {
> +				ret = -EINVAL;
> +				goto err;
> +			}
>  
>  			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;
>  }
>  
>  static int mvneta_setup_tc(struct net_device *dev, enum tc_setup_type type,

[PATCH net v2 5/5] net: mvneta: keep the TX rate limits across open and MTU change

[Josef Schlehofer]

After the interface is brought up again or its MTU is changed, every
per-queue rate limit set by mqprio runs at 10.23 Mbit/s, whatever rate
was configured. tc still shows the configured rates.

mvneta_txq_hw_init() writes the token configuration of the legacy rate
limiter for each TX queue. This also sets the refill value of the
queue's token bucket to 1023, which is 10.23 Mbit/s at the refill
period that mqprio uses.

Remember the refill of each rate limited queue and program the rate
limits again when the port is started. Resume also starts the port
with mvneta_start_dev().

Tested on a Turris Omnia (Armada 385) with 1 Mbit/s limits on a
100 Mbit/s link. After the link was set down and up, or the MTU was
changed, each busy TX queue carried about 10 Mbit/s. With this patch it
stays at about 1 Mbit/s.

Fixes: 2551dc9e398c ("net: mvneta: Add TC traffic shaping offload")
Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
---
 drivers/net/ethernet/marvell/mvneta.c | 29 ++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index d3a6bf8f383e..92a2ea930002 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -686,6 +686,9 @@ struct mvneta_tx_queue {
 
 	u32 done_pkts_coal;
 
+	/* Bucket refill of the mqprio rate limit, 0 when not limited */
+	u32 bucket_refill;
+
 	/* Virtual address of the TX DMA descriptors array */
 	struct mvneta_tx_desc *descs;
 
@@ -3773,6 +3776,8 @@ static int mvneta_config_interface(struct mvneta_port *pp,
 	return ret;
 }
 
+static void mvneta_restore_per_queue_rate_limit(struct mvneta_port *pp);
+
 static void mvneta_start_dev(struct mvneta_port *pp)
 {
 	int cpu;
@@ -3782,6 +3787,7 @@ static void mvneta_start_dev(struct mvneta_port *pp)
 	mvneta_max_rx_size_set(pp, pp->pkt_size);
 	mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
 	mvneta_txq_wrr_set(pp, pp->pkt_size);
+	mvneta_restore_per_queue_rate_limit(pp);
 
 	/* start the Rx/Tx activity */
 	mvneta_port_enable(pp);
@@ -5234,8 +5240,10 @@ static void mvneta_disable_per_queue_rate_limit(struct mvneta_port *pp)
 	/* The token buckets still limit the queues, refill them at full rate */
 	val = (1 << MVNETA_TXQ_BUCKET_REFILL_PERIOD_SHIFT) |
 	      MVNETA_TXQ_BUCKET_REFILL_VALUE_MAX;
-	for (queue = 0; queue < txq_number; queue++)
+	for (queue = 0; queue < txq_number; queue++) {
+		pp->txqs[queue].bucket_refill = 0;
 		mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);
+	}
 }
 
 static int mvneta_setup_queue_rates(struct mvneta_port *pp, int queue,
@@ -5261,11 +5269,30 @@ static int mvneta_setup_queue_rates(struct mvneta_port *pp, int queue,
 	val |= (MVNETA_TXQ_BUCKET_REFILL_PERIOD <<
 		MVNETA_TXQ_BUCKET_REFILL_PERIOD_SHIFT);
 
+	pp->txqs[queue].bucket_refill = val;
 	mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue), val);
 
 	return 0;
 }
 
+/* The TX queue setup overwrites the rate limits, program them again */
+static void mvneta_restore_per_queue_rate_limit(struct mvneta_port *pp)
+{
+	int queue;
+
+	for (queue = 0; queue < txq_number; queue++)
+		if (pp->txqs[queue].bucket_refill)
+			break;
+
+	if (queue == txq_number || mvneta_enable_per_queue_rate_limit(pp))
+		return;
+
+	for (; queue < txq_number; queue++)
+		if (pp->txqs[queue].bucket_refill)
+			mvreg_write(pp, MVNETA_TXQ_BUCKET_REFILL_REG(queue),
+				    pp->txqs[queue].bucket_refill);
+}
+
 static int mvneta_setup_mqprio(struct net_device *dev,
 			       struct tc_mqprio_qopt_offload *mqprio)
 {
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v2 5/5] net: mvneta: keep the TX rate limits across open and MTU change

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message of "net: mvneta: keep the TX rate limits
  across open and MTU change" does not say how the bug was discovered,…

--

net: mvneta: keep the TX rate limits across open and MTU change

This commit stores the mqprio bucket refill value for each rate limited
TX queue. It then programs the rate limits again from
mvneta_start_dev(). Without this, mvneta_txq_hw_init() leaves every
limited queue running at 10.23 Mbit/s after the interface is brought up
again or its MTU is changed.

> Tested on a Turris Omnia (Armada 385) with 1 Mbit/s limits on a
> 100 Mbit/s link. After the link was set down and up, or the MTU was
> changed, each busy TX queue carried about 10 Mbit/s. With this patch it
> stays at about 1 Mbit/s.

[Severity: Low]
This isn't a bug, but the commit message may be missing two statements
that the netdev process now asks for in fixes.

The "Bug fixes" section of Documentation/process/maintainer-netdev.rst
says:

    All bug fixes must state how the issue was discovered and validated,
    and how the fix was tested.

and:

    The testing statement should explain whether the fix was tested
    against the reproducer and against kernel selftests.

The paragraph above names the hardware and gives before and after
numbers from the reproducer, so that part is covered.

It doesn't say how the problem was found, though. The document gives
examples such as "observed on a machine in real use" and "discovered by
manual code inspection". There is also no Reported-by or similar trailer
that would imply it.

It also doesn't say whether kernel selftests were run.

Could the commit message get a sentence on how the issue was found, and
another on whether selftests were run? This requirement was tightened
by the baseline commit f56daaadfecae1 ("docs: netdev: try to force
agents to disclose info for fixes").

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

Re: [PATCH net v2 0/5] net: mvneta: fix the TX queue arbitration and the mqprio offload

[Maxime Chevallier]

Hi Josef,

On 11/3/26 13:50, Josef Schlehofer wrote:
> The NETA TX arbiter is left at its reset default, which serves the TX
> queues in fixed priority. Under load a busy queue starves the others.
> OpenWrt has worked around this since 2017 by sending all TX traffic
> of its 32-bit boards through queue 0 [1].
> 
> Patch 3 serves the queues in weighted round robin. While working on it,
> four older bugs in the mqprio offload showed up. Turning the shaper off
> leaves the per-queue rate limits in place (patch 1). Deleting the qdisc
> never reaches the driver (patch 2). A failed setup is not undone
> (patch 4). Opening the interface or changing its MTU sets every rate
> limit to 10.23 Mbit/s (patch 5).
> 
> The series was tested on a Turris Omnia (Armada 385). The other NETA
> variants were not tested, but the driver already programs registers of
> the same block (MVNETA_TX_MTU and the token sizes) on all of them.
> Suspend and resume were not tested.
> 
> With patch 2, replacing an offloaded mqprio qdisc with another mqprio
> under a new handle loses the new configuration. The new qdisc is set up
> before the old one is torn down, and the driver now acts on that
> teardown. Software mqprio loses the configuration in the same way.

Wouldn't this break mqprio offload ? It's no longer priorising then ?

That behaviour of the high-prio queue potentially starving the other
is likely what mqprio users actually need, so maybe we need to setup
prio-based queue selection when adding an mqprio qdisc, and switch back
to WRR when its removed ?

Maxime

Re: [PATCH net v2 0/5] net: mvneta: fix the TX queue arbitration and the mqprio offload

[Josef Schlehofer]

Hi Maxime,

On 11/3/26 19:56, Maxime Chevallier wrote:
> Wouldn't this break mqprio offload ? It's no longer priorising then ?
>
> That behaviour of the high-prio queue potentially starving the other
> is likely what mqprio users actually need, so maybe we need to setup
> prio-based queue selection when adding an mqprio qdisc, and switch back
> to WRR when its removed ?

You're right, it does. In v3 I switch the queues back to fixed priority
while an mqprio offload is set up, and to WRR when it is removed, as
you suggest.

I tested it on the Omnia. With the offload, the higher traffic class
still takes the whole link (94 vs 0.2 Mbit/s). Without it, the busy
queues share the link equally.

For patch 2 I added one thing to the commit message: if taprio replaces
the offload under a new handle, it ends up without traffic classes and
sends nothing, like after a software mqprio. The code is the same, so I
kept your Reviewed-by. Hope that's OK.

I'd like to send v3 later today. Thanks for the reviews!

Josef

pw-bot: cr