Re: [PATCH net v4 2/7] net: stmmac: selftests: Validate EEE based on the actual LPI timer value

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 3 · Low: 0

- [Medium] In stmmac_test_eee() the deadline of both new polling loops is
  derived exclusively from priv->tx_lpi_timer (max_duration =…
- [Medium] stmmac_test_eee() infers the *current* LPI state from
  monotonic event counters instead of the actual MAC state.
- [Medium] stmmac_test_eee() validates priv->tx_lpi_timer against
  STMMAC_SFT_MAX_LPI (5 s) at stmmac_selftests.c:473 but never caches…

--

net: stmmac: selftests: Validate EEE based on the actual LPI timer value

The EEE selftest now sends a frame, waits up to roughly twice the configured
LPI timer for the enter-LPI interrupt counter to increase, then sends a
second frame and waits the same amount of time for the exit-LPI counter.
It also bails out with -EOPNOTSUPP when the configured LPI timer exceeds
five seconds.

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> index 614b5995dec5f..2f9f7746c40ae 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> @@ -30,6 +30,7 @@ struct stmmachdr {
>  			      sizeof(struct stmmachdr))
>  #define STMMAC_TEST_PKT_MAGIC	0xdeadcafecafedeadULL
>  #define STMMAC_LB_TIMEOUT	msecs_to_jiffies(200)
> +#define STMMAC_SFT_MAX_LPI	(5 * USEC_PER_SEC)
>  
>  struct stmmac_packet_attrs {
>  	int vlan;
> @@ -462,12 +463,16 @@ static int stmmac_test_mmc(struct stmmac_priv *priv)
>  static int stmmac_test_eee(struct stmmac_priv *priv)
>  {
>  	struct stmmac_extra_stats *initial, *final;
> -	int retries = 10;
> +	unsigned long timeout, max_duration;
>  	int ret;
>  
>  	if (!priv->dma_cap.eee || !priv->eee_active)
>  		return -EOPNOTSUPP;
>  
> +	/* Bail out if the configured LPI timer is too long */
> +	if (priv->tx_lpi_timer > STMMAC_SFT_MAX_LPI)
> +		return -EOPNOTSUPP;
> +

[Severity: Medium]
Is it intentional that priv->tx_lpi_timer is read twice here, once for the
bound check and again later for max_duration, without caching the checked
value?

Between the two reads stmmac_test_eee() allocates two stats buffers and runs
a full loopback transmit/receive, which can take up to STMMAC_LB_TIMEOUT
(200 ms).

priv->tx_lpi_timer is written outside the selftest by
stmmac_mac_enable_tx_lpi() in stmmac_main.c:

	priv->tx_lpi_timer = timer;
	priv->eee_active = true;

That path is reached from phylink_link_up()->phylink_activate_lpi() inside
the phylink_resolve() work item, which takes only pl->phydev_mutex,
phy->lock and pl->state_mutex, not rtnl. The selftest itself runs with
rtnl_lock held from __dev_ethtool(), so the resolve work can update
priv->tx_lpi_timer in the middle of the test.

Given that neither ethnl_set_eee() nor phy_ethtool_set_eee() range-check
ETHTOOL_A_EEE_TX_LPI_TIMER, can the value used for max_duration therefore be
one that was never validated against STMMAC_SFT_MAX_LPI? For example, a
timer of 60000000 us gives 120 s per wait loop, and 4000000000 us wraps in
the u32 multiplication below to 3705032704 us, i.e. around 3705 s per loop,
all spent in msleep(100) with rtnl_lock held.

Would reading priv->tx_lpi_timer once into a local u32 and validating and
using that single copy (with 64-bit or otherwise bounded arithmetic for the
doubling) address this?

>  	initial = kzalloc_obj(*initial);
>  	if (!initial)
>  		return -ENOMEM;
> @@ -478,14 +483,21 @@ static int stmmac_test_eee(struct stmmac_priv *priv)
>  		goto out_free_initial;
>  	}
>  
> +	/* Snapshot stats, we want to count the in_lpi events. We may enter
> +	 * LPI just after the packet was sent.
> +	 */
>  	memcpy(initial, &priv->xstats, sizeof(*initial));
>  
> +	/* Send a frame, then wait to enter LPI */
>  	ret = stmmac_test_mac_loopback(priv);
>  	if (ret)
>  		goto out_free_final;
>  
> +	max_duration = usecs_to_jiffies(2 * priv->tx_lpi_timer);
> +

[Severity: Medium]
Does this deadline hold for small LPI timer values?

The patch adds an upper bail-out for priv->tx_lpi_timer but no lower bound,
and the value is user-configurable via ethtool --set-eee tx-lpi-timer, and
can also be 0 via the eee_timer module parameter used in stmmac_open().

For a timer of 0 us, _usecs_to_jiffies(0) is 0, so "timeout = jiffies" is
already at the boundary and the loop body may run zero or one time. For any
timer below ~200 ms the budget is smaller than the loop's own msleep(100)
granularity, so the loop degenerates to a single sample.

Independently of the granularity, stmmac_mac_enable_tx_lpi() programs the
link-status gate on every EEE enable:

	stmmac_set_eee_timer(priv, priv->hw, STMMAC_DEFAULT_LIT_LS,
			     STMMAC_DEFAULT_TWT_LS);

with STMMAC_DEFAULT_LIT_LS defined as 0x3E8 in common.h, and
dwmac4_set_eee_timer() documents it as:

	/* LS: minimum time (ms) for which the link
	 *  status from PHY should be ok before transmitting
	 *  the LPI pattern. */

So 1000 ms of stable link is required before LPI may be transmitted at all.
With a configured LPI timer below roughly 400 ms and a recent link-up (the
usual bring-link-up-then-run-ethtool-t sequence), does the enter-LPI
deadline expire before the hardware is even permitted to enter LPI, making
stmmac_test_eee() return -ETIMEDOUT and stmmac_selftest_run() set
ETH_TEST_FL_FAILED on working hardware? The previous fixed ~900 ms budget
covered those cases.

Would a floor on max_duration, or a lower bail-out mirroring the new upper
one, be appropriate?

>  	/* We have no traffic in the line so, sooner or later it will go LPI */
> -	while (--retries) {
> +	timeout = jiffies + max_duration;
> +	while (!time_after(jiffies, timeout)) {
>  		memcpy(final, &priv->xstats, sizeof(*final));
>  
>  		if (final->irq_tx_path_in_lpi_mode_n >
> @@ -494,20 +506,38 @@ static int stmmac_test_eee(struct stmmac_priv *priv)
>  		msleep(100);
>  	}
>  
> -	if (!retries) {
> +	memcpy(final, &priv->xstats, sizeof(*final));
> +	if (final->irq_tx_path_in_lpi_mode_n <=
> +	    initial->irq_tx_path_in_lpi_mode_n) {
>  		ret = -ETIMEDOUT;
>  		goto out_free_final;
>  	}
>  
> -	if (final->irq_tx_path_in_lpi_mode_n <=
> -	    initial->irq_tx_path_in_lpi_mode_n) {
> -		ret = -EINVAL;
> +	/* Re-snapshot, as we want to measure exit_lpi events. We should be
> +	 * in LPI right now.
> +	 */
> +	memcpy(initial, &priv->xstats, sizeof(*initial));
> +
> +	/* TX something so we go out of LPI */
> +	ret = stmmac_test_mac_loopback(priv);
> +	if (ret)
>  		goto out_free_final;

[Severity: Medium]
Can the "We should be in LPI right now" assumption be established from
irq_tx_path_in_lpi_mode_n?

That counter is a monotonic per-event counter bumped from the IRQ handler
and never decremented on exit, see dwmac4_irq_status():

	if (status & LPI_CTRL_STATUS_TLPIEN) {
		ret |= CORE_IRQ_TX_PATH_IN_LPI_MODE;
		x->irq_tx_path_in_lpi_mode_n++;
	}

The enter baseline is taken before the first loopback frame, so an entry
event that occurred in the window between the memcpy() and the transmit
inside stmmac_test_mac_loopback() satisfies the first comparison even though
the frame itself terminated that LPI interval:

stmmac_xmit()
	if (priv->tx_path_in_lpi_mode && priv->eee_sw_timer_en)
		stmmac_stop_sw_lpi(priv);

stmmac_stop_sw_lpi()
	timer_delete_sync(&priv->eee_ctrl_timer);
	stmmac_set_lpi_mode(priv, priv->hw, STMMAC_LPI_DISABLE, false, 0);
	priv->tx_path_in_lpi_mode = false;

The same applies when unrelated stack traffic (ARP/ND/MLD on an up
interface) wakes the TX path during the up-to-100 ms poll interval, and for
the LPITXA hardware path where queuing a frame leaves LPI immediately.

In that case the re-snapshot folds the matching exit event into the new exit
baseline, the second frame is sent while the MAC is already awake, no new TX
exit-LPI event is generated, and with no further traffic the second loop
drains its full budget and returns -ETIMEDOUT, which stmmac_selftest_run()
reports as ETH_TEST_FL_FAILED.

The driver already tracks the current state in priv->tx_path_in_lpi_mode
(and LPI_CTRL_STATUS is readable). Would checking the actual state, or
waiting for a fresh entry event after taking the exit baseline and before
sending the wake frame, make the two phases deterministic? As written, phase
one can also pass on an entry event that predates the test frame, so the
enter-after-frame sequence described in the commit message is not strictly
verified.

> +
> +	/* Wait for the exit LPI interrupt */
> +	timeout = jiffies + max_duration;
> +	while (!time_after(jiffies, timeout)) {
> +		memcpy(final, &priv->xstats, sizeof(*final));
> +
> +		if (final->irq_tx_path_exit_lpi_mode_n >
> +		    initial->irq_tx_path_exit_lpi_mode_n)
> +			break;
> +		msleep(100);
>  	}
>  
> +	memcpy(final, &priv->xstats, sizeof(*final));
>  	if (final->irq_tx_path_exit_lpi_mode_n <=
>  	    initial->irq_tx_path_exit_lpi_mode_n) {
> -		ret = -EINVAL;
> +		ret = -ETIMEDOUT;
>  		goto out_free_final;
>  	}
>  

Thanks,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917215339.2022523-1-maxime.chevallier%40bootlin.com