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

[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: 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()?

[ ... ]