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

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

This commit adds driver support for viewing and changing the MAC Merge
sublayer parameters and dumping MAC Merge statistics via ethtool
operations.

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

[ ... ]

> @@ -294,6 +293,100 @@ 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;
> +
> +	if (emac->is_sr1)
> +		return -EOPNOTSUPP;
> +
> +	state->tx_enabled = iet->fpe_enabled;
> +	state->pmac_enabled = true;
> +	state->tx_min_frag_size = iet->tx_min_frag_size;
> +	/* 64Bytes is the minimum fragment size supported
> +	 * by the firmware. <64B leads to min frame errors
> +	 */
> +	state->rx_min_frag_size = 64;
> +	state->tx_active = iet->fpe_active;
> +	state->verify_enabled = iet->mac_verify_configure;
> +	state->verify_time = iet->verify_time_ms;
> +
> +	switch (iet->verify_status) {
> +	case ICSSG_IETFPE_STATE_DISABLED:
> +		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
> +		break;
> +	case ICSSG_IETFPE_STATE_SUCCEEDED:
> +		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED;
> +		break;
> +	case ICSSG_IETFPE_STATE_FAILED:
> +		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_FAILED;
> +		break;
> +	default:
> +		state->verify_status = ETHTOOL_MM_VERIFY_STATUS_UNKNOWN;
> +		break;
> +	}

Does this switch statement handle all valid verify_status values?

The switch only explicitly handles ICSSG_IETFPE_STATE_DISABLED,
ICSSG_IETFPE_STATE_SUCCEEDED, and ICSSG_IETFPE_STATE_FAILED, falling
through to ETHTOOL_MM_VERIFY_STATUS_UNKNOWN for any other states.

Looking at the icssg_ietfpe_verify_states enum, there are also
ICSSG_IETFPE_STATE_INITIAL and ICSSG_IETFPE_STATE_VERIFYING states. The
ethtool UAPI defines ETHTOOL_MM_VERIFY_STATUS_INITIAL and
ETHTOOL_MM_VERIFY_STATUS_VERIFYING which directly correspond to these
states.

When the MAC Verify process is actively running, userspace will see
'unknown' instead of the more accurate 'verifying' status. This means
information about the ongoing state machine transition is lost.

Should the switch include explicit cases for ICSSG_IETFPE_STATE_INITIAL
and ICSSG_IETFPE_STATE_VERIFYING to provide more accurate status
reporting to userspace?

> +
> +	/* 802.3-2018 clause 30.14.1.6, says that the aMACMergeVerifyTime
> +	 * variable has a range between 1 and 128 ms inclusive. Limit to that.
> +	 */
> +	state->max_verify_time = ETHTOOL_MM_MAX_VERIFY_TIME_MS;
> +
> +	return 0;
> +}

[ ... ]