Re: [PATCH net-next v7 1/2] net: ti: icssg-prueth: Add Frame Preemption MAC Merge support

[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 Frame Preemption MAC Merge support

This patch adds QoS infrastructure for Frame Preemption (FPE) in the
ICSSG Ethernet driver, including a verify state machine driver helper,
mqprio-based preemptible-tc mapping, and a link-state callback. A
follow-up patch in this series wires this up to ethtool's MAC Merge ops.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -392,6 +392,8 @@ static void emac_adjust_link(struct net_device *ndev)
>  		} else {
>  			icssg_set_port_state(emac, ICSSG_EMAC_PORT_DISABLE);
>  		}
> +
> +		icssg_qos_link_state_update(ndev);
>  	}

[Medium]
emac_adjust_link() sets emac->link = 0 before reaching this call. Inside
icssg_qos_link_state_update() -> icssg_iet_change_preemptible_tcs() ->
icssg_config_ietfpe(), the very first check is:

    if (!emac->link) {
            netdev_dbg(ndev, "cannot change IET/FPE state ... \n");
            return 0;
    }

so on a link-down the IET path returns early without clearing
iet->fpe_active or iet->verify_status, even though firmware has already
been told ICSSG_EMAC_PORT_DISABLE just above. After the follow-up patch
adds get_mm(), would ethtool then report stale tx_active=true and a
stale verify_status while the link is down?

The commit message states:

    Verify state machine re-triggers on link up/down events based on
    fpe_enabled and fpe_active flags

Is the link-down half of that contract implemented anywhere?

[Medium]
A related question on the locking design: emac->link is written here by
emac_adjust_link() without taking iet->fpe_lock, but icssg_config_ietfpe()
reads emac->link inside the fpe_lock-held critical section. The lockset
for writers and readers is disjoint, so fpe_lock does not actually
serialize emac->link.

Can the following interleaving occur?

  CPU0 (ndo_setup_tc)               CPU1 (emac_adjust_link)
  mutex_lock(&iet->fpe_lock);
  reads emac->link == 1
                                     emac->link = 0;
                                     icssg_set_port_state(... DISABLE);
  proceeds to write FPE regs
  and issue PREMPT_TX_ENABLE
  mutex_unlock(&iet->fpe_lock);
                                     icssg_qos_link_state_update();
                                       sees emac->link == 0, returns

leaving stale FPE writes on a port the firmware has just disabled? The
commit message says "fpe_lock serializes all FPE state mutations" but
emac->link is one of those mutations and is not covered.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_qos.c b/drivers/net/ethernet/ti/icssg/icssg_qos.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/ethernet/ti/icssg/icssg_qos.c
> @@ -0,0 +1,282 @@

[ ... ]

> +static int icssg_iet_verify_wait(struct prueth_emac *emac)
> +{
> +	void __iomem *config = emac->dram.va + ICSSG_CONFIG_OFFSET;
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	unsigned long delay_us, timeout_us;
> +	u32 status;
> +	int ret;
> +
> +	delay_us = iet->verify_time_ms * 1000;
> +	timeout_us = delay_us * ICSSG_IET_VERIFY_ATTEMPTS;
> +
> +	ret = readb_poll_timeout(config + PRE_EMPTION_VERIFY_STATUS,
> +				 status,
> +				 status == ICSSG_IETFPE_STATE_SUCCEEDED,
> +				 delay_us,
> +				 timeout_us);
> +
> +	iet->verify_status = status;
> +	return ret;
> +}

[High]
With ICSSG_IET_MAX_VERIFY_TIME = 128 and ICSSG_IET_VERIFY_ATTEMPTS = 3,
this can sleep for up to 384ms while iet->fpe_lock is held. fpe_lock is
acquired by icssg_iet_change_preemptible_tcs() from two paths:

  - emac_tc_setup_mqprio() via .ndo_setup_tc, called with RTNL held.
  - icssg_qos_link_state_update() from emac_adjust_link(), invoked by
    phylib with phydev->lock held (prueth_emac_common_start()
    explicitly does mutex_lock(phydev->lock) before calling
    emac_adjust_link).

Once the follow-up patch wires set_mm() to write iet->mac_verify_configure,
this sleep becomes reachable from any user with CAP_NET_ADMIN. Should
the verify wait be deferred to a workqueue or a completion driven by a
firmware notification rather than block under RTNL / phydev->lock for
hundreds of milliseconds? Other drivers with MAC Merge support
(e.g. fec, stmmac) take the deferred-work approach for the same reason.

[Low]
The condition only breaks on ICSSG_IETFPE_STATE_SUCCEEDED, but the enum
also defines a distinct ICSSG_IETFPE_STATE_FAILED value reported by
firmware on a definitive verify failure. With the current condition,
would a FAILED state cause the loop to keep polling for the entire
timeout and then surface as:

    netdev_err(ndev, "MAC Verification failed with timeout\n");

even though it was not actually a timeout? Could the cond also break on
FAILED so the caller can distinguish the two?

> +int icssg_config_ietfpe(struct net_device *ndev, bool enable)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	void __iomem *config = emac->dram.va + ICSSG_CONFIG_OFFSET;
> +	struct prueth_qos_iet *iet = &emac->qos.iet;
> +	int ret;
> +	u8 val;
> +
> +	lockdep_assert_held(&iet->fpe_lock);
> +
> +	if (!emac->link) {
> +		netdev_dbg(ndev, "cannot change IET/FPE state when interface is down\n");
> +		return 0;
> +	}

[ ... ]

> +	if (enable && iet->mac_verify_configure) {
> +		ret = icssg_iet_verify_wait(emac);
> +		if (ret) {
> +			netdev_err(ndev, "MAC Verification failed with timeout\n");
> +			goto disable_tx;
> +		}
> +	} else if (enable) {
> +		/* Give firmware some time to update
> +		 * PRE_EMPTION_ACTIVE_TX state
> +		 */
> +		usleep_range(100, 200);
> +	}
> +
> +	if (enable) {
> +		val = readb(config + PRE_EMPTION_ACTIVE_TX);
> +		if (val != 1) {
> +			netdev_err(ndev,
> +				   "Firmware fails to activate IET/FPE\n");
> +			ret = -EIO;
> +			goto disable_tx;
> +		}
> +		iet->fpe_active = true;
> +	} else {
> +		iet->fpe_active = false;
> +	}
> +
> +	icssg_iet_set_preempt_mask(emac);
> +	netdev_dbg(ndev, "IET FPE %s successfully\n",
> +		   str_enable_disable(enable));
> +	return 0;
> +
> +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;
> +}

[Medium]
icssg_set_port_state() only synchronizes the R30 command-done handshake;
it does not guarantee firmware has finished updating the
PRE_EMPTION_ACTIVE_TX status byte. Is a 100-200us sleep a documented
worst-case latency for firmware to publish that byte under load
(concurrent Rx, TAS, FDB ageing, link events)?

If the byte has not yet been written, this code returns -EIO,
mischaracterizing it as "Firmware fails to activate IET/FPE", and the
fallback path then writes PRE_EMPTION_ENABLE_TX = 0, defeating the
user's enable. icssg_iet_verify_wait() already uses
readb_poll_timeout() for an analogous status byte; could the same
pattern be applied here with a small millisecond-range timeout?

[Medium]
The success path always calls icssg_iet_set_preempt_mask(), and on
fpe_active=false that helper takes the reset_hw branch and clears all
per-queue BIT(4) preemptive markers. The disable_tx and fallback paths
do not call icssg_iet_set_preempt_mask().

If FPE was previously active (so EXPRESS_PRE_EMPTIVE_Q_MAP[i] = BIT(4)
on some queues) and a subsequent reconfigure fails (verify timeout, or
PRE_EMPTION_ACTIVE_TX != 1), the failure paths only clear
PRE_EMPTION_ENABLE_TX / PRE_EMPTION_ENABLE_VERIFY but leave the
per-queue map and the EXPRESS_PRE_EMPTIVE_Q_MASK with stale preemptive
bits. Should the failure paths also call icssg_iet_set_preempt_mask()
(after iet->fpe_active is set to false) to keep the queue map and the
TX-enable bit consistent?

[ ... ]

> +EXPORT_SYMBOL_GPL(icssg_qos_init);

[ ... ]

> +EXPORT_SYMBOL_GPL(icssg_qos_ndo_setup_tc);

[ ... ]

> +EXPORT_SYMBOL_GPL(icssg_qos_link_state_update);

[Low]
Per the Makefile change in this patch:

  icssg-y := icssg/icssg_common.o \
             ...
             icssg/icssg_ethtool.o \
             icssg/icssg_qos.o

icssg_qos.o is linked into the same icssg.ko module as the only callers
(icssg_prueth.c and icssg_common.c). Are there cross-module consumers
that need these symbols exported, or can the EXPORT_SYMBOL_GPL lines be
dropped since EXPORT_SYMBOL_GPL is only required for cross-module
linkage?
-- 
pw-bot: cr