Re: [PATCH net-next v20 06/13] ax88179_178a: Add EEE configuration support for AX88179A PHYs

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch fills phylink_config.lpi_interfaces in
  ax88179a_phylink_setup() (drivers/net/usb/ax88179a_devices.c).
- [Low] The new ax88179a_set_eee()
  (drivers/net/usb/ax88179a_devices.c:235-240) passes the user's struct…

--

ax88179_178a: Add EEE configuration support for AX88179A PHYs

This commit copies supported_interfaces into phylink_config.lpi_interfaces
in ax88179a_phylink_setup(), so phylink now treats the MAC as supporting
EEE. It also adds get_eee/set_eee ethtool ops that forward to
phylink_ethtool_get_eee() and phylink_ethtool_set_eee().

> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index 62619fd68c408..272e923656832 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
> @@ -225,6 +225,20 @@ static int ax88179a_get_eeprom_len(struct net_device *net)
>  		return AX88179A_EEPROM_LEN;
>  }
>  
> +static int ax88179a_get_eee(struct net_device *net, struct ethtool_keee *edata)
> +{
> +	struct ax88179_data *ax179_data = netdev2data(net);
> +
> +	return phylink_ethtool_get_eee(ax179_data->phylink, edata);
> +}
> +
> +static int ax88179a_set_eee(struct net_device *net, struct ethtool_keee *edata)
> +{
> +	struct ax88179_data *ax179_data = netdev2data(net);
> +
> +	return phylink_ethtool_set_eee(ax179_data->phylink, edata);
> +}

[Severity: Low]
Should ax88179a_set_eee() reject a tx_lpi_timer that the hardware cannot
apply?

Neither ax88179a_set_eee() nor phylink_ethtool_set_eee() checks
edata->tx_lpi_timer. The value is stored in pl->eee_cfg and
phydev->eee_cfg, and get_eee reports it back. The MAC callback ignores
the timer, though:

drivers/net/usb/ax88179a_devices.c:ax88179a_mac_enable_tx_lpi() {
    ...
	/* AX88179A does not provide LPI timer registers */
	return ax88179_write_cmd(dev, AX_GPHY_CTL, AX_GPHY_EEE_CTRL, true, 0, NULL);
}

The phylink_mac_ops documentation for mac_enable_tx_lpi() says
"Configure the LPI timeout accordingly".

As a result, "ethtool --set-eee ethX tx-timer N" succeeds and
"--show-eee" reports N, but the hardware never uses it.
phylink_config.lpi_timer_default is never set either, so the reported
default timer is 0.

Other drivers whose LPI timer cannot be changed reject this request.
e1000e_set_eee() and ixgbe_validate_keee() both return an error when
tx_lpi_timer differs from the fixed value.

Could the wrapper do the same here? Another option is to set
lpi_timer_default to the real hardware value and refuse changes. The
last commit in the series still has this code unchanged.

> +
>  static const struct ethtool_ops ax88179a_ethtool_ops = {

[ ... ]

> @@ -451,6 +467,10 @@ static int ax88179a_phylink_setup(struct usbnet *dev)
>  		phy_if_mode = PHY_INTERFACE_MODE_SGMII;
>  	}
>  
> +	memcpy(data->phylink_config.lpi_interfaces,
> +	       data->phylink_config.supported_interfaces,
> +	       sizeof(data->phylink_config.lpi_interfaces));
> +

[Severity: Medium]
Does this change the default EEE advertisement on the wire for every
non-AX88772D chip, including AX88279?

Before this patch lpi_interfaces was empty. phylink_create() therefore
set mac_supports_eee to false while mac_supports_eee_ops was true, and
phylink_bringup_phy() called phy_disable_eee().

With lpi_interfaces filled in, mac_supports_eee becomes true. This
earlier setup also applies:

	if (!data->is_ax88772d) {
		data->phylink_config.lpi_capabilities = MAC_100FD | MAC_1000FD;
		data->phylink_config.eee_enabled_default = false;
	}

So pl->eee_cfg.eee_enabled is false, and phylink_bringup_phy() takes
neither branch that changes the PHY's EEE enable:

drivers/net/phy/phylink.c:phylink_bringup_phy() {
    ...
	if (pl->mac_supports_eee) {
		...
		if (pl->eee_cfg.eee_enabled)
			phy_support_eee(phy);
		...
	} else if (pl->mac_supports_eee_ops) {
		/* MAC supports phylink EEE, but wants EEE always disabled. */
		phy_disable_eee(phy);
	}
    ...
}

phydev->eee_cfg.eee_enabled keeps the value phy_probe() derived from the
PHY's power-on advertisement:

	phydev->eee_cfg.eee_enabled = !linkmode_empty(phydev->advertising_eee);

If the internal GPHY advertises EEE at reset, genphy_c45_an_config_eee_aneg()
will keep advertising the 100FD/1000FD EEE modes on every autoneg. That
includes after the PHY power-cycle in resume and reset. The link partner
then negotiates EEE while MAC TX LPI stays off.

The legacy ax88179_178a.c reset path explicitly disables EEE and clears
the EEE advertisement. That suggests this family does advertise EEE by
default, but this was not confirmed for the AX88179A/AX88279 PHYs.

This also seems to conflict with the comment in ax88179a_reset():

	/* Ensure EEE state is off, consistent with phylink setup */
	ax88179_write_cmd(dev, AX_GPHY_CTL, AX_GPHY_EEE_CTRL, false, 0, NULL);

That only clears the MAC-side LPI control. phylink_ethtool_get_eee()
would then report the PHY's eee_enabled as true together with
tx_lpi_enabled as false, which does not match phylink's own eee_cfg.

Was this default change intended? If not, should the PHY's EEE
advertisement be disabled explicitly when eee_enabled_default is false?

The commit message also does not mention that AX88279 gets
phylink-managed EEE (lpi_capabilities is set for all !is_ax88772d chips).
Nor does it mention that AX88772D now has get_eee/set_eee ops that always
return -EOPNOTSUPP. The final commit in the series has the same code, so
nothing later in the series changes this.

>  	phylink = phylink_create(&data->phylink_config, dev->net->dev.fwnode,
>  				 phy_if_mode, &ax88179a_phylink_mac_ops);
>  	if (IS_ERR(phylink))

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-ax88179a-v20-0-37b5a1189983%40birger-koblitz.de