Re: [PATCH net v7 1/2] net: stmmac: selftests: Prevent RXC from being disabled in LPI

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The last paragraph of the commit message describes the
  `eee_rx_clk_stop_enable` precondition backwards.

--

net: stmmac: selftests: Prevent RXC from being disabled in LPI

The patch wraps the selftest loop in stmmac_selftest_run() with
phylink_rx_clk_stop_block() / phylink_rx_clk_stop_unblock() so the PHY
does not stop the receive clock while the MAC is in loopback. The code
change itself looks balanced and is a no-op when the phylink config does
not allow RX clock stop.

The question below is only about the changelog, so the diff hunk is not
quoted.

>     As the NETIF_F_VLAN_FEATURES may be empty on some platforms once the
>     software vlan stripping is removed, the phylink config attribute
>     'eee_rx_clk_stop_enable' may no longer be inconditionnally set to
>     true, hence allowing RXC to be disabled during LPI on platforms
>     that don't have any VLAN features.

[Severity: Medium]
Is the true/false direction here inverted?

include/linux/phylink.h documents the field as:

 * @eee_rx_clk_stop_enable: if true, PHY can stop the receive clock during LPI

So true is what permits the PHY to stop RXC, which makes "may no longer
be inconditionnally set to true, hence allowing RXC to be disabled"
read as its own opposite.

The code in stmmac_phylink_setup() also goes the other way, setting the
flag when the VLAN feature mask is empty:

	/* Disable EEE RX clock stop to ensure VLAN register access works
	 * correctly.
	 */
	if (!(priv->plat->flags & STMMAC_FLAG_RX_CLK_RUNS_IN_LPI) &&
	    !(priv->dev->features & NETIF_F_VLAN_FEATURES))
		config->eee_rx_clk_stop_enable = true;

In the current tree __stmmac_dvr_probe() sets, under
STMMAC_VLAN_TAG_USED:

	ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX;

so with CONFIG_VLAN_8021Q=y the flag is never true today, and it only
becomes true once the software VLAN stripping removal drops that
assignment. Would it be clearer to say that the flag starts being set to
true, thereby letting the PHY stop RXC?

Related to that, should the changelog also mention that
CONFIG_VLAN_8021Q=n builds are already affected today? With
STMMAC_VLAN_TAG_USED undefined in common.h, no VLAN features are ever
set, so eee_rx_clk_stop_enable is already true on platforms without
STMMAC_FLAG_RX_CLK_RUNS_IN_LPI and the selftests can already fail there.

As written it is hard to tell whether this fixes a live failure or is a
prerequisite for the following patch in the series, which also makes the
Fixes tag range hard to check.

There is also a spelling slip in the same paragraph: "inconditionnally"
should be "unconditionally".

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914150612.9798-1-peilinhe2020%40163.com