[PATCH net-next] net: stmmac: add tc-mqprio qdisc offload

[PATCH net-next] net: stmmac: add tc-mqprio qdisc offload

[Lorenzo Bianconi]

Implement offload of the tc-mqprio qdisc in the stmmac driver. 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 offload requires the DCB feature and a 1:1 TC to TX queue mapping,
with a single queue per TC and per-TC offsets below the number of traffic
classes, so every queue referenced by the map stays within the TX queues
enabled by the offload. Configurations with AVB queues are rejected, since
forcing strict priority conflicts with the CBS algorithm.

The per-queue priority bitmasks and the scheduling algorithm are stored in
the new per-qdisc state (priv->xmit_qdisc) so they can be reprogrammed on
device reopen.

Destroying the mqprio qdisc, or failing its setup, restores the netdev TC
map and TX queue count to the previously saved values and resets the
per-qdisc state to the devicetree configuration. The devicetree configured
scheduling algorithm, TX queue priorities and TX queue weights are then
restored on the next device open.

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   | 77 ++++++++++++++++++++++-
 4 files changed, 96 insertions(+), 14 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..a2a9680da5c0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -1265,6 +1265,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);
@@ -1278,6 +1295,7 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 	unsigned int ndev_num_tx_queues, num_tx_queues = 0;
 	struct netdev_tc_txq ndev_tc_to_txq[TC_MAX_QUEUE];
 	struct netdev_tc_txq tc_to_txq[TC_MAX_QUEUE] = {};
+	struct plat_stmmacenet_data *pdata = priv->plat;
 	struct netlink_ext_ack *extack = mqprio->extack;
 	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
 	struct net_device *ndev = priv->dev;
@@ -1290,6 +1308,17 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 	if (qopt->num_tc > ARRAY_SIZE(tc_to_txq))
 		return -EINVAL;
 
+	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 < pdata->tx_queues_to_use; i++) {
+		if (pdata->tx_queues_cfg[i].mode_to_use == MTL_QUEUE_AVB)
+			return -EOPNOTSUPP;
+	}
+
 	/* save current tc values for reset */
 	ndev_ntc = netdev_get_num_tc(ndev);
 	for (i = 0; i < ARRAY_SIZE(ndev->tc_to_txq); i++)
@@ -1299,6 +1328,20 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 		ndev_prio_tc_map[i] = READ_ONCE(ndev->prio_tc_map[i]);
 
 	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) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "SP sched requires 1:1 TXQ map");
+			return -EOPNOTSUPP;
+		}
+
+		if (qopt->offset[i] >= qopt->num_tc) {
+			NL_SET_ERR_MSG_MOD(extack, "TX queue range exceeded");
+			return -EINVAL;
+		}
+
 		tc_to_txq[i] = (struct netdev_tc_txq) {
 			.count = qopt->count[i],
 			.offset = qopt->offset[i],
@@ -1315,14 +1358,44 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 	if (err)
 		goto error_reset_tc;
 
+	priv->xmit_qdisc.algo = MTL_TX_ALGORITHM_SP;
 	err = stmmac_fpe_map_preemption_class(priv, ndev, extack,
 					      mqprio->preemptible_tcs);
 	if (err)
-		goto error_reset_num_tx_queues;
+		goto error_reset_xmit_algo;
+
+	for (i = 0; i < pdata->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);
 
 	return 0;
 
-error_reset_num_tx_queues:
+error_reset_xmit_algo:
+	priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;
 	if (netif_set_real_num_tx_queues(ndev, ndev_num_tx_queues))
 		netdev_warn(ndev, "Failed to restore %u TX queues\n",
 			    ndev_num_tx_queues);

---
base-commit: 4bb9710c6a68d35207f123aef55dcd50e7195ec5
change-id: 20260918-stmmac-mqprio-prio-offload-3d82d87f7bc8

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

Re: [PATCH net-next] net: stmmac: add tc-mqprio qdisc offload

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 6 (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] One-directional AVB/CBS rejection: tc_setup_dwmac510_mqprio()
  rejects the mqprio offload if any TX queue is already MTL_QUEUE_AVB…
- [Medium] The patch narrows an mqprio offload that already existed,
  breaking configurations the driver accepted before, and the commit…
- [Medium] The new per-TC offset validation bounds qopt->offset[i]
  against qopt->num_tc (stmmac_tc.c:1340-1343, extack 'TX queue range…
- [Medium] Replacing an offloaded mqprio qdisc makes the old qdisc's
  teardown wipe the replacement's just-installed hardware state.
- [Medium] The new mqprio setup and teardown paths perform read-modify-
  write MMIO from ndo_setup_tc without a runtime-PM reference and…
- [Medium] XGMAC index mismatch: the loop at stmmac_tc.c:1367-1390
  resolves, for each TX queue index i, the traffic class j that owns…

Pre-existing issues:
- [Medium] The mqprio offload accepts TC_MQPRIO_MODE_CHANNEL with
  TC_MQPRIO_SHAPER_BW_RATE and returns success while ignoring…

--

net: stmmac: add tc-mqprio qdisc offload

The patch implements tc-mqprio offload in stmmac by switching the MTL TX
scheduler to strict priority and programming each TX queue's PSTQX/PSTC
priority bitmask from qopt->prio_tc_map. The per-queue masks and the
scheduling algorithm are cached in the new priv->xmit_qdisc so they can be
re-applied on reopen, and teardown restores the devicetree configuration.

A few questions below about the new validation and register programming.

> 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

[ ... ]

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

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 42a00446e9b4..a2a9680da5c0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -1265,6 +1265,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);

[Severity: Medium]
Can a qdisc replace end up undoing the offload that was just installed?

For "tc qdisc replace dev X root mqprio ... hw 1" on top of an already
offloaded mqprio, the core creates and offloads the new qdisc first, and
only then destroys the old one:

net/sched/sch_api.c:qdisc_graft() {
	...
	notify_and_destroy(net, skb, n, classid, old, new, extack);

	if (new && new->ops->attach)
		new->ops->attach(new);
}

qdisc_put(old) -> mqprio_destroy() -> mqprio_disable_offload() issues
num_tc == 0, which tc_setup_dwmac510_mqprio() forwards to
stmmac_reset_tc_mqprio().  That now rewrites the PSTQX priorities and the
MTL scheduling algorithm back to the devicetree values, and also resets
priv->xmit_qdisc, while mqprio_attach() does not re-apply the offload.

The result looks like the replacement qdisc reporting hw offload while the
MAC is back on the devicetree scheduler and priorities, and the stale
cached state is re-applied on the next open.  Should the reset path check
whether an offloaded mqprio is still installed before reprogramming?

[Severity: Medium]
Is it safe to touch these registers from ndo_setup_tc without a runtime PM
reference?

stmmac_reset_tc_mqprio() and the setup path both call
stmmac_tx_queue_prio() and stmmac_prog_mtl_tx_algorithms() unconditionally,
and those are read-modify-write accesses, e.g.:

drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:dwmac4_tx_queue_priority() {
	value = readl(ioaddr + base_register);

	value &= ~GMAC_TXQCTRL_PSTQX_MASK(queue);
	value |= (prio << GMAC_TXQCTRL_PSTQX_SHIFT(queue)) & ...;

	writel(value, ioaddr + base_register);
}

The driver drops its PM reference at the end of probe and in
stmmac_release(), and stmmac_runtime_suspend() gates the bus/CSR clocks via
stmmac_bus_clks_config(priv, false).  Adding or deleting an mqprio qdisc on
a down interface therefore reaches these accesses with the MAC clock
gated, where the readl can return an undefined value that is then written
back over the other queues' PSTQ fields and the other MTL_Operation_Mode
bits.

Other stmmac control paths that touch registers outside open (set_mac_address,
VLAN add/del) wrap the access in pm_runtime_get_sync()/pm_runtime_put().
Would a similar guard, or a netif_running() check, be appropriate here?
These writes also look redundant while the device is down, since
stmmac_mtl_configuration() re-applies priv->xmit_qdisc on the next open.

> @@ -1290,6 +1308,17 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
>  	if (qopt->num_tc > ARRAY_SIZE(tc_to_txq))
>  		return -EINVAL;
>  
> +	if (!priv->dma_cap.dcben)
> +		return -EOPNOTSUPP;
> +

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but since
the entry validation is being extended here: should mqprio->mode,
mqprio->shaper, mqprio->min_rate[] and mqprio->max_rate[] be rejected?

net/sched/sch_mqprio.c:mqprio_enable_offload() copies those into
tc_mqprio_qopt_offload for TC_MQPRIO_MODE_CHANNEL and relies on the driver
to refuse what it cannot do:

	case TC_MQPRIO_MODE_CHANNEL:
		mqprio.flags = priv->flags;
		...
		if (priv->flags & TC_MQPRIO_F_MAX_RATE)
			for (i = 0; i < mqprio.qopt.num_tc; i++)
				mqprio.max_rate[i] = priv->max_rate[i];

tc_setup_dwmac510_mqprio() only consumes qopt and preemptible_tcs, so
"mode channel shaper bw_rlimit max_rate ... hw 1" returns success and the
rate limits are echoed back to userspace without any shaping being
programmed.

[Severity: Medium]
Does this narrow an offload that already worked?

The baseline tc_setup_dwmac510_mqprio() validated only num_tc == 0 and
num_tc > ARRAY_SIZE(tc_to_txq).  The new !priv->dma_cap.dcben check, the
AVB-queue check below, and the count[i] > 1 check make configurations that
previously succeeded return -EOPNOTSUPP, including the FPE preemptible-TC
mapping part of the offload which is unrelated to the new priority
programming.

The count > 1 case looks like it was intentionally supported:

drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c:dwmac5_fpe_map_preemption_class() {
	...
		if (count == 1)
			continue;

		if (priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP) {
			NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
			return -EINVAL;
		}

		queue_weight = priv->plat->tx_queues_cfg[offset].weight;
	...
}

That weight-checking branch becomes unreachable from the mqprio path now
that the algorithm is forced to SP before it runs.  Configurations such as
"mqprio ... queues 2@0 2@2 fp E P hw 1" lose the offload.

Additionally, forcing MTL_TX_ALGORITHM_SP overrides the devicetree
tx-sched-algorithm and the devicetree per-queue priority masks for every
mqprio user that is still accepted.  Should the commit message mention
these behaviour changes for existing users rather than describing them only
as offload requirements?

> +	/* 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 < pdata->tx_queues_to_use; i++) {
> +		if (pdata->tx_queues_cfg[i].mode_to_use == MTL_QUEUE_AVB)
> +			return -EOPNOTSUPP;
> +	}
> +

[Severity: Medium]
Is this rejection effective in only one direction?

tc_setup_cbs() in the same file has no reciprocal check.  It validates the
queue index, priv->dma_cap.av and the slope parameters, then converts a DCB
queue into an AVB queue and programs the shaper:

	mode_to_use = priv->plat->tx_queues_cfg[queue].mode_to_use;
	if (mode_to_use == MTL_QUEUE_DCB && qopt->enable) {
		ret = stmmac_dma_qmode(priv, priv->ioaddr, queue, MTL_QUEUE_AVB);
		...
		priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;

Nothing there consults priv->xmit_qdisc.algo or netdev_get_num_tc(), so the
usual TSN order

  tc qdisc add dev X root mqprio ... hw 1
  tc qdisc replace dev X parent <mqprio child> cbs ... offload 1

succeeds and leaves the MAC in strict priority with a CBS/AVB queue, which
is the combination this new check and the commit message declare
unsupported.

The mismatch also survives a reopen, since stmmac_mtl_configuration()
programs the cached SP algorithm and then re-applies CBS:

	stmmac_prog_mtl_tx_algorithms(priv, priv->hw, priv->xmit_qdisc.algo);
	/* Configure CBS in AVB TX queues */
	if (tx_queues_count > 1)
		stmmac_configure_cbs(priv);

Should tc_setup_cbs() carry the symmetric check on priv->xmit_qdisc.algo,
or is the mqprio-side rejection not needed?

> @@ -1299,6 +1328,20 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
>  		ndev_prio_tc_map[i] = READ_ONCE(ndev->prio_tc_map[i]);
>  
>  	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) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "SP sched requires 1:1 TXQ map");
> +			return -EOPNOTSUPP;
> +		}
> +
> +		if (qopt->offset[i] >= qopt->num_tc) {
> +			NL_SET_ERR_MSG_MOD(extack, "TX queue range exceeded");
> +			return -EINVAL;
> +		}
> +

[Severity: Medium]
Should this bound be priv->plat->tx_queues_to_use rather than
qopt->num_tc?

num_tc is only limited by ARRAY_SIZE(tc_to_txq) here and by the core:

net/sched/sch_mqprio_lib.c:mqprio_validate_queue_counts() {
	...
		if (qopt->offset[i] >= dev->real_num_tx_queues ||
		    last > dev->real_num_tx_queues) {
	...
}

stmmac allocates the netdev with MTL_MAX_TX_QUEUES queues and only clamps
real_num_tx_queues to plat->tx_queues_to_use in stmmac_hw_setup(), so
before the first open a request like "mqprio num_tc 4 ... queues 1@0 1@1
1@2 1@3 hw 1" on a device with tx_queues_to_use == 2 passes both checks.

The priority programming loop below is bounded by pdata->tx_queues_to_use,
so the TCs with offset >= tx_queues_to_use never get a PSTQX/PSTC mask
while the offload still returns success, and netif_setup_tc() remaps those
priorities to TC0 at the next open.

The same check also rejects a sparse but otherwise legal map, e.g.
num_tc 2 with "queues 1@0 1@2".  Is that intentional?

> @@ -1315,14 +1358,44 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
>  	if (err)
>  		goto error_reset_tc;
>  
> +	priv->xmit_qdisc.algo = MTL_TX_ALGORITHM_SP;
>  	err = stmmac_fpe_map_preemption_class(priv, ndev, extack,
>  					      mqprio->preemptible_tcs);
>  	if (err)
> -		goto error_reset_num_tx_queues;
> +		goto error_reset_xmit_algo;
> +
> +	for (i = 0; i < pdata->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);

[Severity: Medium]
Is the last argument the right index on XGMAC?

The mask computed here belongs to traffic class j, but the index passed to
stmmac_tx_queue_prio() is the queue index i.  For XGMAC the callback is:

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:dwxgmac2_tx_queue_prio() {
	reg = (queue < 4) ? XGMAC_TC_PRTY_MAP0 : XGMAC_TC_PRTY_MAP1;
	...
	value &= ~XGMAC_PSTC(queue);
	value |= (prio << XGMAC_PSTC_SHIFT(queue)) & XGMAC_PSTC(queue);
}

XGMAC_PSTC() selects a traffic-class field in MTL_TC_Prty_Map0/1, while the
queue to TC association is programmed separately in XGMAC_Q2TCMAP:

drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c:dwxgmac3_fpe_map_preemption_class() {
	for (u32 i = 0; i < count; i++) {
		val = readl(priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(offset + i));
		writel(u32_replace_bits(val, tc, XGMAC_Q2TCMAP), ...);
}

Both dwxgmac210 and dwxlgmac2 use dwmac510_tc_ops, and a permuted map such
as "mqprio num_tc 2 map ... queues 1@1 1@0 hw 1" passes both
intervals_overlap() in the core and the new offset < num_tc check.  In that
case TC0's mask would be written into the PSTC field of TC1 and vice versa.
Should the XGMAC path program PSTC using the TC index, or should
non-identity offsets be rejected?

> +		priv->xmit_qdisc.prio[i] = prio;
> +	}
> +
> +	stmmac_prog_mtl_tx_algorithms(priv, priv->hw, MTL_TX_ALGORITHM_SP);
>  
>  	return 0;
>  
> -error_reset_num_tx_queues:
> +error_reset_xmit_algo:
> +	priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;
>  	if (netif_set_real_num_tx_queues(ndev, ndev_num_tx_queues))
>  		netdev_warn(ndev, "Failed to restore %u TX queues\n",
>  			    ndev_num_tx_queues);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-stmmac-mqprio-prio-offload-v1-1-5328157fcb58%40oss.qualcomm.com

Re: [PATCH net-next] net: stmmac: add tc-mqprio qdisc offload

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 17865 bytes --]

> Thank you for your contribution! Sashiko AI review found 6 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 6 (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] One-directional AVB/CBS rejection: tc_setup_dwmac510_mqprio()
>   rejects the mqprio offload if any TX queue is already MTL_QUEUE_AVB…
> - [Medium] The patch narrows an mqprio offload that already existed,
>   breaking configurations the driver accepted before, and the commit…
> - [Medium] The new per-TC offset validation bounds qopt->offset[i]
>   against qopt->num_tc (stmmac_tc.c:1340-1343, extack 'TX queue range…
> - [Medium] Replacing an offloaded mqprio qdisc makes the old qdisc's
>   teardown wipe the replacement's just-installed hardware state.
> - [Medium] The new mqprio setup and teardown paths perform read-modify-
>   write MMIO from ndo_setup_tc without a runtime-PM reference and…
> - [Medium] XGMAC index mismatch: the loop at stmmac_tc.c:1367-1390
>   resolves, for each TX queue index i, the traffic class j that owns…
> 
> Pre-existing issues:
> - [Medium] The mqprio offload accepts TC_MQPRIO_MODE_CHANNEL with
>   TC_MQPRIO_SHAPER_BW_RATE and returns success while ignoring…
> 
> --
> 
> net: stmmac: add tc-mqprio qdisc offload
> 
> The patch implements tc-mqprio offload in stmmac by switching the MTL TX
> scheduler to strict priority and programming each TX queue's PSTQX/PSTC
> priority bitmask from qopt->prio_tc_map. The per-queue masks and the
> scheduling algorithm are cached in the new priv->xmit_qdisc so they can be
> re-applied on reopen, and teardown restores the devicetree configuration.
> 
> A few questions below about the new validation and register programming.
> 
> > 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
> 
> [ ... ]
> 
> > @@ -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)
> 
> [ ... ]
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > index 42a00446e9b4..a2a9680da5c0 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> > @@ -1265,6 +1265,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);
> 
> [Severity: Medium]
> Can a qdisc replace end up undoing the offload that was just installed?
> 
> For "tc qdisc replace dev X root mqprio ... hw 1" on top of an already
> offloaded mqprio, the core creates and offloads the new qdisc first, and
> only then destroys the old one:
> 
> net/sched/sch_api.c:qdisc_graft() {
> 	...
> 	notify_and_destroy(net, skb, n, classid, old, new, extack);
> 
> 	if (new && new->ops->attach)
> 		new->ops->attach(new);
> }
> 
> qdisc_put(old) -> mqprio_destroy() -> mqprio_disable_offload() issues
> num_tc == 0, which tc_setup_dwmac510_mqprio() forwards to
> stmmac_reset_tc_mqprio().  That now rewrites the PSTQX priorities and the
> MTL scheduling algorithm back to the devicetree values, and also resets
> priv->xmit_qdisc, while mqprio_attach() does not re-apply the offload.
> 
> The result looks like the replacement qdisc reporting hw offload while the
> MAC is back on the devicetree scheduler and priorities, and the stale
> cached state is re-applied on the next open.  Should the reset path check
> whether an offloaded mqprio is still installed before reprogramming?

I guess this issue is already present and it has not been introduced by
this patch. I think the problem should be fixed with a dedicated patch.
Moreover, the fix would require to change sch_mqprio in order to provide 
the handle of the offloaded qdisc.

> 
> [Severity: Medium]
> Is it safe to touch these registers from ndo_setup_tc without a runtime PM
> reference?
> 
> stmmac_reset_tc_mqprio() and the setup path both call
> stmmac_tx_queue_prio() and stmmac_prog_mtl_tx_algorithms() unconditionally,
> and those are read-modify-write accesses, e.g.:
> 
> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:dwmac4_tx_queue_priority() {
> 	value = readl(ioaddr + base_register);
> 
> 	value &= ~GMAC_TXQCTRL_PSTQX_MASK(queue);
> 	value |= (prio << GMAC_TXQCTRL_PSTQX_SHIFT(queue)) & ...;
> 
> 	writel(value, ioaddr + base_register);
> }
> 
> The driver drops its PM reference at the end of probe and in
> stmmac_release(), and stmmac_runtime_suspend() gates the bus/CSR clocks via
> stmmac_bus_clks_config(priv, false).  Adding or deleting an mqprio qdisc on
> a down interface therefore reaches these accesses with the MAC clock
> gated, where the readl can return an undefined value that is then written
> back over the other queues' PSTQ fields and the other MTL_Operation_Mode
> bits.

I think this issue is not introduce by this patch and it is already present (so
it needs to be fixed with a dedicated patch). There is already a patch upstream
to resolve the problem:
https://lore.kernel.org/netdev/20260827-stmmac-setup-tc-enable-pm-v2-1-a9b8a5948f41@oss.qualcomm.com/

> 
> Other stmmac control paths that touch registers outside open (set_mac_address,
> VLAN add/del) wrap the access in pm_runtime_get_sync()/pm_runtime_put().
> Would a similar guard, or a netif_running() check, be appropriate here?
> These writes also look redundant while the device is down, since
> stmmac_mtl_configuration() re-applies priv->xmit_qdisc on the next open.
> 
> > @@ -1290,6 +1308,17 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
> >  	if (qopt->num_tc > ARRAY_SIZE(tc_to_txq))
> >  		return -EINVAL;
> >  
> > +	if (!priv->dma_cap.dcben)
> > +		return -EOPNOTSUPP;
> > +
> 
> [Severity: Medium]
> This is a pre-existing issue and not introduced by this patch, but since
> the entry validation is being extended here: should mqprio->mode,
> mqprio->shaper, mqprio->min_rate[] and mqprio->max_rate[] be rejected?

As pointed out by sashiko, this issue is already preset and it should be fixed
with a dedicated patch.

> 
> net/sched/sch_mqprio.c:mqprio_enable_offload() copies those into
> tc_mqprio_qopt_offload for TC_MQPRIO_MODE_CHANNEL and relies on the driver
> to refuse what it cannot do:
> 
> 	case TC_MQPRIO_MODE_CHANNEL:
> 		mqprio.flags = priv->flags;
> 		...
> 		if (priv->flags & TC_MQPRIO_F_MAX_RATE)
> 			for (i = 0; i < mqprio.qopt.num_tc; i++)
> 				mqprio.max_rate[i] = priv->max_rate[i];
> 
> tc_setup_dwmac510_mqprio() only consumes qopt and preemptible_tcs, so
> "mode channel shaper bw_rlimit max_rate ... hw 1" returns success and the
> rate limits are echoed back to userspace without any shaping being
> programmed.
> 
> [Severity: Medium]
> Does this narrow an offload that already worked?
> 
> The baseline tc_setup_dwmac510_mqprio() validated only num_tc == 0 and
> num_tc > ARRAY_SIZE(tc_to_txq).  The new !priv->dma_cap.dcben check, the
> AVB-queue check below, and the count[i] > 1 check make configurations that
> previously succeeded return -EOPNOTSUPP, including the FPE preemptible-TC
> mapping part of the offload which is unrelated to the new priority
> programming.
> 
> The count > 1 case looks like it was intentionally supported:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c:dwmac5_fpe_map_preemption_class() {
> 	...
> 		if (count == 1)
> 			continue;
> 
> 		if (priv->xmit_qdisc.algo == MTL_TX_ALGORITHM_SP) {
> 			NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
> 			return -EINVAL;
> 		}
> 
> 		queue_weight = priv->plat->tx_queues_cfg[offset].weight;
> 	...
> }
> 
> That weight-checking branch becomes unreachable from the mqprio path now
> that the algorithm is forced to SP before it runs.  Configurations such as
> "mqprio ... queues 2@0 2@2 fp E P hw 1" lose the offload.

I think the new sanity checks are required in order to properly support SP
priority offloading based on the hw capabilities. There were not necessary
before since this hw capability was not available.

> 
> Additionally, forcing MTL_TX_ALGORITHM_SP overrides the devicetree
> tx-sched-algorithm and the devicetree per-queue priority masks for every
> mqprio user that is still accepted.  Should the commit message mention
> these behaviour changes for existing users rather than describing them only
> as offload requirements?
> 
> > +	/* 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 < pdata->tx_queues_to_use; i++) {
> > +		if (pdata->tx_queues_cfg[i].mode_to_use == MTL_QUEUE_AVB)
> > +			return -EOPNOTSUPP;
> > +	}
> > +
> 
> [Severity: Medium]
> Is this rejection effective in only one direction?
> 
> tc_setup_cbs() in the same file has no reciprocal check.  It validates the
> queue index, priv->dma_cap.av and the slope parameters, then converts a DCB
> queue into an AVB queue and programs the shaper:
> 
> 	mode_to_use = priv->plat->tx_queues_cfg[queue].mode_to_use;
> 	if (mode_to_use == MTL_QUEUE_DCB && qopt->enable) {
> 		ret = stmmac_dma_qmode(priv, priv->ioaddr, queue, MTL_QUEUE_AVB);
> 		...
> 		priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
> 
> Nothing there consults priv->xmit_qdisc.algo or netdev_get_num_tc(), so the
> usual TSN order
> 
>   tc qdisc add dev X root mqprio ... hw 1
>   tc qdisc replace dev X parent <mqprio child> cbs ... offload 1
> 
> succeeds and leaves the MAC in strict priority with a CBS/AVB queue, which
> is the combination this new check and the commit message declare
> unsupported.
> 
> The mismatch also survives a reopen, since stmmac_mtl_configuration()
> programs the cached SP algorithm and then re-applies CBS:
> 
> 	stmmac_prog_mtl_tx_algorithms(priv, priv->hw, priv->xmit_qdisc.algo);
> 	/* Configure CBS in AVB TX queues */
> 	if (tx_queues_count > 1)
> 		stmmac_configure_cbs(priv);
> 
> Should tc_setup_cbs() carry the symmetric check on priv->xmit_qdisc.algo,
> or is the mqprio-side rejection not needed?

I think even this one is an issue already present since the user can provide
queue priorities via device tree and at the same time create a cbs qdisc
offloaded in hw. I think we should provide a fix that needs to be backported
here.

> 
> > @@ -1299,6 +1328,20 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
> >  		ndev_prio_tc_map[i] = READ_ONCE(ndev->prio_tc_map[i]);
> >  
> >  	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) {
> > +			NL_SET_ERR_MSG_MOD(extack,
> > +					   "SP sched requires 1:1 TXQ map");
> > +			return -EOPNOTSUPP;
> > +		}
> > +
> > +		if (qopt->offset[i] >= qopt->num_tc) {
> > +			NL_SET_ERR_MSG_MOD(extack, "TX queue range exceeded");
> > +			return -EINVAL;
> > +		}
> > +
> 
> [Severity: Medium]
> Should this bound be priv->plat->tx_queues_to_use rather than
> qopt->num_tc?
> 
> num_tc is only limited by ARRAY_SIZE(tc_to_txq) here and by the core:
> 
> net/sched/sch_mqprio_lib.c:mqprio_validate_queue_counts() {
> 	...
> 		if (qopt->offset[i] >= dev->real_num_tx_queues ||
> 		    last > dev->real_num_tx_queues) {
> 	...
> }
> 
> stmmac allocates the netdev with MTL_MAX_TX_QUEUES queues and only clamps
> real_num_tx_queues to plat->tx_queues_to_use in stmmac_hw_setup(), so
> before the first open a request like "mqprio num_tc 4 ... queues 1@0 1@1
> 1@2 1@3 hw 1" on a device with tx_queues_to_use == 2 passes both checks.
> 
> The priority programming loop below is bounded by pdata->tx_queues_to_use,
> so the TCs with offset >= tx_queues_to_use never get a PSTQX/PSTC mask
> while the offload still returns success, and netif_setup_tc() remaps those
> priorities to TC0 at the next open.
> 
> The same check also rejects a sparse but otherwise legal map, e.g.
> num_tc 2 with "queues 1@0 1@2".  Is that intentional?

As the previous ones, even this issue is reachable even without this patch
applied. I think we should address it in net with a proper fixes tag.

> 
> > @@ -1315,14 +1358,44 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
> >  	if (err)
> >  		goto error_reset_tc;
> >  
> > +	priv->xmit_qdisc.algo = MTL_TX_ALGORITHM_SP;
> >  	err = stmmac_fpe_map_preemption_class(priv, ndev, extack,
> >  					      mqprio->preemptible_tcs);
> >  	if (err)
> > -		goto error_reset_num_tx_queues;
> > +		goto error_reset_xmit_algo;
> > +
> > +	for (i = 0; i < pdata->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);
> 
> [Severity: Medium]
> Is the last argument the right index on XGMAC?
> 
> The mask computed here belongs to traffic class j, but the index passed to
> stmmac_tx_queue_prio() is the queue index i.  For XGMAC the callback is:
> 
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:dwxgmac2_tx_queue_prio() {
> 	reg = (queue < 4) ? XGMAC_TC_PRTY_MAP0 : XGMAC_TC_PRTY_MAP1;
> 	...
> 	value &= ~XGMAC_PSTC(queue);
> 	value |= (prio << XGMAC_PSTC_SHIFT(queue)) & XGMAC_PSTC(queue);
> }
> 
> XGMAC_PSTC() selects a traffic-class field in MTL_TC_Prty_Map0/1, while the
> queue to TC association is programmed separately in XGMAC_Q2TCMAP:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c:dwxgmac3_fpe_map_preemption_class() {
> 	for (u32 i = 0; i < count; i++) {
> 		val = readl(priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(offset + i));
> 		writel(u32_replace_bits(val, tc, XGMAC_Q2TCMAP), ...);
> }
> 
> Both dwxgmac210 and dwxlgmac2 use dwmac510_tc_ops, and a permuted map such
> as "mqprio num_tc 2 map ... queues 1@1 1@0 hw 1" passes both
> intervals_overlap() in the core and the new offset < num_tc check.  In that
> case TC0's mask would be written into the PSTC field of TC1 and vice versa.
> Should the XGMAC path program PSTC using the TC index, or should
> non-identity offsets be rejected?

I think the issue is real since in dwmac4_tx_queue_priority(, queue) queue
index is the real hw queue, while in dwxgmac2_tx_queue_prio(, queue) queue
index is actually a tc index (even if it called queue).
Moreover, the dts path just assumes the tc index is equal to queue index.
I guess we should just forbid this configuration in order to keep dts and tc
paths aligned. What do you think?

Regards,
Lorenzo

> 
> > +		priv->xmit_qdisc.prio[i] = prio;
> > +	}
> > +
> > +	stmmac_prog_mtl_tx_algorithms(priv, priv->hw, MTL_TX_ALGORITHM_SP);
> >  
> >  	return 0;
> >  
> > -error_reset_num_tx_queues:
> > +error_reset_xmit_algo:
> > +	priv->xmit_qdisc.algo = priv->plat->tx_sched_algorithm;
> >  	if (netif_set_real_num_tx_queues(ndev, ndev_num_tx_queues))
> >  		netdev_warn(ndev, "Failed to restore %u TX queues\n",
> >  			    ndev_num_tx_queues);
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-stmmac-mqprio-prio-offload-v1-1-5328157fcb58%40oss.qualcomm.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next] net: stmmac: add tc-mqprio qdisc offload

[Jakub Kicinski]

On Wed, 23 Sep 2026 18:54:40 +0200 Lorenzo Bianconi wrote:
> I guess we should just forbid this configuration in order to keep dts and tc
> paths aligned. What do you think?

Are you asking the bot? :)
IMO rejecting ambiguous||contradictory configs is a good idea.