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

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

[Lorenzo Bianconi]

In tc_setup_dwmac510_mqprio(), if stmmac_fpe_map_preemption_class()
fails after the number of real TX queues has been set to
num_tx_queues, the error path calls stmmac_reset_tc_mqprio(), which resets
the number of real TX queues to priv->plat->tx_queues_to_use
(the maximum the platform supports).
This overwrites the value that was active before the mqprio offload was
attempted, which may have been lower than the platform maximum (for
example after a previous mqprio configuration reduced the queue count).

Save ndev->real_num_tx_queues before lowering it and restore it if the
FPE preemption-class mapping fails. Drop the use of
stmmac_reset_tc_mqprio() from the error path: the queue count is now
preserved and the TC-to-queue and priority-to-TC mappings are restored
to the previously saved values.
Introduce stmmac_set_ndev_tcs utility routine.

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/hwif.h      |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 80 ++++++++++++++++++-------
 2 files changed, 61 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 04dafec021b4..9314bcb85c22 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -494,7 +494,7 @@ struct stmmac_ops {
 #define stmmac_set_arp_offload(__priv, __args...) \
 	stmmac_do_void_callback(__priv, mac, set_arp_offload, __args)
 #define stmmac_fpe_map_preemption_class(__priv, __args...) \
-	stmmac_do_void_callback(__priv, mac, fpe_map_preemption_class, __args)
+	stmmac_do_callback(__priv, mac, fpe_map_preemption_class, __args)
 
 /* PTP and HW Timer helpers */
 struct stmmac_hwtimestamp {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 14cabe76e53e..c99d9b7e26dc 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 void stmmac_reset_tc_mqprio(struct net_device *ndev,
 				   struct netlink_ext_ack *extack)
 {
@@ -1250,45 +1274,61 @@ static void 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) {
 		stmmac_reset_tc_mqprio(ndev, extack);
 		return 0;
 	}
 
-	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;
 }

---
base-commit: 2f38e26a5741abdf152c1b56b22144a06d30fd66
change-id: 20260903-stmmac-tc_setup_dwmac510_mqprio-error-path-8d472a43d3f2

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

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

[netdev-bot+sashiko]

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

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

- [High] The patch silently changes the semantics of the shared dispatch
  macro stmmac_fpe_map_preemption_class() in hwif.h from…
- [Medium] The TC state that the new error path saves and restores
  (num_tc via stmmac_set_ndev_tcs(), plus prio_tc_map) is immediately…

--

net: stmmac: preserve real_num_tx_queues on mqprio setup failure

This reworks tc_setup_dwmac510_mqprio() so that the previous num_tc,
tc_to_txq and prio_tc_map values are saved and restored on failure instead
of calling stmmac_reset_tc_mqprio(), and adds a stmmac_set_ndev_tcs()
helper.  It also switches the stmmac_fpe_map_preemption_class() dispatch
macro from stmmac_do_void_callback() to stmmac_do_callback().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> index 04dafec021b4..9314bcb85c22 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> @@ -494,7 +494,7 @@ struct stmmac_ops {
>  #define stmmac_set_arp_offload(__priv, __args...) \
>  	stmmac_do_void_callback(__priv, mac, set_arp_offload, __args)
>  #define stmmac_fpe_map_preemption_class(__priv, __args...) \
> -	stmmac_do_void_callback(__priv, mac, fpe_map_preemption_class, __args)
> +	stmmac_do_callback(__priv, mac, fpe_map_preemption_class, __args)

[Severity: High]
Should this macro conversion be called out in the commit message, and
possibly split into its own patch?  It changes the semantics of a shared
dispatch macro for all of its users, not only for the mqprio path.

stmmac_do_void_callback() forces the result to 0 whenever the op pointer is
populated, so before this patch the FPE callback return value was discarded
everywhere.  Two things seem to follow from that.

First, does the bug premise in the commit message hold for the tree the
Fixes: tag points at?  The message says:

  "In tc_setup_dwmac510_mqprio(), if stmmac_fpe_map_preemption_class()
   fails after the number of real TX queues has been set to
   num_tx_queues, the error path calls stmmac_reset_tc_mqprio()"

tc_setup_dwmac510_mqprio() is only reachable through dwmac510_tc_ops, and
every hwif.c entry that installs dwmac510_tc_ops pairs it with dwmac410_ops,
dwmac510_ops, dwxgmac210_ops or dwxlgmac2_ops, all of which populate
.fpe_map_preemption_class.  With the old void dispatch, that call could
therefore only evaluate to 0, so the described FPE failure path looks
unreachable before this patch.  The latent problem actually being fixed is
the swallowed callback error, which the message does not mention.

Second, the same macro is used by tc_taprio_configure():

drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c:tc_taprio_configure() {
	...
	ret = stmmac_fpe_map_preemption_class(priv, priv->dev, extack,
					      qopt->mqprio.preemptible_tcs);
	if (ret)
		goto disable;
	...
}

That "if (ret)" was dead code and becomes live here.  In
dwmac5_fpe_map_preemption_class():

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

tx_sched_algorithm defaults to MTL_TX_ALGORITHM_SP for DT platforms in
stmmac_platform.c, and the default per-queue weights differ (0x10 + queue),
so a taprio or mqprio configuration with a preemptible TC owning more than
one TXQ now gets -EINVAL where it previously returned success with the
preemption-class register left unprogrammed.

For taprio, the new error also takes the disable label, which clears
priv->est->enable and reprograms EST, so the schedule that was just
installed is torn down.  Is that intended for configurations that used to be
accepted, for example:

  tc qdisc replace dev eth0 root taprio ... queues 2@0 2@2 ... fp E P

Also, the two remaining users that still ignore the now meaningful return
value, the taprio disable path and stmmac_reset_tc_mqprio(), were not
updated.  Should they be?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 14cabe76e53e..c99d9b7e26dc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c

[ ... ]

> @@ -1250,45 +1274,61 @@ static void stmmac_reset_tc_mqprio(struct net_device *ndev,
>  	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;
>  }

[Severity: Medium]
Can this restore of num_tc, tc_to_txq and prio_tc_map ever be observed?

tc_setup_dwmac510_mqprio() with num_tc != 0 is only reached from
mqprio_enable_offload() inside mqprio_init(), and the error is returned
before hw_offload is recorded:

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

qdisc_create() then always runs ops->destroy() on init failure:

net/sched/sch_api.c:qdisc_create() {
	if (ops->init) {
		err = ops->init(sch, tca[TCA_OPTIONS], extack);
		if (err != 0)
			goto err_out4;
	}
...
err_out4:
	if (ops->destroy)
		ops->destroy(sch);
}

and with hw_offload still 0 the else branch is taken:

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

So dev->num_tc goes back to 0 immediately after the driver restored it, and
skb_tx_hash() ignores tc_to_txq and prio_tc_map when num_tc is 0.  On a
failing "tc qdisc replace" over an already offloaded mqprio, does the still
grafted old qdisc lose its priority classification anyway?

If so, should the commit message text

  "the TC-to-queue and priority-to-TC mappings are restored to the
   previously saved values"

be adjusted, since only the real_num_tx_queues part of the rollback appears
to survive?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-stmmac-tc_setup_dwmac510_mqprio-error-path-v1-1-eb171bdd340a%40oss.qualcomm.com

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

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 1 · Low: 0
> 
> - [High] The patch silently changes the semantics of the shared dispatch
>   macro stmmac_fpe_map_preemption_class() in hwif.h from…
> - [Medium] The TC state that the new error path saves and restores
>   (num_tc via stmmac_set_ndev_tcs(), plus prio_tc_map) is immediately…
> 
> --
> 
> net: stmmac: preserve real_num_tx_queues on mqprio setup failure
> 
> This reworks tc_setup_dwmac510_mqprio() so that the previous num_tc,
> tc_to_txq and prio_tc_map values are saved and restored on failure instead
> of calling stmmac_reset_tc_mqprio(), and adds a stmmac_set_ndev_tcs()
> helper.  It also switches the stmmac_fpe_map_preemption_class() dispatch
> macro from stmmac_do_void_callback() to stmmac_do_callback().
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> > index 04dafec021b4..9314bcb85c22 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
> > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
> > @@ -494,7 +494,7 @@ struct stmmac_ops {
> >  #define stmmac_set_arp_offload(__priv, __args...) \
> >  	stmmac_do_void_callback(__priv, mac, set_arp_offload, __args)
> >  #define stmmac_fpe_map_preemption_class(__priv, __args...) \
> > -	stmmac_do_void_callback(__priv, mac, fpe_map_preemption_class, __args)
> > +	stmmac_do_callback(__priv, mac, fpe_map_preemption_class, __args)
> 
> [Severity: High]
> Should this macro conversion be called out in the commit message, and
> possibly split into its own patch?  It changes the semantics of a shared
> dispatch macro for all of its users, not only for the mqprio path.

I do not think so since this patch adds the only place where the
stmmac_fpe_map_preemption_class() return value is honored, all the other
stmmac_fpe_map_preemption_class() calls discard the error value.


[...]

> 
> Second, the same macro is used by tc_taprio_configure():
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c:tc_taprio_configure() {
> 	...
> 	ret = stmmac_fpe_map_preemption_class(priv, priv->dev, extack,
> 					      qopt->mqprio.preemptible_tcs);
> 	if (ret)
> 		goto disable;
> 	...
> }
> 
> That "if (ret)" was dead code and becomes live here.  In
> dwmac5_fpe_map_preemption_class():
> 
> 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;
> 		}
> 	...
> }
> 
> tx_sched_algorithm defaults to MTL_TX_ALGORITHM_SP for DT platforms in
> stmmac_platform.c, and the default per-queue weights differ (0x10 + queue),
> so a taprio or mqprio configuration with a preemptible TC owning more than
> one TXQ now gets -EINVAL where it previously returned success with the
> preemption-class register left unprogrammed.

I think it is better to return an error if the hw is not programmed properly
since otherwise the NIC is silently not working fine. The user can check and
understand what is the wrong configuration.

> 
> For taprio, the new error also takes the disable label, which clears
> priv->est->enable and reprograms EST, so the schedule that was just
> installed is torn down.  Is that intended for configurations that used to be
> accepted, for example:
> 
>   tc qdisc replace dev eth0 root taprio ... queues 2@0 2@2 ... fp E P
> 
> Also, the two remaining users that still ignore the now meaningful return
> value, the taprio disable path and stmmac_reset_tc_mqprio(), were not
> updated.  Should they be?
> 

[...]

> 
> [Severity: Medium]
> Can this restore of num_tc, tc_to_txq and prio_tc_map ever be observed?
> 
> tc_setup_dwmac510_mqprio() with num_tc != 0 is only reached from
> mqprio_enable_offload() inside mqprio_init(), and the error is returned
> before hw_offload is recorded:
> 
> 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;
> }
> 
> qdisc_create() then always runs ops->destroy() on init failure:
> 
> net/sched/sch_api.c:qdisc_create() {
> 	if (ops->init) {
> 		err = ops->init(sch, tca[TCA_OPTIONS], extack);
> 		if (err != 0)
> 			goto err_out4;
> 	}
> ...
> err_out4:
> 	if (ops->destroy)
> 		ops->destroy(sch);
> }
> 
> and with hw_offload still 0 the else branch is taken:
> 
> 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);
> }
> 
> So dev->num_tc goes back to 0 immediately after the driver restored it, and
> skb_tx_hash() ignores tc_to_txq and prio_tc_map when num_tc is 0.  On a
> failing "tc qdisc replace" over an already offloaded mqprio, does the still
> grafted old qdisc lose its priority classification anyway?

I think in a error condition, the driver is supposed to restore the previous
overwritten values, then it is up to qdisc layer to manage the error condition.

Regards,
Lorenzo

> 
> If so, should the commit message text
> 
>   "the TC-to-queue and priority-to-TC mappings are restored to the
>    previously saved values"
> 
> be adjusted, since only the real_num_tx_queues part of the rollback appears
> to survive?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-stmmac-tc_setup_dwmac510_mqprio-error-path-v1-1-eb171bdd340a%40oss.qualcomm.com

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

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

[Jakub Kicinski]

On Tue, 8 Sep 2026 12:12:43 +0200 Lorenzo Bianconi wrote:
> > So dev->num_tc goes back to 0 immediately after the driver restored it, and
> > skb_tx_hash() ignores tc_to_txq and prio_tc_map when num_tc is 0.  On a
> > failing "tc qdisc replace" over an already offloaded mqprio, does the still
> > grafted old qdisc lose its priority classification anyway?  
> 
> I think in a error condition, the driver is supposed to restore the previous
> overwritten values, then it is up to qdisc layer to manage the error condition.

I'd translate all the AI complaints to a basic confusion about what
exactly is the problem you're fixing and how it can be triggered.
A better commit message may be sufficient. An example of sequence of
tc commands which used to lead to loss of config / broken device and
now does not would go a long way.

> > For taprio, the new error also takes the disable label, which clears
> > priv->est->enable and reprograms EST, so the schedule that was just
> > installed is torn down.  Is that intended for configurations that used to be
> > accepted, for example:
> > 
> >   tc qdisc replace dev eth0 root taprio ... queues 2@0 2@2 ... fp E P
> > 
> > Also, the two remaining users that still ignore the now meaningful return
> > value, the taprio disable path and stmmac_reset_tc_mqprio(), were not
> > updated.  Should they be?

This sounds relevant as well.