Re: [PATCH net-next v3 1/2] net: stmmac: add tc mqprio hw queue priority offload

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 (3) · 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] Stale shadow priorities in xmit_qdisc.prio[] get written back
  to hardware.
- [Medium] net: stmmac: state corruption when switching mqprio mode from
  CHANNEL to DCB

Pre-existing issues:
- [Medium] Channel-mode shaper and rate requests are accepted but never
  applied.
- [Medium] DCB mappings with queue gaps quietly disable TC
  classification.
- [Medium] The mqprio path accesses MAC/MTL registers without a runtime-
  PM reference.

--

net: stmmac: add tc mqprio hw queue priority offload

This patch adds mqprio channel mode offload to stmmac. In that mode the
MTL TX scheduler is switched to strict priority and the per-queue
PSTQx/PSTC priority bitmasks are programmed from prio_tc_map. The patch
also adds a shadow copy of the TX algorithm and the per-queue priorities
in priv->xmit_qdisc, which is programmed into the hardware on open.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 42a00446e9b4..07cf4582ed76 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c

[ ... ]

> @@ -1266,12 +1269,109 @@ static int stmmac_reset_tc_mqprio(struct net_device *ndev,
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
>  
> +	if (priv->xmit_qdisc.prio_offload) {
> +		int i;
> +
> +		for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
> +			u32 prio;
> +
> +			if (priv->plat->tx_queues_cfg[i].use_prio)
> +				prio = priv->plat->tx_queues_cfg[i].prio;
> +			else
> +				prio = 0;
> +
> +			stmmac_tx_queue_prio(priv, priv->hw, prio, i);
> +			priv->xmit_qdisc.prio[i] = prio;
> +		}

[Severity: Medium]
Can stale entries in priv->xmit_qdisc.prio[] get written back to the
hardware?

This loop, and the one in tc_mqprio_config_queue_prio(), only update
prio[i] for i below the tx_queues_to_use value at that moment. With this
patch, stmmac_mac_config_tx_queues_prio() programs PSTQx/PSTC from the
shadow array on every open, where it used to read tx_queues_cfg[]:

stmmac_main.c:stmmac_mac_config_tx_queues_prio() {
	for (i = 0; i < priv->plat->tx_queues_to_use; i++)
		stmmac_tx_queue_prio(priv, priv->hw,
				     priv->xmit_qdisc.prio[i], i);
}

With 8 TX queues from DT, each with snps,priority, this sequence looks
possible:

1) tc qdisc add ... mqprio num_tc 4 queues 1@0 1@1 1@2 1@3 hw 1 mode channel
   tc_mqprio_config_queue_prio() sets prio[4..7] = 0
2) ethtool -L <dev> tx 4
   tx_queues_to_use becomes 4
3) tc qdisc del ...
   stmmac_reset_tc_mqprio() restores only prio[0..3]
4) ethtool -L <dev> tx 8
   stmmac_open()->stmmac_hw_setup()->stmmac_mtl_configuration()->
   stmmac_mac_config_tx_queues_prio() writes 0 for queues 4..7

After that, queues 4..7 keep a zero priority map in place of their DT
values until the driver is re-probed.

The follow-up commit "net: stmmac: align real_num_tx_queues to mqprio
configuration" makes stmmac_reinit_queues() reject tx_cnt below
xmit_qdisc.num_tx_queues while mqprio is enabled. That covers shrinking
below the TC queues. Shrinking to exactly num_tx_queues is still allowed,
though, so step 2 still works at the end of the series.

Should the restore loop here cover all MTL_MAX_TX_QUEUES entries?

> +
> +		stmmac_prog_mtl_tx_algorithms(priv, priv->hw,
> +					      priv->plat->tx_sched_algorithm);
> +		priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;
> +		priv->xmit_qdisc.prio_offload = false;
> +	}
> +
>  	netdev_reset_tc(ndev);
>  	netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use);
>  
>  	return stmmac_fpe_map_preemption_class(priv, ndev, extack, 0);
>  }
>  
> +static void tc_mqprio_config_queue_prio(struct stmmac_priv *priv,
> +					struct tc_mqprio_qopt *qopt)
> +{

[ ... ]

> +		stmmac_tx_queue_prio(priv, priv->hw, prio, i);
> +		priv->xmit_qdisc.prio[i] = prio;
> +	}
> +
> +	stmmac_prog_mtl_tx_algorithms(priv, priv->hw, MTL_TX_ALGORITHM_SP);
> +	priv->xmit_qdisc.algo = MTL_TX_ALGORITHM_SP;
> +	priv->xmit_qdisc.prio_offload = true;
> +}

[Severity: Medium]
This is a pre-existing issue, but is it safe to access the MTL registers
here without a runtime PM reference?

stmmac_setup_tc() passes TC_SETUP_QDISC_MQPRIO to
stmmac_tc_setup_mqprio() without calling pm_runtime_resume_and_get().
While the interface is down, stmmac_runtime_suspend() can gate
stmmac_clk and pclk through stmmac_bus_clks_config(priv, false).

A tc qdisc add or del on a down interface would then reach
stmmac_tx_queue_prio() and stmmac_prog_mtl_tx_algorithms() here, and in
the prio_offload block of stmmac_reset_tc_mqprio(), with those clocks
off.

This path already did unguarded MMIO before this patch, through
dwmac5_fpe_map_preemption_class():

	val = readl(priv->ioaddr + GMAC5_MTL_FPE_CTRL_STS);

Other register-touching ndos in this driver, such as set_mac_address and
the VLAN add/kill ops, call pm_runtime_resume_and_get() first.

The xmit_qdisc shadow is reapplied on open. Could the register writes be
skipped when !netif_running(), or could a PM reference be taken around
them?

> +
> +static int tc_mqprio_validate_chan_mode(struct stmmac_priv *priv,
> +					struct tc_mqprio_qopt_offload *mqprio)
> +{
> +	struct plat_stmmacenet_data *pdata = priv->plat;
> +	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
> +	int i;
> +
> +	if (!priv->dma_cap.dcben) {
> +		NL_SET_ERR_MSG_MOD(mqprio->extack,
> +				   "hw DCB is required to offload mqprio");
> +		return -EOPNOTSUPP;
> +	}

[ ... ]

> +	for (i = 0; i < qopt->num_tc; i++) {
> +		/* The offload switches the MTL scheduler to strict
> +		 * priority, which only supports a 1:1 TC to TX queue
> +		 * mapping.
> +		 */
> +		if (qopt->count[i] > 1 || qopt->offset[i] != i) {
> +			NL_SET_ERR_MSG_MOD(mqprio->extack,
> +					   "SP requires 1:1 TXQ map");
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
This is a pre-existing issue, but should channel mode reject shaper and
rate requests?

In channel mode, mqprio_enable_offload() in net/sched/sch_mqprio.c
copies the flags, shaper, min_rate[] and max_rate[] into struct
tc_mqprio_qopt_offload. If the driver returns 0, it sets hw_offload and
later dumps TCA_MQPRIO_SHAPER.

tc_setup_dwmac510_mqprio() only checks TC_MQPRIO_F_MODE, and this
validator does not check TC_MQPRIO_F_SHAPER, TC_MQPRIO_F_MIN_RATE or
TC_MQPRIO_F_MAX_RATE.

So a request like "mqprio ... hw 1 mode channel shaper bw_rlimit
max_rate ..." with a 1:1 mapping succeeds. Only SP and PSTQ get
programmed, and the qdisc reports rate limiting the hardware isn't
doing.

The baseline handler ignored mode and flags entirely. Would this new
validator be a good place to return -EOPNOTSUPP for those flags?

[ ... ]

> @@ -1306,6 +1405,22 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
>  		num_tx_queues += qopt->count[i];
>  	}

[Severity: Medium]
This is a pre-existing issue, but is num_tx_queues too small when the
queue ranges have gaps?

mqprio_validate_queue_counts() does not require the ranges to be
contiguous from queue 0. Here num_tx_queues is the sum of the counts,
not the highest offset + count.

On a 4 queue device, a DCB request with a single TC at 1@2 passes both
that check and the new offset + count check in this function. It then
calls netif_set_real_num_tx_queues(ndev, 1), and netif_setup_tc() does:

	/* If TC0 is invalidated disable TC mapping */
	if (res.offset + res.count > txq) {
		netdev_warn(dev, "Number of in use tx queues changed invalidating tc mappings. Priority traffic classification disabled!\n");
		WRITE_ONCE(dev->num_tc, 0);
		return;
	}

The driver still returns 0, so mqprio reports a working offload while
the device runs on one queue with no TC mapping. With 1@0 1@2, the
priorities for TC1 are quietly remapped to TC0 instead.

The baseline code computed num_tx_queues the same way. Should this use
the highest offset + count, or reject non-contiguous ranges?

>  
> +	mode = mqprio->flags & TC_MQPRIO_F_MODE ? mqprio->mode
> +						: TC_MQPRIO_MODE_DCB;
> +	if (mode == TC_MQPRIO_MODE_CHANNEL) {
> +		err = tc_mqprio_validate_chan_mode(priv, mqprio);
> +		if (err)
> +			return err;
> +	}

[Severity: Medium]
Does a DCB mode request clean up after a previous channel mode
offload?

The xmit_qdisc state is only restored in stmmac_reset_tc_mqprio(),
which runs only when num_tc is 0. When tc_setup_dwmac510_mqprio() is
called with num_tc > 0 and mode resolves to TC_MQPRIO_MODE_DCB, nothing
touches priv->xmit_qdisc. A prior channel mode configuration therefore
leaves three things behind: xmit_qdisc.algo stays at
MTL_TX_ALGORITHM_SP, xmit_qdisc.prio_offload stays set, and the channel
mode PSTQx/PSTC maps stay programmed in the hardware.

One way to get here is replacing a channel mode mqprio with a DCB one
under a new handle. qdisc_graft() runs mqprio_init() for the new qdisc,
and so the driver offload, before mqprio_destroy() of the old qdisc
calls back with num_tc 0.

With this patch, dwmac5_fpe_map_preemption_class() tests
xmit_qdisc.algo instead of plat->tx_sched_algorithm. So on that path, a
valid DCB request with a TC spanning more than one queue is rejected
with ALG_ERR_MSG, even when DT selected a non-SP algorithm.

Any DCB request that arrives without an intervening reset has the same
problem. The MTL scheduler stays in SP with the old priority maps, and
tc_setup_cbs() keeps refusing CBS because prio_offload is still true.
That contradicts the commit message, which says DCB mode leaves the
scheduler and per-queue priorities to the device-tree configuration.

Could the prio_offload restore block in stmmac_reset_tc_mqprio() be
moved into a helper and also called on the DCB path, before
stmmac_fpe_map_preemption_class() runs? If so, the error unwind in
tc_setup_dwmac510_mqprio() would need to put the channel mode state
back when the FPE mapping fails. How would you prefer to handle that?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-stmmac-mqprio-prio-offload-v3-0-abbe181f5024%40oss.qualcomm.com