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

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

[netdev-bot+sashiko]

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

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

[Lorenzo Bianconi]

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

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

[Lorenzo Bianconi]

With the FPE preemption-class mapping error now propagated from
stmmac_fpe_map_preemption_class(), tc_setup_dwmac510_mqprio() can fail
on the mapping step. The error path used to call stmmac_reset_tc_mqprio(),
which resets the number of real TX queues to priv->plat->tx_queues_to_use
(the platform maximum), overwriting the value that was active before the
offload was attempted (for example a lower count left over from a previous
mqprio configuration).

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

  # Replace mqprio configuration with a second one that fails FPE
  # preemption-class mapping. stmmac driver resets the real_num_tx_queues
  # to the platform maximum, losing the previous configuration.
  $tc qdisc replace dev eth0 root handle 2: mqprio queues 2@0 2@2 fp E P

Save ndev->real_num_tx_queues before lowering it and restore it,
together with the TC-to-queue and priority-to-TC mappings, when the FPE
preemption-class mapping fails, instead of resetting the queue count to
the platform maximum.

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.

Fixes: 195e4f409a40 ("net: stmmac: support fp parameter of tc-mqprio")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 80 ++++++++++++++++++-------
 1 file changed, 60 insertions(+), 20 deletions(-)

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
@@ -1237,6 +1237,30 @@ static int tc_query_caps(struct stmmac_priv *priv,
 	}
 }
 
+static int stmmac_set_ndev_tcs(struct net_device *ndev, u8 ntc,
+			       struct netdev_tc_txq *tc_to_txq)
+{
+	int i, err;
+
+	netdev_reset_tc(ndev);
+	if (!ntc)
+		return 0;
+
+	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 int stmmac_reset_tc_mqprio(struct net_device *ndev,
 				  struct netlink_ext_ack *extack)
 {
@@ -1251,43 +1275,59 @@ static int stmmac_reset_tc_mqprio(struct net_device *ndev,
 static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 				    struct tc_mqprio_qopt_offload *mqprio)
 {
+	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 netlink_ext_ack *extack = mqprio->extack;
 	struct tc_mqprio_qopt *qopt = &mqprio->qopt;
-	u32 offset, count, num_stack_tx_queues = 0;
 	struct net_device *ndev = priv->dev;
-	u32 num_tc = qopt->num_tc;
-	int err;
+	u8 ndev_prio_tc_map[TC_BITMASK + 1];
+	int i, err, ndev_ntc;
 
-	if (!num_tc)
+	if (!qopt->num_tc)
 		return stmmac_reset_tc_mqprio(ndev, extack);
 
-	err = netdev_set_num_tc(ndev, num_tc);
-	if (err)
-		return err;
-
-	for (u32 tc = 0; tc < num_tc; tc++) {
-		offset = qopt->offset[tc];
-		count = qopt->count[tc];
-		num_stack_tx_queues += count;
+	if (qopt->num_tc > ARRAY_SIZE(tc_to_txq))
+		return -EINVAL;
 
-		err = netdev_set_tc_queue(ndev, tc, count, offset);
-		if (err)
-			goto err_reset_tc;
+	/* 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++) {
+		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);
+	for (i = 0; i < ARRAY_SIZE(ndev_prio_tc_map); i++)
+		netdev_set_prio_tc_map(ndev, i, ndev_prio_tc_map[i]);
 
 	return err;
 }

-- 
2.55.0