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

[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] 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