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

[Jakub Kicinski <kuba@kernel.org>]

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