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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Baseline decision: false positive, because the next patch in the
  series fixes it.
- [Low] Baseline decision: false positive, because the next patch in the
  series fixes it.

--

ax88179_178a: Add EEE configuration support for AX88179A MACs

This commit adds mac_enable_tx_lpi and mac_disable_tx_lpi phylink callbacks
for the AX88179A. They toggle the single AX_GPHY_EEE_CTRL register through
ax88179_write_cmd(). The hardware has no LPI timer registers.

> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index 1aa8479422517..62619fd68c408 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c

[ ... ]

>  static const struct phylink_mac_ops ax88179a_phylink_mac_ops = {
>  	.mac_config = ax88179a_mac_config,
>  	.mac_link_down = ax88179a_mac_link_down,
>  	.mac_link_up = ax88179a_mac_link_up,
> +	.mac_disable_tx_lpi = ax88179a_mac_disable_tx_lpi,
> +	.mac_enable_tx_lpi = ax88179a_mac_enable_tx_lpi,
>  };

[Severity: Low]
With both LPI ops present, phylink_create() sets pl->mac_supports_eee_ops
to true. At this commit, ax88179a_phylink_setup() sets lpi_capabilities but
does not fill phylink_config.lpi_interfaces:

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

phylink_create() requires a non-empty lpi_interfaces:

    pl->mac_supports_eee = pl->mac_supports_eee_ops &&
                           pl->config->lpi_capabilities &&
                           !phy_interface_empty(pl->config->lpi_interfaces);

So mac_supports_eee comes out false. Does phylink_bringup_phy() then take
this branch for every AX88179A/AX88279 device?

    } else if (pl->mac_supports_eee_ops) {
        /* MAC supports phylink EEE, but wants EEE always disabled. */
        phy_disable_eee(phy);

ax88179a_mac_enable_tx_lpi() also looks unreachable here.
phylink_link_up() does not call phylink_activate_lpi() when
mac_supports_eee is false, and phylink_activate_lpi() returns early in any
case:

    if (!test_bit(pl->cur_interface, pl->config->lpi_interfaces)) {

phylink_ethtool_get_eee() and phylink_ethtool_set_eee() would return
-EOPNOTSUPP in this state as well.

The next patch in the series, "ax88179_178a: Add EEE configuration support
for AX88179A PHYs", copies supported_interfaces into lpi_interfaces before
phylink_create(), so this gap only exists at this one commit. The default
behaviour seen by users is unchanged, because the driver already defaults
to EEE off with eee_enabled_default = false.

Would it make sense to fill lpi_interfaces in this patch, so the new
callbacks can be reached at this commit?

[Severity: Low]
At this commit ax88179a_ethtool_ops has no .get_eee or .set_eee entries:

    static const struct ethtool_ops ax88179a_ethtool_ops = {
        ...
        .set_eeprom             = ax88179_set_eeprom,
        .nway_reset             = phy_ethtool_nway_reset,
        ...
    };

So ethtool --set-eee on an AX88179A returns -EOPNOTSUPP. Userspace has no
way to enable EEE through the new mac_enable_tx_lpi/mac_disable_tx_lpi
callbacks. EEE stays off, which matches eee_enabled_default = false.

The next patch, "ax88179_178a: Add EEE configuration support for AX88179A
PHYs", adds ax88179a_get_eee() and ax88179a_set_eee(). These wrap
phylink_ethtool_get_eee() and phylink_ethtool_set_eee() and are wired into
ax88179a_ethtool_ops.

Is splitting the MAC side and the ethtool side across these two patches
intended?

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