Re: [PATCH net v2 2/2] net: stmmac: preserve real_num_tx_queues on mqprio setup failure

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The reproducer quoted in the commit message cannot demonstrate
  the bug being fixed.
- [Medium] The new error path no longer reprograms the MAC's FPE
  preemption-class register.
- [Medium] Spurious kernel warning triggered during mqprio teardown path
  in `tc_setup_dwmac510_mqprio`
- [Low] The driver-side restore of dev->num_tc / tc_to_txq[] /
  prio_tc_map[] on the FPE-failure path is immediately overridden in…
- [Low] The newly added rollback in tc_setup_dwmac510_mqprio() discards
  the return value of netif_set_real_num_tx_queues(ndev,…

--

net: stmmac: preserve real_num_tx_queues on mqprio setup failure

This patch saves ndev->real_num_tx_queues along with the TC-to-queue and
priority-to-TC mappings before an mqprio offload is applied, and restores
them if stmmac_fpe_map_preemption_class() fails, instead of calling
stmmac_reset_tc_mqprio() which reset the queue count to the platform
maximum.

Some questions about the commit message and the new rollback path below.

>     The issue can be triggered using the following configuration:
>     
>       # First mqprio config lowers the hw queue count.
>       $tc qdisc add dev eth0 root handle 1: mqprio queues 4@0 4@4

[Severity: Medium]
Does this reproducer actually show the difference between the old and new
error paths?

"queues 4@0 4@4" totals 8 TX queues, and mqprio only accepts it if
real_num_tx_queues is already >= 8:

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 opts into that check via caps->validate_queue_counts = true in
tc_query_caps(), allocates MTL_MAX_TX_QUEUES (8) TX queues, and
stmmac_hw_setup() sets:

	netif_set_real_num_tx_queues(dev, priv->plat->tx_queues_to_use);

So the first command is only accepted when tx_queues_to_use == 8, i.e.
real_num_tx_queues is already at the platform maximum, and the comment
"First mqprio config lowers the hw queue count" does not match what
happens.

With real_num_tx_queues == 8, the old path
stmmac_reset_tc_mqprio() -> netif_set_real_num_tx_queues(ndev,
priv->plat->tx_queues_to_use) restores exactly the same 8 that the new
code restores from ndev_num_tx_queues, so the two behave identically for
the quoted commands.

Would a first configuration that totals fewer queues than
tx_queues_to_use, for example "queues 2@0 2@2" on an 8-queue MAC followed
by the failing "queues 2@0 2@2 fp E P", be a better reproducer?

>     Note that a failed setup makes the qdisc layer run mqprio_destroy() on
>     the new qdisc, which resets dev->num_tc to 0; that is qdisc-layer handling,
>     out of the driver's control, and the driver-side restore remains correct
>     for the still-active previous configuration.

[Severity: Low]
Is "the driver-side restore remains correct for the still-active previous
configuration" accurate for num_tc and the two map arrays?

mqprio_enable_offload() only assigns priv->hw_offload after ndo_setup_tc()
succeeds:

net/sched/sch_mqprio.c:mqprio_enable_offload() {
	err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQPRIO, &mqprio);
	if (err)
		return err;

	priv->hw_offload = mqprio.qopt.hw;
}

so on the failure path mqprio_destroy() takes the else branch:

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);
}

netdev_set_num_tc() writes num_tc only, so the netdev ends up with num_tc
== 0 while the driver-restored tc_to_txq[] and prio_tc_map[] stay
populated, a combination netdev_reset_tc() never produces on its own.

Since skb_tx_hash(), netif_setup_tc(), mqprio_qopt_reconstruct() and XPS
all gate on num_tc, is the newly added save/restore of num_tc and of the
two 16-entry maps doing anything observable, and is priority
classification for the still-installed previous qdisc actually preserved?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 5398616fcdfe..7d73b0c16f50 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c

[ ... ]

> @@ -1251,43 +1275,59 @@ static int stmmac_reset_tc_mqprio(struct net_device *ndev,

[ ... ]

> +	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);
> +	for (i = 0; i < ARRAY_SIZE(ndev_prio_tc_map); i++)
> +		netdev_set_prio_tc_map(ndev, i, ndev_prio_tc_map[i]);

[Severity: Medium]
Is the rollback order at these two labels right when the new mqprio
configuration asks for more TX queues than the one it replaces?

When error_reset_num_tx_queues is reached, the netdev still carries
the mapping that stmmac_set_ndev_tcs(ndev, qopt->num_tc, tc_to_txq)
just installed: dev->num_tc is qopt->num_tc and tc_to_txq[] describes
num_tx_queues queues. The restore call then shrinks
real_num_tx_queues back to ndev_num_tx_queues, and because
dev->num_tc is non-zero netif_set_real_num_tx_queues() goes through
netif_setup_tc():

net/core/dev.c:netif_setup_tc() {
	struct netdev_tc_txq *tc = &dev->tc_to_txq[0];

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

	for (i = 1; i < TC_BITMASK + 1; i++) {
		...
		if (tc->offset + tc->count > txq) {
			netdev_warn(dev, "... Setting map to 0\n");
			netdev_set_prio_tc_map(dev, i, 0);
		}
	}
}

So for a sequence such as "queues 2@0 2@2" followed by a failing
"queues 4@0 4@4 fp E P", the driver's own rollback makes the core
print "Priority traffic classification disabled!" (and possibly the
per-priority warnings), zero dev->num_tc and rewrite prio_tc_map[]
entries, purely as a side effect of the intermediate state. The
following stmmac_set_ndev_tcs(ndev, ndev_ntc, ndev_tc_to_txq) and
the netdev_set_prio_tc_map() loop do repair all of that, so the
end state looks correct, but users see a scary warning for an
internal undo step and the tc mappings are transiently invalid.

Would reversing the two rollback steps, restoring ndev_ntc /
ndev_tc_to_txq / ndev_prio_tc_map first and only then calling
netif_set_real_num_tx_queues(ndev, ndev_num_tx_queues), avoid this?
The saved mapping by construction fits ndev_num_tx_queues, so
netif_setup_tc() should have nothing to complain about. If there is
a reason the queue count has to be restored first, could that be
spelled out in a comment?

[Severity: Low]
Should the return value of the restoring netif_set_real_num_tx_queues() be
checked here?

On this path the call always grows the queue count, because
mqprio_validate_queue_counts() already ensured num_tx_queues <=
ndev_num_tx_queues, and growing can fail:

net/core/dev.c:netif_set_real_num_tx_queues() {
	...
		rc = netdev_queue_update_kobjects(dev, dev->real_num_tx_queues,
						  txq);
		if (rc)
			return rc;
	...
}

net/core/net-sysfs.c:netdev_queue_update_kobjects() {
	for (i = old_num; i < new_num; i++) {
		error = netdev_queue_add_kobject(dev, i);
		if (error) {
			new_num = old_num;
			break;
		}
	}
}

The early return happens before dev->real_num_tx_queues is assigned, so on
-ENOMEM the device keeps the lower queue count while the following
stmmac_set_ndev_tcs() reinstates a tc_to_txq[] whose offsets and counts
describe queues beyond real_num_tx_queues. The function still returns only
the original FPE error, so nothing above sees the partial restore.

[Severity: Medium]
Does this error path need to reprogram the FPE preemption-class register?

The previous code reached the disarm through:

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c:stmmac_reset_tc_mqprio() {
	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);
}

dwmac5_fpe_map_preemption_class() does all of its validation before its
single read-modify-write:

drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c {
		if (priv->plat->tx_sched_algorithm == MTL_TX_ALGORITHM_SP) {
			NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
			return -EINVAL;
		}
	...
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);
}

so on the -EINVAL path GMAC5_MTL_FPE_CTRL_STS still holds the mask
programmed by the previous successful configuration.

The new error_reset_num_tx_queues/error_reset_tc labels touch no MAC
register, and the qdisc layer then zeroes dev->num_tc via
mqprio_destroy() -> netdev_set_num_tc(dev, 0). Is the hardware then left
with a per-TXQ preemptible mask armed while the driver's only description
of which TXQs belong to which TC says there are no traffic classes, so
express-intended frames can land on TXQs the MAC merge layer will
fragment?

The driver recomputes the mask only from netdev_get_num_tc() and
tc_to_txq[] on later calls, so it appears the stale mask stays until the
previous qdisc is deleted or replaced. Would it be worth mentioning this
hardware-state change in the commit message?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-stmmac-tc_setup_dwmac510_mqprio-error-path-v2-0-8158b8333b3d%40oss.qualcomm.com