Re: [PATCH net-next v5 2/2] net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

Jakub Kicinski
2026-05-05T23:01:39Z

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: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

This patch adds driver support for viewing and changing the MAC Merge
sublayer parameters via the .get_mm(), .set_mm() and .get_mm_stats()
ethtool ops, and extends emac_get_stat_by_name() to return u64.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> index b715af21d23a..6a08c40adde5 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c

[ ... ]

> @@ -294,6 +293,109 @@ static int emac_set_per_queue_coalesce(struct net_device *ndev, u32 queue,
> return 0;
> }
>
> +static int emac_get_mm(struct net_device *ndev, struct ethtool_mm_state *state)
> +{
> + struct prueth_emac *emac = netdev_priv(ndev);
> + struct prueth_qos_iet *iet = &emac->qos.iet;
> + enum icssg_ietfpe_verify_states verify_status;
> +
> + if (emac->is_sr1)
> + return -EOPNOTSUPP;
> +
> + mutex_lock(&iet->fpe_lock);
> + state->tx_enabled = iet->fpe_enabled;
> + state->tx_min_frag_size = iet->tx_min_frag_size - ETH_FCS_LEN;

Can emac_get_mm() return sensible values on first call?

Looking at icssg_qos_init() in icssg_qos.c:

void icssg_qos_init(struct net_device *ndev)
{
struct prueth_emac *emac = netdev_priv(ndev);
struct prueth_qos_iet *iet = &emac->qos.iet;

iet->emac = emac;
mutex_init(&iet->fpe_lock);
}

Only emac and the mutex are initialized, so iet->tx_min_frag_size and
iet->verify_time_ms remain zero (netdev_priv() is zero-initialized).

On first invocation state->tx_min_frag_size becomes 0 - ETH_FCS_LEN,
which wraps to 0xFFFFFFFC and is then reported to userspace via
ETHTOOL_A_MM_TX_MIN_FRAG_SIZE.

There is a follow-on effect in ethnl_set_mm() (net/ethtool/mm.c) which
calls get_mm first and copies tx_min_frag_size and verify_time into
cfg via mm_state_to_cfg(). If the user does not pass
ETHTOOL_A_MM_TX_MIN_FRAG_SIZE, cfg.tx_min_frag_size stays at
0xFFFFFFFC. emac_set_mm() then hands that value to
icssg_qos_validate_tx_min_frag_size(), where:

min_frag_size += ETH_FCS_LEN; /* 0xFFFFFFFC + 4 == 0 */
if (min_frag_size < 64)
return -EINVAL; /* "tx_min_frag_size must be at least 64 bytes" */

so the first ethtool --set-mm invocation fails with a misleading
diagnostic unless the user specifies every tunable explicitly.

The verify_time_ms = 0 case behaves similarly and is rejected by
icssg_qos_validate_verify_time().

Should icssg_qos_init() seed iet->tx_min_frag_size (e.g. 64 including
FCS) and iet->verify_time_ms (e.g. 128) to sane defaults?

> + state->tx_active = iet->fpe_active;
> + state->verify_enabled = iet->mac_verify_configure;
> + state->verify_time = iet->verify_time_ms;
> + verify_status = iet->verify_status;
> + mutex_unlock(&iet->fpe_lock);
> +
> + state->pmac_enabled = true;
> + /* 64Bytes is the minimum fragment size supported
> + * by the firmware. <64B leads to min frame errors
> + */
> + state->rx_min_frag_size = 64;

Is the unit of rx_min_frag_size consistent with tx_min_frag_size here?

The 64 byte floor includes FCS, but the tx_min_frag_size reported just
above is iet->tx_min_frag_size - ETH_FCS_LEN, i.e. excluding FCS.

Peer drivers report rx_min_frag_size without FCS, for example
mscc/ocelot_mm.c and stmmac_ethtool.c use ETH_ZLEN (60), and
enetc_ethtool.c uses ethtool_mm_frag_size_add_to_min(lafs).

The inconsistency also means a user echoing the reported rx value back
as tx_min_frag_size would hit icssg_qos_validate_tx_min_frag_size()
because 64 + 4 = 68 is not a multiple of 64.

Should this be 64 - ETH_FCS_LEN (or ETH_ZLEN) to match the uAPI
convention?

[ ... ]

> + return 0;
> +}
> +
> +static int emac_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
> + struct netlink_ext_ack *extack)
> +{
> + struct prueth_emac *emac = netdev_priv(ndev);
> + struct prueth_qos_iet *iet = &emac->qos.iet;
> + int err;
> +
> + if (emac->is_sr1)
> + return -EOPNOTSUPP;
> +
> + if (!cfg->pmac_enabled)
> + NL_SET_ERR_MSG_MOD(extack, "preemptible MAC is always enabled");

Should this path also return an error?

The extack is set but control falls through, all remaining fields are
applied, icssg_config_ietfpe() runs, and the function returns 0. A
caller doing "ethtool --set-mm ethN pmac-enabled off" sees success even
though emac_get_mm() hardcodes state->pmac_enabled = true and the
request was ignored.

Peer drivers (enetc, igc, ocelot, am65-cpsw) either honour
pmac_enabled or return -EOPNOTSUPP. Would returning -EOPNOTSUPP (or
-EINVAL) here be more consistent with the ethtool convention that a
non-empty extack accompanies a non-zero return?

> +
> + err = icssg_qos_validate_tx_min_frag_size(cfg->tx_min_frag_size, extack);
> + if (err)
> + return err;
> +
> + err = icssg_qos_validate_verify_time(cfg->verify_time, extack);
> + if (err)
> + return err;
> +
> + mutex_lock(&iet->fpe_lock);
> + iet->verify_time_ms = cfg->verify_time;
> + iet->tx_min_frag_size = cfg->tx_min_frag_size + ETH_FCS_LEN;
> + iet->fpe_enabled = cfg->tx_enabled;
> + iet->mac_verify_configure = cfg->verify_enabled;
> + icssg_config_ietfpe(emac, cfg->tx_enabled);
> + mutex_unlock(&iet->fpe_lock);
> +
> + return err;
> +}

Can firmware-level failures in icssg_config_ietfpe() be surfaced to
userspace?

icssg_config_ietfpe() returns void, but it can fail in several ways:

/* icssg_set_port_state() command failure */
ret = icssg_set_port_state(emac, ... );
if (ret) {
netdev_err(emac->ndev, "TX preempt %s command failed\n", ...);
...
return;
}

/* MAC Verify wait timeout */
ret = icssg_iet_verify_wait(emac);
if (ret) {
netdev_err(emac->ndev, "MAC Verification failed with timeout\n");
return;
}

/* Firmware failed to activate FPE */
val = readb(config + PRE_EMPTION_ACTIVE_TX);
if (val != 1) {
netdev_err(emac->ndev, "Firmware fails to activate IET/FPE\n");
return;
}

Each case emits only a netdev_err(). emac_set_mm() then returns err,
which is 0 at that point, so "ethtool --set-mm" reports success even
when the hardware rejected the configuration or MAC Verify timed out.

Would it be reasonable to change icssg_config_ietfpe() to return int
and propagate the error (with an extack message) from emac_set_mm()?

[ ... ]