Re: [PATCH net-next v3 3/3] net: dsa: realtek: rtl8365mb: add EEE support

[netdev-bot+sashiko]

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

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

- [Medium] config->eee_enabled_default = true (rtl8365mb_main.c:1724)
  turns EEE on out of the box for every integrated-PHY port; no user…

--

net: dsa: realtek: rtl8365mb: add EEE support

This adds EEE support for the integrated-PHY ports of rtl8365mb by mapping
the EEE MMD registers onto the switch OCP address space through new
phy_{read,write}_c45 realtek_ops. It also advertises LPI awareness to
phylink with no-op mac_{en,dis}able_tx_lpi callbacks and rejects the
per-port TX LPI knobs in .set_mac_eee.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index efde1f3b604e1..87ec924a06118 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -1649,6 +1714,14 @@ static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  		 */
>  		__set_bit(PHY_INTERFACE_MODE_GMII,
>  			  config->supported_interfaces);
> +
> +		/* Integrated PHYs support EEE at 100M/1G; the hardware manages
> +		 * LPI on its own, so just advertise LPI awareness to phylink.
> +		 */
> +		memcpy(config->lpi_interfaces, config->supported_interfaces,
> +		       sizeof(config->lpi_interfaces));
> +		config->lpi_capabilities = MAC_100FD | MAC_1000FD;
> +		config->eee_enabled_default = true;
                ^^^^

[Severity: Medium]
Should the commit message mention that this line changes the default
behaviour of already-supported boards?

The commit message says:

    Add EEE support on the integrated-PHY ports, controllable per port via
    ethtool.

and:

    The hardware manages LPI on its own, so advertise EEE to phylink with
    no-op mac_{en,dis}able_tx_lpi callbacks.

That reads as opt-in, but setting config->eee_enabled_default in
rtl8365mb_phylink_get_caps() makes EEE active without any ethtool command.
phylink_create() copies the flag into its own configuration:

drivers/net/phy/phylink.c:phylink_create() {
	...
	pl->eee_cfg.eee_enabled = pl->config->eee_enabled_default;
	pl->eee_cfg.tx_lpi_enabled = pl->eee_cfg.eee_enabled;
	...
}

and the advertisement is then written at the first link bring-up:

drivers/net/phy/phylink.c:phylink_bringup_phy() {
	...
	if (pl->eee_cfg.eee_enabled)
		phy_support_eee(phy);
	...
}

So after a kernel upgrade every integrated-PHY port on every supported
rtl8365mb board starts advertising EEE and entering LPI, where previously it
did not. EEE enabled by default is known to cause link instability with some
link partners.

Could the commit message state this default change explicitly, so that
maintainers and backporters see it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916090102.349157-1-o.rempel%40pengutronix.de