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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

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

> 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:

ack, I will fix it in v3.

> 
> 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?

I think mqprio replace error path is broken since the user would assume if
'qdisc replace' fails, the old qdisc will be properly working, right? So I
guess this issue should be fixed in sch_mqprio.

> 
> > 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?

Can this really happen? mqprio_validate_queue_counts() should avoid that.

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

ack, I will fix it in v3.

> 
> [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?

as pointed out above, I guess this is a problem in sch_mqprio error path, and
it should be fixed there.

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

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