Re: [net,1/2] net: stmmac: preserve real_num_tx_queues on mqprio setup failure

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

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

- [High] The error path this patch is built around cannot be taken:
  `stmmac_fpe_map_preemption_class()` is dispatched through…
- [High] The new helper `stmmac_set_ndev_tcs()`
  (drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c:1240) is a verbatim…
- [Medium] The commit message justifies dropping stmmac_reset_tc_mqprio()
  from the error path with 'the TC-to-queue mapping is restored to the…

--

net: stmmac: preserve real_num_tx_queues on mqprio setup failure

This reworks tc_setup_dwmac510_mqprio() so that the previous
real_num_tx_queues and TC-to-queue mapping are snapshotted and restored
if stmmac_fpe_map_preemption_class() fails, instead of falling back to
stmmac_reset_tc_mqprio().  A new stmmac_set_ndev_tcs() helper is added to
program and restore the netdev TC state.

A few questions below.

[Severity: High]
Is the "ntc == 1" early return correct for stmmac?

This shortcut matches mlx5e_netdev_set_tcs() in
drivers/net/ethernet/mellanox/mlx5/core/en_main.c, where one TC means "no
TC differentiation" and mlx5 keeps its own queue/TC bookkeeping.  In
stmmac the netdev TC state is the only input to the FPE programming
callbacks.

For a legal single-TC offload request, for example

  tc qdisc replace dev eth0 root mqprio num_tc 1 map 0 0 0 0 0 0 0 0 \
      queues 4@0 fp P hw 1

mqprio_validate_qopt() accepts num_tc 1 and mqprio_fp_to_offload() sets
mqprio->preemptible_tcs = BIT(0).  stmmac_set_ndev_tcs(ndev, 1, tc_to_txq)
then leaves dev->num_tc == 0 and tc_to_txq[] zeroed, and
tc_setup_dwmac510_mqprio() still returns 0.

Before this patch the same request ran netdev_set_num_tc(ndev, 1) followed
by netdev_set_tc_queue(ndev, 0, count, offset).

Does that change the hardware programming?  dwmac5_fpe_map_preemption_class()
derives everything from netdev_get_num_tc():

drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c:dwmac5_fpe_map_preemption_class() {
	int num_tc = netdev_get_num_tc(ndev);
	...
	for (u32 tc = 0; tc < num_tc; tc++) {
		...
		if (pclass & BIT(tc))
			preemptible_txqs |= GENMASK(offset + count - 1, offset);
		...
	}
update_mapping:
	val = readl(priv->ioaddr + GMAC5_MTL_FPE_CTRL_STS);
	writel(u32_replace_bits(val, preemptible_txqs, FPE_MTL_PREEMPTION_CLASS),
	       priv->ioaddr + GMAC5_MTL_FPE_CTRL_STS);
}

With num_tc == 0 the loop body never runs, so FPE_MTL_PREEMPTION_CLASS is
written as 0 and the TC0 the user marked preemptible is never armed.  The
one-to-many SP-algorithm and equal-weight checks in the same loop are
skipped as well.

On XGMAC the effect is the opposite direction:

drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c:dwxgmac3_fpe_map_preemption_class() {
	if (!num_tc) {
		/* Restore default TC:Queue mapping */
		for (u32 i = 0; i < priv->plat->tx_queues_to_use; i++) {
			val = readl(priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(i));
			writel(u32_replace_bits(val, i, XGMAC_Q2TCMAP),
			       priv->ioaddr + XGMAC_MTL_TXQ_OPMODE(i));
		}
	}

so the identity queue-to-TC map is reprogrammed even though the offload of
N queues into TC0 was reported as successful.  The netdev also ends up
reporting num_tc 0 to mqprio_dump() while the mqprio qdisc is installed.

+
+	err = netdev_set_num_tc(ndev, ntc);
+	if (err)
+		return err;
+
+	for (i = 0; i < ntc; i++) {
+		u16 count, offset;
+
+		count = tc_to_txq[i].count;
+		offset = tc_to_txq[i].offset;
+		netdev_set_tc_queue(ndev, i, count, offset);
+	}
+
+	return 0;
+}
+
 static void stmmac_reset_tc_mqprio(struct net_device *ndev,
 				   struct netlink_ext_ack *extack)
 {

[ ... ]

[ ... ]

+	/* save current tc values for reset */
+	ndev_ntc = netdev_get_num_tc(ndev) ? : 1;
+	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);

[Severity: Medium]
The snapshot covers num_tc and tc_to_txq[], but not ndev->prio_tc_map[].
stmmac_set_ndev_tcs() starts with netdev_reset_tc(), which clears all
three:

net/core/dev.c:netdev_reset_tc() {
	WRITE_ONCE(dev->num_tc, 0);
	for (i = 0; i < TC_MAX_QUEUE; i++)
		WRITE_ONCE(dev->tc_to_txq[i].combined, 0);
	for (i = 0; i <= TC_BITMASK; i++)
		WRITE_ONCE(dev->prio_tc_map[i], 0);
}

The core only repopulates prio_tc_map after a successful driver callback:

net/sched/sch_mqprio.c:mqprio_init() {
	...
	/* Always use supplied priority mappings */
	for (i = 0; i < TC_BITMASK + 1; i++)
		netdev_set_prio_tc_map(dev, i, qopt->prio_tc_map[i]);
}

so on the error path the previously active priority-to-TC map is lost.
Should prio_tc_map[] be saved and restored too?

Also, the "? : 1" encoding collapses an existing num_tc == 1 into the
helper's reset case, so a device that legitimately had one TC ends up with
num_tc == 0 after a failed attempt.

Separately, the commit message says "the TC-to-queue mapping is restored to
the previously saved values".  Is the num_tc restore observable in the
scenario the message describes (a new mqprio configuration over a working
one)?  qdisc_create() calls ops->destroy() even when ops->init() failed,
and mqprio_destroy() takes the non-offload branch because priv->hw_offload
was never set:

net/sched/sch_mqprio.c:mqprio_destroy() {
	if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc)
		mqprio_disable_offload(sch);
	else
		netdev_set_num_tc(dev, 0);
}

which zeroes dev->num_tc again right after the driver restored it.

+
+	for (i = 0; i < qopt->num_tc; i++) {
+		tc_to_txq[i] = (struct netdev_tc_txq) {
+			.count = qopt->count[i],
+			.offset = qopt->offset[i],
+		};
+		num_tx_queues += qopt->count[i];
 	}
 
-	err = netif_set_real_num_tx_queues(ndev, num_stack_tx_queues);
+	err = stmmac_set_ndev_tcs(ndev, qopt->num_tc, tc_to_txq);
+	if (err)
+		goto error_reset_tc;
+
+	ndev_num_tx_queues = ndev->real_num_tx_queues;
+	err = netif_set_real_num_tx_queues(ndev, num_tx_queues);
 	if (err)
-		goto err_reset_tc;
+		goto error_reset_tc;
 
 	err = stmmac_fpe_map_preemption_class(priv, ndev, extack,
 					      mqprio->preemptible_tcs);
 	if (err)
-		goto err_reset_tc;
+		goto error_reset_num_tx_queues;
 
 	return 0;
 
-err_reset_tc:
-	stmmac_reset_tc_mqprio(ndev, extack);
+error_reset_num_tx_queues:
+	netif_set_real_num_tx_queues(ndev, ndev_num_tx_queues);
+error_reset_tc:
+	stmmac_set_ndev_tcs(ndev, ndev_ntc, ndev_tc_to_txq);
 
 	return err;
 }

[Severity: High]
Can err here ever be non-zero on a MAC that reaches this function?
stmmac_fpe_map_preemption_class() is dispatched through the void wrapper:

drivers/net/ethernet/stmicro/stmmac/hwif.h {
#define stmmac_do_void_callback(__priv, __module, __cname,  __arg0, __args...) \
({ \
	int __result = -EINVAL; \
	if ((__priv)->hw->__module && (__priv)->hw->__module->__cname) { \
		(__priv)->hw->__module->__cname((__arg0), ##__args); \
		__result = 0; \
	} \
	__result; \
})
...
#define stmmac_fpe_map_preemption_class(__priv, __args...) \
	stmmac_do_void_callback(__priv, mac, fpe_map_preemption_class, __args)
}

The op is declared as returning int in struct stmmac_ops, but the wrapper
calls it as a bare statement and forces 0 whenever the pointer is set.
Every ops table paired with .tc = &dwmac510_tc_ops in hwif.c (dwmac410_ops,
dwmac510_ops, dwxgmac210_ops, dwxlgmac2_ops) populates
.fpe_map_preemption_class, so err is always 0 and the new
error_reset_num_tx_queues label looks unreachable.

If so, does that mean the failure described in the commit message,

  "if stmmac_fpe_map_preemption_class() fails after the number of real TX
   queues has been set to num_tx_queues"

cannot currently happen, and the Fixes: 195e4f409a40 tag is pointing at a
path that is not taken?

The same wrapper also drops the real rejections in
dwmac5_fpe_map_preemption_class():

drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c:dwmac5_fpe_map_preemption_class() {
		if (priv->plat->tx_sched_algorithm == MTL_TX_ALGORITHM_SP) {
			NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
			return -EINVAL;
		}
		...
			if (priv->plat->tx_queues_cfg[offset + i].weight !=
			    queue_weight) {
				NL_SET_ERR_MSG_FMT_MOD(extack, WEIGHT_ERR_MSG,
						       queue_weight, tc);
				return -EINVAL;
			}
}

Both returns happen before the GMAC5_MTL_FPE_CTRL_STS write, so the
preemption-class register keeps its previous value while the qdisc layer is
told the offload succeeded.  Would switching the wrapper to
stmmac_do_callback() be the prerequisite change here, so that this new
error path can actually run?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260829-stmmac-fix-graft-overwrite-v1-0-a4f84c4c264e%40oss.qualcomm.com