This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: ti: icssg-prueth: Add ethtool ops for Frame Preemption MAC Merge

This patch wires up the ethtool MAC Merge ops (.get_mm, .set_mm,
.get_mm_stats) for the ICSSG PRU Ethernet driver and adds five firmware
preemption counters that feed both the new MM stats and the existing
ethtool -S table.

[ ... ]

+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");
+		return -EOPNOTSUPP;
+	}
+
+	mutex_lock(&iet->fpe_lock);
+	iet->verify_time_ms = cfg->verify_time;
+	iet->tx_min_frag_size = cfg->tx_min_frag_size;
+	iet->fpe_enabled = cfg->tx_enabled;
+	iet->mac_verify_configure = cfg->verify_enabled;
+	err = icssg_config_ietfpe(ndev, cfg->tx_enabled);
+	mutex_unlock(&iet->fpe_lock);
+
+	return err;
+}

[Medium]
Can the cached software state in iet diverge from the hardware here when
icssg_config_ietfpe() fails?

The new fields (verify_time_ms, tx_min_frag_size, fpe_enabled,
mac_verify_configure) are written to iet before icssg_config_ietfpe() is
called, but the error paths in icssg_config_ietfpe() only roll back a
subset of state:

icssg_qos.c:icssg_config_ietfpe() {
    ...
disable_tx:
        icssg_set_port_state(emac, ICSSG_EMAC_PORT_PREMPT_TX_DISABLE);
fallback:
        writeb(0, config + PRE_EMPTION_ENABLE_TX);
        writeb(0, config + PRE_EMPTION_ENABLE_VERIFY);
        iet->verify_status = ICSSG_IETFPE_STATE_DISABLED;
        iet->fpe_active =  false;
        return ret;
}

iet->fpe_enabled is left set to cfg->tx_enabled even though the hardware
TX preempt path has been disabled.

Two follow-on effects appear to come from this:

emac_get_mm() reports state->tx_enabled = iet->fpe_enabled, so userspace
sees tx_enabled=true after a set_mm that returned an error and left FPE
disabled in hardware.

icssg_qos_link_state_update() drives icssg_iet_change_preemptible_tcs(),
which calls icssg_config_ietfpe(emac->ndev, iet->fpe_enabled) on every
link transition, so the failed configuration is retried each link-up
without further user action.

Would it make sense to stage cfg into local variables, only commit them
to iet after icssg_config_ietfpe() returns success, or otherwise
restore the previous iet fields on the error return?

+static void emac_get_mm_stats(struct net_device *ndev,
+			      struct ethtool_mm_stats *s)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+
+	if (emac->is_sr1)
+		return;
+
+	if (!emac->prueth->pa_stats)
+		return;
+
+	emac_update_hardware_stats(emac);
+
+	/* MACMergeHoldCount stats is not tracked by the firmware */
+	s->MACMergeFrameAssOkCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_OK");
+	s->MACMergeFrameAssErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_ASSEMBLY_ERR");
+	s->MACMergeFragCountRx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_RX");
+	s->MACMergeFragCountTx = emac_get_stat_by_name(emac, "FW_PREEMPT_FRAG_CNT_TX");
+	s->MACMergeFrameSmdErrorCount = emac_get_stat_by_name(emac, "FW_PREEMPT_BAD_FRAG");
+}

[ ... ]

[Medium]
Are these five new entries duplicating values that already have a
standard uAPI?

The same five firmware counters are exposed through the new
.get_mm_stats callback as the standardized MAC Merge stats
(MACMergeFrameAssOkCount, MACMergeFrameAssErrorCount, MACMergeFragCountRx,
MACMergeFragCountTx, MACMergeFrameSmdErrorCount in struct
ethtool_mm_stats), and adding them to icssg_all_pa_stats[] also
publishes them via emac_get_strings() / emac_get_ethtool_stats() as
ethtool -S strings.

Documentation/networking/statistics.rst describes ethtool -S as the
private-driver-stats interface; counters that have a standard uAPI are
expected to flow only through that uAPI.

Could the firmware-register lookup table used by emac_get_stat_by_name()
be separated from the ethtool -S string table, so the new preemption
counters feed get_mm_stats without also showing up under ethtool -S?