[PATCH net 0/2] net: stmmac: fix qdisc configuration overwrite during qdisc_graft().

[PATCH net 0/2] net: stmmac: fix qdisc configuration overwrite during qdisc_graft().

[Lorenzo Bianconi]

---
Lorenzo Bianconi (2):
      net: stmmac: preserve real_num_tx_queues on mqprio setup failure
      net: stmmac: preserve FPE preemption class across qdisc replacement

 drivers/net/ethernet/stmicro/stmmac/stmmac.h    |  7 ++
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 97 +++++++++++++++++++------
 2 files changed, 80 insertions(+), 24 deletions(-)
---
base-commit: 2188569e7e1b0bc3f3b557dc97ab7a02befc11c8
change-id: 20260827-stmmac-fix-graft-overwrite-d3ec2e72d901

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

[PATCH net 1/2] 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
restored explicitly and the TC-to-queue mapping is 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/stmmac_tc.c | 75 ++++++++++++++++++-------
 1 file changed, 55 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 14cabe76e53e..be8ddda2aa76 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 == 1)
+		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,56 @@ 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;
+	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) ? : 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);
+
+	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;
 }

-- 
2.55.0

[PATCH net 2/2] net: stmmac: preserve FPE preemption class across qdisc replacement

[Lorenzo Bianconi]

The kernel replaces a root qdisc by creating the new qdisc first and
destroying the old one only afterwards (qdisc_create() then
qdisc_graft() -> notify_and_destroy()). For a cross-kind replace this
means the teardown of the old qdisc runs after the offload of the new
one.

This breaks the FPE preemption-class mapping when replacing between
qdiscs that both program it. For example replacing taprio with mqprio:

  1. the mqprio offload (REPLACE) programs FPE_MTL_PREEMPTION_CLASS
     with mqprio->preemptible_tcs
  2. the old taprio is then torn down; tc_taprio_configure() runs the
     TAPRIO_CMD_DESTROY path and resets the preemption class to 0

Track which root qdisc policy is currently offloaded in a new
priv->qdisc_type field and only clear the FPE preemption class during
teardown if the qdisc being destroyed actually owns it.

Fixes: 15d8a407a547 ("net: stmmac: support fp parameter of tc-taprio")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h    |  7 +++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 22 ++++++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 7582fca63741..957317ec1461 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -258,6 +258,12 @@ struct stmmac_msi {
 	char int_name_tx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 18];
 };
 
+enum stmmac_qdisc_type {
+	STMMAC_QDISC_NONE = 0,
+	STMMAC_QDISC_MQPRIO,
+	STMMAC_QDISC_TAPRIO,
+};
+
 struct stmmac_priv {
 	/* Frequently used values are kept adjacent for cache effect */
 	u32 tx_coal_frames[MTL_MAX_TX_QUEUES];
@@ -299,6 +305,7 @@ struct stmmac_priv {
 	/* Protect est parameters */
 	struct mutex est_lock;
 	struct stmmac_est *est;
+	enum stmmac_qdisc_type qdisc_type;
 	struct dma_features dma_cap;
 	struct stmmac_counters mmc;
 	int hw_cap_support;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index be8ddda2aa76..7fe587b47d52 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -1103,6 +1103,8 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 	if (ret)
 		goto disable;
 
+	priv->qdisc_type = STMMAC_QDISC_TAPRIO;
+
 	return 0;
 
 disable:
@@ -1120,7 +1122,11 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
 		mutex_unlock(&priv->est_lock);
 	}
 
-	stmmac_fpe_map_preemption_class(priv, priv->dev, extack, 0);
+	if (priv->qdisc_type == STMMAC_QDISC_TAPRIO &&
+	    qopt->cmd == TAPRIO_CMD_DESTROY) {
+		stmmac_fpe_map_preemption_class(priv, priv->dev, extack, 0);
+		priv->qdisc_type = STMMAC_QDISC_NONE;
+	}
 
 	return ret;
 }
@@ -1266,9 +1272,15 @@ static void stmmac_reset_tc_mqprio(struct net_device *ndev,
 {
 	struct stmmac_priv *priv = netdev_priv(ndev);
 
-	netdev_reset_tc(ndev);
-	netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use);
-	stmmac_fpe_map_preemption_class(priv, ndev, extack, 0);
+	if (priv->qdisc_type != STMMAC_QDISC_TAPRIO)
+		stmmac_fpe_map_preemption_class(priv, ndev, extack, 0);
+
+	if (priv->qdisc_type == STMMAC_QDISC_MQPRIO) {
+		netdev_reset_tc(ndev);
+		netif_set_real_num_tx_queues(ndev,
+					     priv->plat->tx_queues_to_use);
+		priv->qdisc_type = STMMAC_QDISC_NONE;
+	}
 }
 
 static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
@@ -1318,6 +1330,8 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
 	if (err)
 		goto error_reset_num_tx_queues;
 
+	priv->qdisc_type = STMMAC_QDISC_MQPRIO;
+
 	return 0;
 
 error_reset_num_tx_queues:

-- 
2.55.0

Re: [PATCH net 0/2] net: stmmac: fix qdisc configuration overwrite during qdisc_graft().

[Lorenzo Bianconi]

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

> 
> ---
> Lorenzo Bianconi (2):
>       net: stmmac: preserve real_num_tx_queues on mqprio setup failure
>       net: stmmac: preserve FPE preemption class across qdisc replacement
> 
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h    |  7 ++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 97 +++++++++++++++++++------
>  2 files changed, 80 insertions(+), 24 deletions(-)
> ---
> base-commit: 2188569e7e1b0bc3f3b557dc97ab7a02befc11c8
> change-id: 20260827-stmmac-fix-graft-overwrite-d3ec2e72d901

Please drop this version, I will address some sashiko's comments in v2.

Regards,
Lorenzo

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

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

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

[netdev-bot+sashiko]

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.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 14cabe76e53e..be8ddda2aa76 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 == 1)
> +		return 0;

[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)
>  {

[ ... ]

> @@ -1250,45 +1274,56 @@ 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)
>  {

[ ... ]

> +	/* 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

Re: [net,2/2] net: stmmac: preserve FPE preemption class across qdisc replacement

[netdev-bot+sashiko]

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

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

- [High] Incomplete fix: ownership of the FPE preemption class (and of
  the EST block) is tracked per qdisc *kind* (priv->qdisc_type) instead…
- [High] The patch also gates netdev_reset_tc() and the
  netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use)…
- [Medium] Reordering regression on XGMAC: in stmmac_reset_tc_mqprio()
  the stmmac_fpe_map_preemption_class(priv, ndev, extack, 0) call is now…
- [Medium] Lost error-path rollback in tc_taprio_configure(): the
  `disable:` tail now resets the FPE preemption class only when…

Pre-existing issues:
- [Medium] struct stmmac_ops::fpe_map_preemption_class is declared as
  returning int (drivers/net/ethernet/stmicro/stmmac/hwif.h:411-413),…

--

net: stmmac: preserve FPE preemption class across qdisc replacement

The patch adds a priv->qdisc_type field recording which root qdisc policy
(mqprio or taprio) is currently offloaded, and uses it to skip clearing the
FPE preemption class during teardown when the qdisc being destroyed is not
the one that programmed it. The mqprio reset path is additionally gated on
the same field.

A few questions about the new ownership tracking below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 7582fca63741..957317ec1461 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -258,6 +258,12 @@ struct stmmac_msi {
>  	char int_name_tx_irq[MTL_MAX_TX_QUEUES][IFNAMSIZ + 18];
>  };
>  
> +enum stmmac_qdisc_type {
> +	STMMAC_QDISC_NONE = 0,
> +	STMMAC_QDISC_MQPRIO,
> +	STMMAC_QDISC_TAPRIO,
> +};
> +

[Severity: High]
Is tracking the qdisc kind enough to identify the owner here?  The create-
then-destroy ordering the commit message describes also happens for a
same-kind replace when a different handle is used.  In
net/sched/sch_api.c:__tc_modify_qdisc():

	if (!q || !tcm->tcm_handle || q->handle != tcm->tcm_handle) {
		if (tcm->tcm_handle) {
			...
			q = qdisc_lookup(dev, tcm->tcm_handle);
			if (!q)
				goto create_n_graft;

so tc qdisc replace dev X root handle 8002: mqprio ... over an mqprio
installed at handle 8001: creates and offloads the new instance first and
destroys the old one afterwards, with both instances reporting the same
kind.

For mqprio -> mqprio, the new tc_setup_dwmac510_mqprio() stores
STMMAC_QDISC_MQPRIO, and the old instance's teardown then reaches
stmmac_reset_tc_mqprio() with qdisc_type == STMMAC_QDISC_MQPRIO, so it
clears the live qdisc's FPE class, calls netdev_reset_tc() and restores the
queue count, leaving qdisc_type == STMMAC_QDISC_NONE.

For taprio -> taprio, the new REPLACE stores STMMAC_QDISC_TAPRIO and the old
instance's TAPRIO_CMD_DESTROY satisfies the new guard, so
FPE_MTL_PREEMPTION_CLASS is cleared for the live qdisc.  The unconditional
disable: tail in tc_taprio_configure() also runs for that DESTROY:

	if (priv->est) {
		mutex_lock(&priv->est_lock);
		priv->est->enable = false;
		stmmac_est_configure(priv, priv, priv->est,
				     priv->plat->clk_ptp_rate);

which switches EST off (and zeroes the per-queue taprio stats) after the
replacing taprio has programmed its gate control list.  Would per-instance
identity (handle or a cookie/generation) be needed instead of the kind, and
should EST ownership be covered too?

>  struct stmmac_priv {
>  	/* Frequently used values are kept adjacent for cache effect */
>  	u32 tx_coal_frames[MTL_MAX_TX_QUEUES];

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index be8ddda2aa76..7fe587b47d52 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c

[ ... ]

> @@ -1120,7 +1122,11 @@ static int tc_taprio_configure(struct stmmac_priv *priv,
>  		mutex_unlock(&priv->est_lock);
>  	}
>  
> -	stmmac_fpe_map_preemption_class(priv, priv->dev, extack, 0);
> +	if (priv->qdisc_type == STMMAC_QDISC_TAPRIO &&
> +	    qopt->cmd == TAPRIO_CMD_DESTROY) {
> +		stmmac_fpe_map_preemption_class(priv, priv->dev, extack, 0);
> +		priv->qdisc_type = STMMAC_QDISC_NONE;
> +	}

[Severity: Medium]
Does the added cmd == TAPRIO_CMD_DESTROY term drop the rollback on the
error path?  The disable: label is also reached with cmd ==
TAPRIO_CMD_REPLACE:

	ret = stmmac_est_configure(priv, priv, priv->est,
				   priv->plat->clk_ptp_rate);
	mutex_unlock(&priv->est_lock);
	if (ret) {
		netdev_err(priv->dev, "failed to configure EST\n");
		goto disable;
	}

est_configure() in stmmac_est.c can return -EINVAL when ptp_rate is 0, and
-ETIMEDOUT via the readl_poll_timeout() inside est_write().  For a failing
REPLACE on an already offloaded taprio (qdisc_type == STMMAC_QDISC_TAPRIO),
the block now disables EST but leaves the previously programmed
FPE_MTL_PREEMPTION_CLASS mask in the hardware, while taprio_change() has
already installed the new num_tc/tc_to_txq mapping.  Before the patch that
mask was cleared unconditionally.

Would the ownership test alone (qdisc_type == STMMAC_QDISC_TAPRIO) be
sufficient for the case described in the commit message?

>  
>  	return ret;
>  }
> @@ -1266,9 +1272,15 @@ static void stmmac_reset_tc_mqprio(struct net_device *ndev,
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
>  
> -	netdev_reset_tc(ndev);
> -	netif_set_real_num_tx_queues(ndev, priv->plat->tx_queues_to_use);
> -	stmmac_fpe_map_preemption_class(priv, ndev, extack, 0);
> +	if (priv->qdisc_type != STMMAC_QDISC_TAPRIO)
> +		stmmac_fpe_map_preemption_class(priv, ndev, extack, 0);
> +
> +	if (priv->qdisc_type == STMMAC_QDISC_MQPRIO) {
> +		netdev_reset_tc(ndev);
> +		netif_set_real_num_tx_queues(ndev,
> +					     priv->plat->tx_queues_to_use);
> +		priv->qdisc_type = STMMAC_QDISC_NONE;
> +	}
>  }

[Severity: High]
Can this leave real_num_tx_queues permanently reduced?  The commit message
only mentions gating the FPE preemption class, but the queue-count restore
is now gated too.

On an mqprio -> taprio root replace the taprio REPLACE offload runs first
and sets qdisc_type = STMMAC_QDISC_TAPRIO.  The old mqprio teardown
(mqprio_destroy() -> mqprio_disable_offload() with num_tc == 0 ->
stmmac_reset_tc_mqprio()) then takes neither branch, so the value programmed
earlier by tc_setup_dwmac510_mqprio() stays in place:

	ndev_num_tx_queues = ndev->real_num_tx_queues;
	err = netif_set_real_num_tx_queues(ndev, num_tx_queues);

where num_tx_queues is the sum of qopt->count[i], which
mqprio_validate_queue_counts() allows to be smaller than
plat->tx_queues_to_use.

Nothing appears to restore it afterwards: taprio_destroy() in
net/sched/sch_taprio.c only calls netdev_reset_tc(dev) and never touches
real_num_tx_queues, and the stmmac taprio DESTROY path only clears the FPE
class.  Recovery then requires an interface down/up so that
stmmac_hw_setup() re-runs netif_set_real_num_tx_queues().  Later tc
configurations would also be validated against the stale
dev->real_num_tx_queues.

Should the netdev TC/queue-count restore be kept independent of the FPE
ownership state, since the two resources have different owners?

[Severity: Medium]
Does the reordering here break the XGMAC default TXQ-to-TC restore?  The
FPE clear is now issued before netdev_reset_tc(ndev), while previously the
order was netdev_reset_tc() -> netif_set_real_num_tx_queues() -> FPE clear.

dwxgmac3_fpe_map_preemption_class() in stmmac_fpe.c restores the default
mapping only when the netdev already has no traffic classes:

	int num_tc = netdev_get_num_tc(ndev);

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

and the qdisc core does not clear dev->num_tc for an offloaded mqprio:

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 on tc qdisc del dev X root with an offloaded mqprio, num_tc is still
non-zero when the callback runs, the restore branch is skipped, and the MTL
Q2TCMAP keeps the grouping of the mqprio that is being torn down.  Could the
FPE clear stay after netdev_reset_tc()?

>  
>  static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
> @@ -1318,6 +1330,8 @@ static int tc_setup_dwmac510_mqprio(struct stmmac_priv *priv,
>  	if (err)
>  		goto error_reset_num_tx_queues;
>  
> +	priv->qdisc_type = STMMAC_QDISC_MQPRIO;
> +

[Severity: Medium]
This isn't a problem introduced by this patch, but can err here ever be
non-zero?  struct stmmac_ops declares

	int (*fpe_map_preemption_class)(struct net_device *ndev,
					struct netlink_ext_ack *extack,
					u32 pclass);

while the accessor in hwif.h goes through the void variant:

	#define stmmac_fpe_map_preemption_class(__priv, __args...) \
		stmmac_do_void_callback(__priv, mac, fpe_map_preemption_class, __args)

stmmac_do_void_callback() discards the callback result and evaluates to 0
whenever the pointer is set.  dwmac5_fpe_map_preemption_class() does return
an error before programming FPE_MTL_PREEMPTION_CLASS:

	if (priv->plat->tx_sched_algorithm == MTL_TX_ALGORITHM_SP) {
		NL_SET_ERR_MSG_MOD(extack, ALG_ERR_MSG);
		return -EINVAL;
	}

so with an SP scheduler (or mismatched TXQ weights inside a TC) the mapping
is rejected but reported as success.  With this patch, qdisc_type is then set
to STMMAC_QDISC_MQPRIO (and likewise to STMMAC_QDISC_TAPRIO in
tc_taprio_configure()) for a mapping that was never written to the hardware,
so the new ownership state describes hardware state that does not exist.
Would routing the accessor through stmmac_do_callback(), or making the ops
member void, be preferable before building ownership tracking on the return
value?

>  	return 0;
>  
>  error_reset_num_tx_queues:

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