[PATCH net-next v3 0/2] net: stmmac: fpe: pmac and fprq fixes

[PATCH net-next v3 0/2] net: stmmac: fpe: pmac and fprq fixes

[muhammad.nazim.amirul.nazle.asmade]

From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>

Two fixes for the stmmac FPE implementation:

- Keep pmac_enabled true in DWMAC since the preemptive MAC is always
  on, enforce this in set_mm(), and only initialize it when FPE is
  actually supported
- Move fprq configuration to stmmac_fpe_configure_pmac() where it
  belongs

v3:
  - Rework stmmac_fpe_init(): gate the pmac_enabled initialization on
    stmmac_fpe_supported() and return early otherwise, so hardware
    without FPE (dma_cap.fpesel clear) no longer gets pmac_enabled set.
    This folds in the v2 "Add missing return in stmmac_fpe_init()" patch
    and fixes the condition that did not reach its intended effect, per
    Paolo Abeni / Sashiko. The separate patch 2/3 from v2 is dropped.
  - v2: https://lore.kernel.org/all/20260803032358.16282-1-muhammad.nazim.amirul.nazle.asmade@altera.com/

v2:
  - Drop u32_replace_bits() in the fprq patch; fprq_mask is not a
    compile-time constant and triggered a __bad_mask build error on x86.
    Keep the open-coded mask/shift update instead.

Nazim Amirul (2):
  net: stmmac: fpe: Enforce pmac_enabled in set_mm and keep it on at
    init
  net: stmmac: fpe: Move fprq configuration to
    stmmac_fpe_configure_pmac()

 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |  7 ++++
 .../net/ethernet/stmicro/stmmac/stmmac_fpe.c  | 37 ++++++++++++-------
 2 files changed, 30 insertions(+), 14 deletions(-)

-- 
2.43.7

[PATCH net-next v3 1/2] net: stmmac: fpe: Enforce pmac_enabled in set_mm and keep it on at init

[muhammad.nazim.amirul.nazle.asmade]

From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>

The preemptive MAC in DWMAC is always enabled, meaning DWMAC is always
ready to receive preemptive mPackets. Initialize pmac_enabled to true
at init time and reject attempts to disable it via ethtool set_mm.

Only initialize pmac_enabled when FPE is actually supported: gate it on
stmmac_fpe_supported() and return early otherwise, so hardware without
FPE (dma_cap.fpesel clear) does not get pmac_enabled set. Keep emitting
the "FPE is not supported by driver" notice for the case where the
hardware advertises FPE but the driver cannot wire it up.

Also add a missing fpesel capability check in stmmac_set_mm() to return
-EOPNOTSUPP early on hardware without FPE support.

Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
v3: Gate the pmac_enabled init on stmmac_fpe_supported() and return
    early, so hardware without FPE no longer gets pmac_enabled set. This
    folds in the previous "Add missing return in stmmac_fpe_init()" patch
    and fixes the condition that did not reach its intended effect, per
    Paolo Abeni / Sashiko.
v2: No changes. Collect Reviewed-by from Maxime Chevallier.

 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c    |  7 +++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c    | 13 ++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index c616a1c95e5d..0dfb312d72f1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -1066,6 +1066,13 @@ static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
 	if (err)
 		return err;
 
+	if (!priv->dma_cap.fpesel)
+		return -EOPNOTSUPP;
+
+	/* DWMAC always have preemptible MAC enabled */
+	if (!cfg->pmac_enabled)
+		return -EINVAL;
+
 	stmmac_fpe_set_add_frag_size(priv, frag_size);
 	ethtool_mmsv_set_mm(&priv->fpe_cfg.mmsv, cfg);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
index b6b5ef7c8fc4..2607e29930b7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
@@ -173,9 +173,16 @@ void stmmac_fpe_init(struct stmmac_priv *priv)
 	ethtool_mmsv_init(&priv->fpe_cfg.mmsv, priv->dev,
 			  &stmmac_mmsv_ops);
 
-	if ((!priv->fpe_cfg.reg || !priv->hw->mac->fpe_map_preemption_class) &&
-	    priv->dma_cap.fpesel)
-		dev_info(priv->device, "FPE is not supported by driver.\n");
+	if (!stmmac_fpe_supported(priv)) {
+		if (priv->dma_cap.fpesel)
+			dev_info(priv->device, "FPE is not supported by driver.\n");
+		return;
+	}
+
+	/* The preemptive MAC in DWMAC is always enabled, so initialize
+	 * pmac_enabled to true to reflect the hardware state.
+	 */
+	priv->fpe_cfg.mmsv.pmac_enabled = true;
 }
 
 int stmmac_fpe_get_add_frag_size(struct stmmac_priv *priv)
-- 
2.43.7

[PATCH net-next v3 2/2] net: stmmac: fpe: Move fprq configuration to stmmac_fpe_configure_pmac()

[muhammad.nazim.amirul.nazle.asmade]

From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>

The Frame Preemption Residue Queue (fprq) controls which Rx queue
receives residual preemptive mPackets from the pmac. This is a pmac
concern, not a TX concern, so move its configuration from
stmmac_fpe_configure_tx() to stmmac_fpe_configure_pmac() where it
logically belongs.

Keep the open-coded mask/shift update rather than u32_replace_bits(),
since fprq_mask is not a compile-time constant.

Signed-off-by: Rohan G Thomas <rohan.g.thomas@altera.com>
Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
v3: No changes. Was patch 3/3 in v2.
v2: Drop u32_replace_bits(); fprq_mask is not a compile-time constant and
    triggered a __bad_mask build error on x86. Keep the open-coded
    mask/shift update instead, per Maxime Chevallier and Jakub Kicinski.

 .../net/ethernet/stmicro/stmmac/stmmac_fpe.c  | 24 ++++++++++---------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
index 2607e29930b7..cb153857077f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_fpe.c
@@ -47,20 +47,13 @@ static void stmmac_fpe_configure_tx(struct ethtool_mmsv *mmsv, bool tx_enable)
 	struct stmmac_fpe_cfg *cfg = container_of(mmsv, struct stmmac_fpe_cfg, mmsv);
 	struct stmmac_priv *priv = container_of(cfg, struct stmmac_priv, fpe_cfg);
 	const struct stmmac_fpe_reg *reg = cfg->reg;
-	u32 num_rxq = priv->plat->rx_queues_to_use;
 	void __iomem *ioaddr = priv->ioaddr;
-	u32 value;
 
-	if (tx_enable) {
+	if (tx_enable)
 		cfg->fpe_csr = STMMAC_MAC_FPE_CTRL_STS_EFPE;
-		value = readl(ioaddr + reg->rxq_ctrl1_reg);
-		value &= ~reg->fprq_mask;
-		/* Keep this SHIFT, FIELD_PREP() expects a constant mask :-/ */
-		value |= (num_rxq - 1) << __ffs(reg->fprq_mask);
-		writel(value, ioaddr + reg->rxq_ctrl1_reg);
-	} else {
+	else
 		cfg->fpe_csr = 0;
-	}
+
 	writel(cfg->fpe_csr, ioaddr + reg->mac_fpe_reg);
 }
 
@@ -68,10 +61,11 @@ static void stmmac_fpe_configure_pmac(struct ethtool_mmsv *mmsv, bool pmac_enabl
 {
 	struct stmmac_fpe_cfg *cfg = container_of(mmsv, struct stmmac_fpe_cfg, mmsv);
 	struct stmmac_priv *priv = container_of(cfg, struct stmmac_priv, fpe_cfg);
+	u32 num_rxq = priv->plat->rx_queues_to_use;
 	const struct stmmac_fpe_reg *reg = cfg->reg;
 	void __iomem *ioaddr = priv->ioaddr;
 	unsigned long flags;
-	u32 value;
+	u32 value, value_2;
 
 	spin_lock_irqsave(&priv->hw->irq_ctrl_lock, flags);
 	value = readl(ioaddr + reg->int_en_reg);
@@ -83,6 +77,14 @@ static void stmmac_fpe_configure_pmac(struct ethtool_mmsv *mmsv, bool pmac_enabl
 
 			value |= reg->int_en_bit;
 		}
+		/* Frame Preemption Residue Queue is the Rx Queue to which
+		 * residual preemptive mPackets must be forwarded from the pmac.
+		 */
+		value_2 = readl(ioaddr + reg->rxq_ctrl1_reg);
+		value_2 &= ~reg->fprq_mask;
+		/* Keep this SHIFT, FIELD_PREP() expects a constant mask :-/ */
+		value_2 |= (num_rxq - 1) << __ffs(reg->fprq_mask);
+		writel(value_2, ioaddr + reg->rxq_ctrl1_reg);
 	} else {
 		value &= ~reg->int_en_bit;
 	}
-- 
2.43.7

Re: [PATCH net-next v3 1/2] net: stmmac: fpe: Enforce pmac_enabled in set_mm and keep it on at init

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: stmmac: fpe: Enforce pmac_enabled in set_mm and keep it on at init

This patch sets the FPE pMAC state to enabled in stmmac_fpe_init() when
FPE is supported. It also makes stmmac_set_mm() reject requests that try
to disable the pMAC, and adds an fpesel capability check to
stmmac_set_mm().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index c616a1c95e5dd..0dfb312d72f15 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -1066,6 +1066,13 @@ static int stmmac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
>  	if (err)
>  		return err;
>  
> +	if (!priv->dma_cap.fpesel)
> +		return -EOPNOTSUPP;
> +

[Severity: Low]

The commit message says:

  Also add a missing fpesel capability check in stmmac_set_mm() to return
  -EOPNOTSUPP early on hardware without FPE support.

Can this check ever be reached? The only caller of ->set_mm() is
ethnl_set_mm(). It calls ->get_mm() first and returns if that fails:

net/ethtool/mm.c:ethnl_set_mm() {
    ...
	ret = dev->ethtool_ops->get_mm(dev, &state);
	if (ret)
		return ret;
    ...
	ret = dev->ethtool_ops->set_mm(dev, &cfg, extack);
    ...
}

stmmac_get_mm() already rejects this case with a stricter test:

drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c:stmmac_get_mm() {
    ...
	if (!stmmac_fpe_supported(priv))
		return -EOPNOTSUPP;
    ...
}

stmmac_fpe_supported() checks fpesel && fpe_cfg.reg &&
fpe_map_preemption_class. So stmmac_set_mm() is never called when fpesel
is clear.

The new test is also weaker than stmmac_fpe_supported(), which this same
patch uses in stmmac_fpe_init(). Suppose fpesel is set but the driver has
no FPE register map. Even if this check could run, it would not stop
stmmac_fpe_set_add_frag_size() from dereferencing a NULL fpe_cfg.reg.

Should this check be dropped or changed to use stmmac_fpe_supported(),
with the commit message updated to match?

> +	/* DWMAC always have preemptible MAC enabled */
> +	if (!cfg->pmac_enabled)
> +		return -EINVAL;

[Severity: Medium]

Does this change the ethtool MM uAPI behaviour in a way that breaks
existing users?

Before this patch, pmac-enabled off had a real effect on stmmac.
ethtool_mmsv_event_handle() does not send Respond mPackets while the pMAC
is disabled:

net/ethtool/mm.c:ethtool_mmsv_event_handle() {
    ...
	if (!mmsv->pmac_enabled)
		goto unlock;
    ...
}

stmmac_fpe_configure_pmac(false) also masks the FPE interrupt. Now
stmmac_fpe_init() sets pmac_enabled to true and nothing can clear it. On
every link-up the FPE IRQ is unmasked, and the device answers any link
partner's Verify mPacket.

The rejection also happens before ethtool_mmsv_set_mm() runs, so the
whole request is dropped. ethnl_set_mm() only rejects tx_enabled combined
with !pmac_enabled, so a request like this one reaches stmmac_set_mm():

  ethtool --set-mm $dev pmac-enabled off tx-enabled off verify-enabled off

It gets -EINVAL, and tx_enabled and verify_enabled stay unchanged.

h1_destroy() and h2_destroy() in
tools/testing/selftests/drivers/net/hw/ethtool_mm.sh use exactly this
request. The lldp test has lldpad enable MM TX, and afterwards the cleanup
cannot clear tx-enabled or verify-enabled on stmmac.

manual_failed_verification() in the same script relies on this command to
make the peer's verification fail:

  ethtool --set-mm $rx pmac-enabled off

The script does not check its exit code, so the rx side keeps
pmac_enabled set. If the DWMAC still sends a Respond mPacket in that
state, verification succeeds and check_fail fires. Whether the hardware
does that could not be confirmed from the code.

The other set_mm implementations in drivers/net (enetc, ocelot,
am65-cpsw) accept pmac_enabled=false. Would it be better to keep
accepting it here as well?

[Severity: Low]

This isn't a bug, but stmmac_set_mm() is passed an extack and this new
-EINVAL leaves it empty. The other rejections on the ETHTOOL_MSG_MM_SET
path all set a message. For example, in ethnl_set_mm():

	if (cfg.tx_enabled && !cfg.pmac_enabled) {
		NL_SET_ERR_MSG(extack, "TX enabled requires pMAC enabled");
		return -EINVAL;
	}

ethtool_mm_frag_size_min_to_add(), called a few lines earlier in
stmmac_set_mm(), fills in extack too.

Userspace gets a plain "Invalid argument" for a bool that the uAPI
documents as settable. Could something like
NL_SET_ERR_MSG_MOD(extack, "pMAC cannot be disabled on DWMAC") be added
here?

> +
>  	stmmac_fpe_set_add_frag_size(priv, frag_size);
>  	ethtool_mmsv_set_mm(&priv->fpe_cfg.mmsv, cfg);

[ ... ]
-- 
pw-bot: cr