[PATCH net-next v2 0/2] net: stmmac: Introduce hw queue priority offload

[PATCH net-next v2 0/2] net: stmmac: Introduce hw queue priority offload

[Lorenzo Bianconi]

Implement the offload of the tc mqprio hw queue priority in the stmmac
driver. This feature requires the mqprio to be configured in channel
mode.
Do not overwrite real_num_tx_queues configured by the qdisc in
stmmac_hw_setup().

---
Changes in v2:
- Forbid cbs queue offload if mqprio is already enabled.
- SP requires 1:1 TC to TXQ map.
- Fix the real_num_tx_queue overwrite during device open.
- Require the qdisc to be configured in channel mode to enable hw queue
  priority offload in order to not introduce any regression in the
  previous driver behaviour.
- Link to v1: https://lore.kernel.org/r/20260918-stmmac-mqprio-prio-offload-v1-1-5328157fcb58@oss.qualcomm.com

---
Lorenzo Bianconi (2):
      net: stmmac: add tc mqprio hw queue priority offload
      net: stmmac: align real_num_tx_queues to mqprio configuration

 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   7 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c  |   2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  28 +++--
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 132 ++++++++++++++++++++--
 4 files changed, 146 insertions(+), 23 deletions(-)
---
base-commit: 6516ac5e6b164bf78fb34d36b4d84d495a07c844
change-id: 20260918-stmmac-mqprio-prio-offload-3d82d87f7bc8

Best regards,
-- 
Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

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

[Lorenzo Bianconi]

Implement the offload of the tc mqprio hw queue priority in the stmmac
driver. When the mqprio qdisc is configured in channel mode, the MTL TX
scheduler is switched to strict priority, and the PSTQX/PSTC priority
bitmask of each TX queue is programmed from the set of frame priorities
mapped to the owning traffic class (qopt->prio_tc_map).

The channel mode offload requires the DCB hw feature and a 1:1 TC to TX
queue mapping, with a single queue per TC. Configurations with AVB queues
are rejected, since forcing strict priority conflicts with the CBS
algorithm.

In the default DCB mode, only the netdev TC map and the FPE preemption
class mapping are offloaded, leaving the MTL scheduler and the per-queue
priorities to the devicetree configuration.

Reviewed-by: Davide Caratti <dcaratti@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   6 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c  |   2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  25 +++--
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 129 ++++++++++++++++++++--
 4 files changed, 140 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 4fc96b317d79..69b400e9b138 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -299,6 +299,12 @@ struct stmmac_priv {
 	/* Protect est parameters */
 	struct mutex est_lock;
 	struct stmmac_est *est;
+
+	struct {
+		u32 prio[MTL_MAX_TX_QUEUES];
+		u8 algo;
+	} xmit_qdisc;
+
 	struct dma_features dma_cap;
 	struct stmmac_counters mmc;
 	int hw_cap_support;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
index c889204a7aa5..b6b5ef7c8fc4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
@@ -230,7 +230,7 @@ int dwmac5_fpe_map_preemption_class(struct net_device *ndev,
 		if (count == 1)
 			continue;
 
-		if (priv->plat->tx_sched_algorithm == MTL_TX_ALGORITHM_SP) {
+		if (priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP) {
 			NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
 			return -EINVAL;
 		}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index bf9e7e4cb1c3..0c9083f8846e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3533,17 +3533,11 @@ static void stmmac_mac_config_rx_queues_prio(struct stmmac_priv *priv)
  */
 static void stmmac_mac_config_tx_queues_prio(struct stmmac_priv *priv)
 {
-	u8 tx_queues_count = priv->plat->tx_queues_to_use;
-	u8 queue;
-	u32 prio;
-
-	for (queue = 0; queue < tx_queues_count; queue++) {
-		if (!priv->plat->tx_queues_cfg[queue].use_prio)
-			continue;
+	int i;
 
-		prio = priv->plat->tx_queues_cfg[queue].prio;
-		stmmac_tx_queue_prio(priv, priv->hw, prio, queue);
-	}
+	for (i = 0; i < priv->plat->tx_queues_to_use; i++)
+		stmmac_tx_queue_prio(priv, priv->hw,
+				     priv->xmit_qdisc.prio[i], i);
 }
 
 /**
@@ -3604,7 +3598,7 @@ static void stmmac_mtl_configuration(struct stmmac_priv *priv)
 	/* Configure MTL TX algorithms */
 	if (tx_queues_count > 1)
 		stmmac_prog_mtl_tx_algorithms(priv, priv->hw,
-				priv->plat->tx_sched_algorithm);
+					      priv->xmit_qdisc.algo);
 
 	/* Configure CBS in AVB TX queues */
 	if (tx_queues_count > 1)
@@ -7921,6 +7915,15 @@ static int __stmmac_dvr_probe(struct device *device,
 	priv->wol_irq = res->wol_irq;
 	priv->sfty_irq = res->sfty_irq;
 
+	/* Default xmit qdisc configuration */
+	for (i = 0; i < MTL_MAX_TX_QUEUES; i++) {
+		if (!priv->plat->tx_queues_cfg[i].use_prio)
+			continue;
+
+		priv->xmit_qdisc.prio[i] = priv->plat->tx_queues_cfg[i].prio;
+	}
+	priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;
+
 	if (priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN) {
 		ret = stmmac_msi_init(priv, res);
 		if (ret)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 42a00446e9b4..54242461a860 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -346,6 +346,9 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
 	if (!priv->dma_cap.av)
 		return -EOPNOTSUPP;
 
+	if (qopt->enable && priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP)
+		return -EOPNOTSUPP;
+
 	port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope;
 
 	if (qopt->enable) {
@@ -1265,6 +1268,23 @@ static int stmmac_reset_tc_mqprio(struct net_device *ndev,
 				  struct netlink_ext_ack *extack)
 {
 	struct stmmac_priv *priv = netdev_priv(ndev);
+	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;
+	}
+
+	stmmac_prog_mtl_tx_algorithms(priv, priv->hw,
+				      priv->plat->tx_sched_algorithm);
+	priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;
 
 	netdev_reset_tc(ndev);
 	netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use);
@@ -1272,6 +1292,73 @@ static int stmmac_reset_tc_mqprio(struct net_device *ndev,
 	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)
+{
+	int i;
+
+	for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
+		u32 prio = 0;
+		int j;
+
+		for (j = 0; j < qopt->num_tc; j++) {
+			int p;
+
+			if (qopt->offset[j] != i)
+				continue;
+
+			/* The PSTQX/PSTC priority map is 8 bits wide, so only
+			 * priorities 0-7 can be represented in hardware.
+			 * Priorities 8-15 are handled in software by the
+			 * kernel through the netdev prio_tc_map.
+			 */
+			for (p = 0; p < 8; p++) {
+				if (qopt->prio_tc_map[p] == j)
+					prio |= BIT(p);
+			}
+			break;
+		}
+
+		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);
+}
+
+static int tc_mqprio_validate_chan_mode(struct stmmac_priv *priv,
+					struct tc_mqprio_qopt_offload *mqprio)
+{
+	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
+	int i;
+
+	if (!priv->dma_cap.dcben)
+		return -EOPNOTSUPP;
+
+	/* Forcing strict priority conflicts with the CBS algorithm
+	 * of AVB queues, so reject the offload when any queue is
+	 * configured as AVB.
+	 */
+	for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
+		if (priv->plat->tx_queues_cfg[i].mode_to_use == MTL_QUEUE_AVB)
+			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;
+}
+
 static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 				    struct tc_mqprio_qopt_offload *mqprio)
 {
@@ -1282,23 +1369,22 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
 	struct net_device *ndev = priv->dev;
 	u8 ndev_prio_tc_map[TC_BITMASK + 1];
-	int i, err, ndev_ntc;
+	int i, err, ndev_ntc, mode;
 
 	if (!qopt->num_tc)
 		return stmmac_reset_tc_mqprio(ndev, extack);
 
-	if (qopt->num_tc > ARRAY_SIZE(tc_to_txq))
+	if (qopt->num_tc > priv->plat->tx_queues_to_use)
 		return -EINVAL;
 
-	/* save current tc values for reset */
-	ndev_ntc = netdev_get_num_tc(ndev);
-	for (i = 0; i < ARRAY_SIZE(ndev->tc_to_txq); i++)
-		ndev_tc_to_txq[i].combined =
-			READ_ONCE(ndev->tc_to_txq[i].combined);
-	for (i = 0; i < ARRAY_SIZE(ndev_prio_tc_map); i++)
-		ndev_prio_tc_map[i] = READ_ONCE(ndev->prio_tc_map[i]);
-
 	for (i = 0; i < qopt->num_tc; i++) {
+		if (qopt->offset[i] + qopt->count[i] >
+		    priv->plat->tx_queues_to_use) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Queues exceed hw TX queues");
+			return -EINVAL;
+		}
+
 		tc_to_txq[i] = (struct netdev_tc_txq) {
 			.count = qopt->count[i],
 			.offset = qopt->offset[i],
@@ -1306,6 +1392,24 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 		num_tx_queues += qopt->count[i];
 	}
 
+	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;
+
+		priv->xmit_qdisc.algo = MTL_TX_ALGORITHM_SP;
+	}
+
+	/* save current tc values for reset */
+	ndev_ntc = netdev_get_num_tc(ndev);
+	for (i = 0; i < ARRAY_SIZE(ndev->tc_to_txq); i++)
+		ndev_tc_to_txq[i].combined =
+			READ_ONCE(ndev->tc_to_txq[i].combined);
+	for (i = 0; i < ARRAY_SIZE(ndev_prio_tc_map); i++)
+		ndev_prio_tc_map[i] = READ_ONCE(ndev->prio_tc_map[i]);
+
 	err = stmmac_set_ndev_tcs(ndev, qopt->num_tc, tc_to_txq);
 	if (err)
 		goto error_reset_tc;
@@ -1320,6 +1424,9 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 	if (err)
 		goto error_reset_num_tx_queues;
 
+	if (mode == TC_MQPRIO_MODE_CHANNEL)
+		tc_mqprio_config_queue_prio(priv, qopt);
+
 	return 0;
 
 error_reset_num_tx_queues:
@@ -1327,6 +1434,8 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 		netdev_warn(ndev, "Failed to restore %u TX queues\n",
 			    ndev_num_tx_queues);
 error_reset_tc:
+	if (mode == TC_MQPRIO_MODE_CHANNEL)
+		priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;
 	stmmac_set_ndev_tcs(ndev, ndev_ntc, ndev_tc_to_txq);
 	for (i = 0; i < ARRAY_SIZE(ndev_prio_tc_map); i++)
 		netdev_set_prio_tc_map(ndev, i, ndev_prio_tc_map[i]);

-- 
2.55.0

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 (4) · Low: 1

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).

- [High] This patch adds a check to tc_setup_cbs() (stmmac_tc.c:349-350):
  `if (qopt->enable && priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP)…
- [Medium] stmmac_reset_tc_mqprio() now calls
  stmmac_prog_mtl_tx_algorithms() on every mqprio teardown (num_tc ==…
- [Medium] net: stmmac: hw state desynchronization on mqprio replacement
  error path
- [Low] tc_mqprio_validate_chan_mode() (stmmac_tc.c:1335-1344) returns a
  bare -EOPNOTSUPP in two new cases: when priv->dma_cap.dcben is clear,…

Pre-existing issues:
- [Medium] In channel mode, mqprio_enable_offload() (sch_mqprio.c) passes
  mqprio->flags, shaper, min_rate[] and max_rate[] to the driver.
- [Medium] Replacing an offloaded mqprio root with a new mqprio that uses
  a different handle goes through create_n_graft.
- [Medium] tc_setup_dwmac510_mqprio() computes num_tx_queues as the sum
  of qopt->count[] and not as the highest queue endpoint.
- [Medium] stmmac_setup_tc() dispatches TC_SETUP_QDISC_MQPRIO to
  tc_setup_dwmac510_mqprio() without pm_runtime_resume_and_get().

--

net: stmmac: add tc mqprio hw queue priority offload

This commit offloads the tc mqprio TX queue priority configuration in
stmmac. In channel mode it switches the MTL TX scheduler to strict
priority and programs each queue's priority bitmask from prio_tc_map. A
new xmit_qdisc state tracks the active scheduler and per-queue
priorities, and CBS offload is now rejected while that scheduler is SP.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 42a00446e9b4..54242461a860 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -346,6 +346,9 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
>  	if (!priv->dma_cap.av)
>  		return -EOPNOTSUPP;
>  
> +	if (qopt->enable && priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP)
> +		return -EOPNOTSUPP;
> +

[Severity: Medium]
Can this error path leave xmit_qdisc.algo out of sync with the
hardware?

Say a channel mode mqprio is already offloaded. The MTL TX scheduler
is then programmed as SP, and xmit_qdisc.algo is MTL_TX_ALGORITHM_SP.
Now a second channel mode request arrives. One example is "tc qdisc
replace dev X root handle 2: mqprio ... mode channel hw 1", which
initializes the new qdisc while the old one is still installed.

tc_setup_dwmac510_mqprio() sets algo to SP early. If a later step
fails, the error path sets algo to priv->plat->tx_sched_algorithm.
The failing step could be stmmac_set_ndev_tcs(),
netif_set_real_num_tx_queues() or stmmac_fpe_map_preemption_class().
The value from before the call is never restored.

tc_mqprio_config_queue_prio() is never reached on that path. The
hardware is still in SP with the old qdisc's per-queue priorities,
and the old qdisc stays installed. But algo now holds whatever the
DT selected, for example WRR.

>From then on, the checks that rely on algo don't match the hardware:

  - tc_setup_cbs() would accept enabling CBS while the scheduler is
    still SP.
  - dwmac5_fpe_map_preemption_class() would accept a DCB mode mqprio
    with more than one queue per TC. A DCB request doesn't reprogram
    the scheduler, so the hardware would stay in SP.

The next stmmac_hw_setup() goes wrong in the other direction.
stmmac_mtl_configuration() programs the stale algo, while the old
channel mode qdisc is still installed and expects SP. That setup runs
on ifdown/ifup or resume.

Would it be better to save xmit_qdisc.algo on entry and restore that
value on failure? Another option is to set algo only once
tc_mqprio_config_queue_prio() has actually run. Is there a reason it
needs to be set before stmmac_fpe_map_preemption_class() is called?

[Severity: High]
Can this reject CBS offload on boards that never installed an mqprio
qdisc?

At probe, priv->xmit_qdisc.algo is copied from the platform data:

__stmmac_dvr_probe()
    priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;

stmmac_mtl_setup() sets that to SP when the DT has no snps,tx-sched-*
property:

stmmac_mtl_setup() {
    ...
	else if (of_property_read_bool(tx_node, "snps,tx-sched-dwrr"))
		plat->tx_sched_algorithm = MTL_TX_ALGORITHM_DWRR;
	else
		plat->tx_sched_algorithm = MTL_TX_ALGORITHM_SP;
    ...
}

This means SP set by the DT can't be told apart from SP forced by
channel mode mqprio. For example, imx8mp-evk has
snps,tx-queues-to-use = <5> and no tx-sched property. On that board,
"tc qdisc ... cbs ... offload 1" would now always fail with -EOPNOTSUPP.

dwmac4_tc_ops also uses tc_setup_cbs(), and its mqprio offload is
tc_setup_mqprio_unimplemented. So channel mode can't be the cause on
those parts.

The DT path still accepts SP together with CBS.
stmmac_mtl_configuration() programs CBS for AVB queues whatever the
scheduler is:

	/* Configure CBS in AVB TX queues */
	if (tx_queues_count > 1)
		stmmac_configure_cbs(priv);

The commit message says DCB mode leaves the scheduler to the devicetree.
It doesn't mention any change to CBS. The later patch in the series,
"net: stmmac: align real_num_tx_queues to mqprio configuration", doesn't
change tc_setup_cbs() either.

Could this check test whether channel mode offload is actually active,
for example with a separate flag, rather than the scheduler value?

[ ... ]

> @@ -1265,6 +1268,23 @@ static int stmmac_reset_tc_mqprio(struct net_device *ndev,
>  				  struct netlink_ext_ack *extack)
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> +	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]
This isn't a bug introduced by this patch, but is it safe to access
these registers while the device may be runtime suspended?

stmmac_setup_tc() dispatches TC_SETUP_QDISC_MQPRIO without calling
pm_runtime_resume_and_get(). stmmac_set_mac_address() and the VLAN
add/kill handlers do take that reference.

When the interface is down, the device can be runtime suspended.
stmmac_runtime_suspend()->stmmac_bus_clks_config(priv, false) then gates
stmmac_clk and pclk.

This path already did an unguarded read-modify-write of the FPE control
register through fpe_map_preemption_class. The new stmmac_tx_queue_prio()
and stmmac_prog_mtl_tx_algorithms() calls, here and in
tc_mqprio_config_queue_prio(), add more MMIO to it:

  - MAC_TxQ_Prty_Map / XGMAC_TC_PRTY_MAP
  - MTL_OPERATION_MODE
  - XGMAC_MTL_TCx_ETS_CONTROL on XGMAC

One way to hit this is "ip link set X down" followed by
"tc qdisc add/del ... mqprio ... hw 1".

> +
> +	stmmac_prog_mtl_tx_algorithms(priv, priv->hw,
> +				      priv->plat->tx_sched_algorithm);

[Severity: Medium]
Does this unconditional reprogramming drop CBS on AVB queues on XGMAC?

stmmac_reset_tc_mqprio() runs on every mqprio teardown. That includes
removing a DCB mode mqprio, which is still allowed when AVB queues
exist. XGMAC and XLGMAC use dwmac510_tc_ops, so this call reaches
dwxgmac2_prog_mtl_tx_algorithms(), which clears TSA on every TC:

	for (i = 0; i < MTL_MAX_TX_QUEUES; i++) {
		value = readl(ioaddr + XGMAC_MTL_TCx_ETS_CONTROL(i));
		value &= ~XGMAC_TSA;
		if (ets)
			value |= XGMAC_ETS;
		writel(value, ioaddr + XGMAC_MTL_TCx_ETS_CONTROL(i));
	}

That erases the TSA=CBS setting that dwxgmac2_config_cbs() wrote to the
same register. Before this patch, the only caller was
stmmac_mtl_configuration(), which calls stmmac_configure_cbs()
afterwards, so CBS was always written last.

After "tc qdisc del" of a DCB mqprio, AVB queues set up from the DT
(snps,avb-algorithm) would lose credit-based shaping in hardware.
tx_queues_cfg[q].mode_to_use still says MTL_QUEUE_AVB. This lasts until
the next stmmac_hw_setup().

Should the reset path reapply CBS for AVB queues? Or should it skip
reprogramming the scheduler when channel mode was never offloaded?

> +	priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;

[ ... ]

> @@ -1272,6 +1292,73 @@ static int stmmac_reset_tc_mqprio(struct net_device *ndev,

[ ... ]

> +static int tc_mqprio_validate_chan_mode(struct stmmac_priv *priv,
> +					struct tc_mqprio_qopt_offload *mqprio)
> +{
> +	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
> +	int i;
> +
> +	if (!priv->dma_cap.dcben)
> +		return -EOPNOTSUPP;
> +
> +	/* Forcing strict priority conflicts with the CBS algorithm
> +	 * of AVB queues, so reject the offload when any queue is
> +	 * configured as AVB.
> +	 */
> +	for (i = 0; i < priv->plat->tx_queues_to_use; i++) {
> +		if (priv->plat->tx_queues_cfg[i].mode_to_use == MTL_QUEUE_AVB)
> +			return -EOPNOTSUPP;
> +	}

[Severity: Low]
This isn't a bug, but could the dcben check and the AVB check set an
extack message, as the 1:1 mapping check below does? At the moment the
user only sees "Operation not supported". Missing DCB support looks the
same as a conflict with AVB queues set by the DT or by an earlier cbs
offload.

> +
> +	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 also reject shaper
and rate requests here?

In channel mode, mqprio_enable_offload() passes these through:

	case TC_MQPRIO_MODE_CHANNEL:
		mqprio.flags = priv->flags;
		...
		if (priv->flags & TC_MQPRIO_F_SHAPER)
			mqprio.shaper = priv->shaper;
		if (priv->flags & TC_MQPRIO_F_MIN_RATE)
			...
		if (priv->flags & TC_MQPRIO_F_MAX_RATE)
			...

Neither tc_mqprio_validate_chan_mode() nor tc_setup_dwmac510_mqprio()
checks TC_MQPRIO_F_SHAPER, TC_MQPRIO_F_MIN_RATE or TC_MQPRIO_F_MAX_RATE.
So "mode channel shaper bw_rlimit max_rate ..." is accepted and reported
as offloaded, but no rate limiting is programmed.

The old code also accepted channel mode without checking the shaper. Now
that this function validates channel mode, would it be the right place
to reject shaper != TC_MQPRIO_SHAPER_DCB?

> +
>  static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
>  				    struct tc_mqprio_qopt_offload *mqprio)
>  {
> @@ -1282,23 +1369,22 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,

[ ... ]

>  	for (i = 0; i < qopt->num_tc; i++) {
> +		if (qopt->offset[i] + qopt->count[i] >
> +		    priv->plat->tx_queues_to_use) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Queues exceed hw TX queues");
> +			return -EINVAL;
> +		}
> +
>  		tc_to_txq[i] = (struct netdev_tc_txq) {
>  			.count = qopt->count[i],
>  			.offset = qopt->offset[i],
> @@ -1306,6 +1392,24 @@ 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 does adding up count[] give the right
real_num_tx_queues when the queue layout has gaps?

The core validation and the new offset + count <= tx_queues_to_use check
both allow gaps. Take 4 TX queues and a DCB request with num_tc=1,
count[0]=1, offset[0]=2. num_tx_queues becomes 1, and then:

tc_setup_dwmac510_mqprio()
  netif_set_real_num_tx_queues(ndev, 1)
    netif_setup_tc()
	/* If TC0 is invalidated disable TC mapping */
	if (res.offset + res.count > txq) {
		...
		WRITE_ONCE(dev->num_tc, 0);
		return;
	}

With preemptible_tcs == 0 the function still returns 0. The offload
reports success, but TC classification is disabled and only queue 0 is
used.

Would the highest offset + count avoid this, instead of the sum? The
later patch "net: stmmac: align real_num_tx_queues to mqprio
configuration" saves this same value in xmit_qdisc.num_tx_queues and
applies it on every open.

[ ... ]

> @@ -1320,6 +1424,9 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
>  	if (err)
>  		goto error_reset_num_tx_queues;
>  
> +	if (mode == TC_MQPRIO_MODE_CHANNEL)
> +		tc_mqprio_config_queue_prio(priv, qopt);
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but does this configuration
survive replacing an offloaded mqprio root with one that has a different
handle?

Take "tc qdisc replace dev X root handle 2: mqprio ... mode channel
hw 1" over an existing offloaded root 1:. qdisc_create() initializes the
new qdisc first. That reaches this call and programs SP and the queue
priorities.

qdisc_graft() then destroys the old qdisc:

qdisc_graft()
  notify_and_destroy()
    mqprio_destroy()
      mqprio_disable_offload()      /* num_tc = 0 */
        tc_setup_dwmac510_mqprio()
          stmmac_reset_tc_mqprio()

That resets the scheduler, the per-queue priorities and xmit_qdisc to
the platform defaults. mqprio's ->attach doesn't offload again, and
stmmac doesn't handle TC_SETUP_ROOT_QDISC. The replace reports success,
but the new qdisc's channel configuration is gone.

Before this patch, the same ordering already wiped the netdev TC map,
real_num_tx_queues and the FPE mapping. This patch adds the scheduler
and priority state to what gets lost.

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

[PATCH net-next v2 2/2] net: stmmac: align real_num_tx_queues to mqprio configuration

[Lorenzo Bianconi]

stmmac_hw_setup() unconditionally sets the number of real TX queues to
plat->tx_queues_to_use on every device open. When a tc-mqprio qdisc is
offloaded the driver reduces netif_set_real_num_tx_queues() to the number
of queues enabled by the offload, but the next device open reverts it to
the devicetree value while the netdev TC map is still the one programmed
by the qdisc, leaving the two inconsistent.

Store the number of TX queues enabled by the current qdisc configuration
in the per-qdisc state (priv->xmit_qdisc.num_tx_queues) and use it in
stmmac_hw_setup() in place of the devicetree value.

The field defaults to plat->tx_queues_to_use at probe time and is restored
to it when the mqprio qdisc is destroyed or its setup fails, so a device
without an active mqprio offload keeps the devicetree configured queue
count across reopens.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      | 1 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c   | 3 +++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 69b400e9b138..a8f06192d696 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -302,6 +302,7 @@ struct stmmac_priv {
 
 	struct {
 		u32 prio[MTL_MAX_TX_QUEUES];
+		u32 num_tx_queues;
 		u8 algo;
 	} xmit_qdisc;
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 0c9083f8846e..ad6be5166b95 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3772,7 +3772,7 @@ static int stmmac_hw_setup(struct net_device *dev)
 
 	/* Configure real RX and TX queues */
 	netif_set_real_num_rx_queues(dev, priv->plat->rx_queues_to_use);
-	netif_set_real_num_tx_queues(dev, priv->plat->tx_queues_to_use);
+	netif_set_real_num_tx_queues(dev, priv->xmit_qdisc.num_tx_queues);
 
 	/* Start the ball rolling... */
 	stmmac_start_all_dma(priv);
@@ -7923,6 +7923,7 @@ static int __stmmac_dvr_probe(struct device *device,
 		priv->xmit_qdisc.prio[i] = priv->plat->tx_queues_cfg[i].prio;
 	}
 	priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;
+	priv->xmit_qdisc.num_tx_queues = priv->plat->tx_queues_to_use;
 
 	if (priv->plat->flags & STMMAC_FLAG_MULTI_MSI_EN) {
 		ret = stmmac_msi_init(priv, res);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 54242461a860..1accc8e962d9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -1285,6 +1285,7 @@ static int stmmac_reset_tc_mqprio(struct net_device *ndev,
 	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.num_tx_queues = priv->plat->tx_queues_to_use;
 
 	netdev_reset_tc(ndev);
 	netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use);
@@ -1427,6 +1428,8 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 	if (mode == TC_MQPRIO_MODE_CHANNEL)
 		tc_mqprio_config_queue_prio(priv, qopt);
 
+	priv->xmit_qdisc.num_tx_queues = num_tx_queues;
+
 	return 0;
 
 error_reset_num_tx_queues:

-- 
2.55.0

Re: [PATCH net-next v2 2/2] net: stmmac: align real_num_tx_queues to mqprio configuration

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 0 · Low: 2

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).

- [High] After this patch, stmmac_hw_setup() sets the real TX queue count
  from the cached priv->xmit_qdisc.num_tx_queues instead of…
- [High] __stmmac_dvr_probe() sets priv->xmit_qdisc.num_tx_queues =
  priv->plat->tx_queues_to_use (line 7926) before calling…
- [Low] The commit message says the new field 'is restored to
  [plat->tx_queues_to_use] when the mqprio qdisc is destroyed or its…
- [Low] The patch fixes a user-visible inconsistency: on reopen,
  real_num_tx_queues is reset to plat->tx_queues_to_use while the netdev…

--

net: stmmac: align real_num_tx_queues to mqprio configuration

This commit adds priv->xmit_qdisc.num_tx_queues to hold the number of TX
queues enabled by the current mqprio offload. stmmac_hw_setup() now uses
it instead of plat->tx_queues_to_use when setting real_num_tx_queues on
open. The field is set at probe, and reset when the mqprio offload is
removed.

> The field defaults to plat->tx_queues_to_use at probe time and is restored
> to it when the mqprio qdisc is destroyed or its setup fails, so a device
> without an active mqprio offload keeps the devicetree configured queue
> count across reopens.

[Severity: Low]

Is the "or its setup fails" part accurate? In tc_setup_dwmac510_mqprio(),
the new field is only assigned on the success path:

	priv->xmit_qdisc.num_tx_queues = num_tx_queues;

	return 0;

error_reset_num_tx_queues:
	...
error_reset_tc:
	...

The error labels roll back algo, the TC map and real_num_tx_queues. They
leave num_tx_queues at the previous configuration's value.

stmmac_reset_tc_mqprio() does not run on a failed setup either. Because
hw_offload was never set, mqprio_destroy() calls netdev_set_num_tc(dev, 0)
instead of ndo_setup_tc.

Keeping the previous value matches commit 02fffd1939f6 ("net: stmmac:
preserve real_num_tx_queues on mqprio setup failure"). So the code looks
right, and it is the description that differs.

The message also calls plat->tx_queues_to_use "the devicetree value".
stmmac_hw_init() clamps that value, and ethtool -L can change it at
runtime (see the comments below).

> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

[Severity: Low]

Should this have a Fixes: tag? The mqprio offload has been lowering
real_num_tx_queues since commit 195e4f409a40 ("net: stmmac: support fp
parameter of tc-mqprio"). The related fix 02fffd1939f6 carries
Fixes: 195e4f409a40.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 0c9083f8846e0..ad6be5166b956 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3772,7 +3772,7 @@ static int stmmac_hw_setup(struct net_device *dev)
>  
>  	/* Configure real RX and TX queues */
>  	netif_set_real_num_rx_queues(dev, priv->plat->rx_queues_to_use);
> -	netif_set_real_num_tx_queues(dev, priv->plat->tx_queues_to_use);
> +	netif_set_real_num_tx_queues(dev, priv->xmit_qdisc.num_tx_queues);

[Severity: High]

What keeps priv->xmit_qdisc.num_tx_queues in sync with
plat->tx_queues_to_use when the channel count is changed with ethtool?

stmmac_set_channels()->stmmac_reinit_queues() only updates the plat
fields before it reopens the device:

stmmac_reinit_queues() {
    ...
	priv->plat->rx_queues_to_use = rx_cnt;
	priv->plat->tx_queues_to_use = tx_cnt;
    ...
}

The new field is only written in three places: __stmmac_dvr_probe(),
stmmac_reset_tc_mqprio() and the success path of
tc_setup_dwmac510_mqprio().

Take "ethtool -L eth0 tx 2" on a 4-queue device with no mqprio offload.
The reopen then does this:

__stmmac_open()
  alloc_dma_tx_desc_resources()   /* rings for queues 0..1 only */
  stmmac_hw_setup()
    netif_set_real_num_tx_queues(dev, 4)
    stmmac_start_all_dma()        /* starts 2 TX channels */

The netif_set_real_num_tx_queues() call succeeds because its only upper
bound is dev->num_tx_queues, which is MTL_MAX_TX_QUEUES. Since dma_conf
is freshly kzalloc'd, tx_queue[2..3].tx_skbuff and dma_tx are NULL.

stmmac has no ndo_select_queue, so skb_tx_hash() can pick queue 2 or 3.
stmmac_xmit() takes skb_get_queue_mapping() without a bounds check and
then does:

	WARN_ON(tx_q->tx_skbuff[first_entry]);

Can this dereference a NULL tx_skbuff? stmmac_tso_xmit() seems to have
the same problem. The same thing happens if the channel change is made
while the interface is down and applied on the next open.

In the other direction, when TX channels are increased, the extra queues
are never exposed. ethtool -L then silently has no effect on TX, and
ethtool -l reports a count the netdev does not use.

Before this patch, stmmac_hw_setup() always used the current
plat->tx_queues_to_use, so real_num_tx_queues matched the allocated rings.

Would it make sense to update or clamp the cached value in
stmmac_reinit_queues()? Using min(xmit_qdisc.num_tx_queues,
plat->tx_queues_to_use) in stmmac_hw_setup() would also work.

Either way, what should happen to an active mqprio offload whose ranges
no longer fit the new count? It could be reset, or the channel change
could be rejected.

[ ... ]

> @@ -7923,6 +7923,7 @@ static int __stmmac_dvr_probe(struct device *device,
>  		priv->xmit_qdisc.prio[i] = priv->plat->tx_queues_cfg[i].prio;
>  	}
>  	priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;
> +	priv->xmit_qdisc.num_tx_queues = priv->plat->tx_queues_to_use;

[Severity: High]

Is this set too early in probe? __stmmac_dvr_probe() calls
stmmac_hw_init() later, and that can lower plat->tx_queues_to_use:

stmmac_hw_init() {
    ...
	if (priv->dma_cap.number_tx_queues &&
	    priv->plat->tx_queues_to_use > priv->dma_cap.number_tx_queues) {
		dev_warn(priv->device,
			 "Number of Tx queues (%u) exceeds dma capability\n",
			 priv->plat->tx_queues_to_use);
		priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues;
	}
    ...
}

The cached value keeps the count from before the clamp. Take a platform
where the devicetree or glue driver asks for more TX queues than the core
has. Every open then calls netif_set_real_num_tx_queues() with the larger
count.

Ring allocation, DMA init, NAPI and stmmac_start_all_dma() all use the
clamped plat->tx_queues_to_use.

Could traffic hashed to the extra queues hit the same NULL
tx_q->tx_skbuff dereference in stmmac_xmit()? That would need no
reconfiguration, only opening the interface and sending traffic.

The only thing that brings the cache back in sync is installing and then
removing an mqprio qdisc, through stmmac_reset_tc_mqprio().

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