Re: [PATCH net v3 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 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] In drivers/net/ethernet/marvell/mvneta.c,
  mvneta_disable_per_queue_rate_limit() now writes every…

--

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

When the per-queue TX rate limit is disabled, this commit sets the token
bucket refill registers of all TX queues to full rate. Without it, the
limits set by an earlier mqprio shaper stayed in effect after the shaper
was 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: Low]
Does "full rate" here depend on MVNETA_REFILL_NUM_CLK_REG having been
programmed first?

A period of 1 means one base refill period. The length of that period
comes from MVNETA_REFILL_NUM_CLK_REG. As far as I can see, the only write
to that register in the driver is in mvneta_enable_per_queue_rate_limit():

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

mvneta_setup_mqprio() can reach the disable path without the enable path
ever having run:

	if (!num_tc) {
		mvneta_disable_per_queue_rate_limit(pp);
		...

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

One example is the first offloaded mqprio after boot with no bw_rlimit
shaper, such as one that only maps VLAN priorities to RX queues. Later
in the series, deleting such an offload, or a setup that fails before
mvneta_enable_per_queue_rate_limit() runs, also takes this path.

In those cases the per-queue refill registers are overwritten while
MVNETA_REFILL_NUM_CLK_REG still holds its hardware reset value. Before
this patch they kept their defaults on a port where the shaper was never
enabled.

If the reset period is long, could this cap the TX queues below line
rate? For example, with NUM_CLK at 0xffff cycles, MAX tokens per period
works out to roughly 2 Gbit/s, which is below a 2.5G port's rate.

This depends on the reset value of MVNETA_REFILL_NUM_CLK_REG, which
can't be checked from the code. Period 1 with the maximum value is the
fastest setting the register can encode, so this may never be visible in
practice.

Would it make sense to also program MVNETA_REFILL_NUM_CLK_REG on the
disable path, so the "full rate" setting doesn't depend on the enable
path having run first?

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

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