Re: [PATCH net-next v12 07/15] ax88179_178a: Add EEE configuration support for AX88179A MACs

[netdev-bot+sashiko]

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

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

- [Medium] The AX179A EEE control register (AX_GPHY_CTL /
  AX_GPHY_EEE_CTRL) is never forced to a known state, while the driver…
- [Low] In this patch alone, ax88179a_phylink_mac_ops gains
  .mac_enable_tx_lpi/.mac_disable_tx_lpi while ax88179a_phylink_setup()…
- [Low] The changelog of "ax88179_178a: Add EEE configuration support for
  AX88179A MACs" ends with the truncated/incorrect sentence "Add support…
- [Low] At this commit, ax88179a_ethtool_ops
  (drivers/net/usb/ax88179a_devices.c:209) has no .get_eee/.set_eee…

--

ax88179_178a: Add EEE configuration support for AX88179A MACs

This adds mac_enable_tx_lpi and mac_disable_tx_lpi callbacks to
ax88179a_phylink_mac_ops, which write the single AX_GPHY_EEE_CTRL register
of the AX88179A family.  The chip has no LPI timer registers, so the timer
argument is ignored.

A couple of questions below.

> ax88179_178a: Add EEE configuration support for AX88179A MACs
>
>     The AX88179A uses a simple HW configuration for EEE
>     via a single EEE configuration register without LPI timer support
>     Add support for this EEE enable/disable register and replace the EEE.

[Severity: Low]
The last sentence looks unfinished: "Add support for this EEE
enable/disable register and replace the EEE."  What is being replaced?
The diff is purely additive (17 insertions, no deletions) and
ax88179a_devices.c has no prior EEE handling apart from the
eee_enabled_default assignment in ax88179a_phylink_setup().

Also, the subject and body mention only AX88179A MACs, but
ax88179a_phylink_mac_ops is the single ops table passed to
phylink_create() by ax88179a_phylink_setup() for every chip handled by
this file, including AX88772D/E and AX88279.  Could the message state
that these chips share the same single-register EEE semantics?

> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index 5dac7a137c323..785919e14e861 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
> @@ -384,10 +384,27 @@ static void ax88179a_mac_link_up(struct phylink_config *config,
>  	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_MAC_PATH, 1, 1, &tmp8);
>  }
>  
> +static void ax88179a_mac_disable_tx_lpi(struct phylink_config *config)
> +{
> +	struct usbnet *dev = netdev_priv(to_net_dev(config->dev));
> +
> +	ax88179_write_cmd(dev, AX_GPHY_CTL, AX_GPHY_EEE_CTRL, false, 0, NULL);
> +}

[Severity: Medium]
Is AX_GPHY_EEE_CTRL ever brought to a known state at bind or reset?

Neither ax88179a_bind() nor ax88179a_reset() writes AX_GPHY_EEE_CTRL, and
these two new callbacks are its only writers.  Meanwhile
ax88179a_phylink_setup() tells phylink that EEE is off:

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

phylink cannot clear it either, because the disable path only runs for a
bit phylink itself set:

drivers/net/phy/phylink.c:phylink_deactivate_lpi() {
	if (pl->mac_enable_tx_lpi) {
		pl->mac_enable_tx_lpi = false;
		...
		pl->mac_ops->mac_disable_tx_lpi(pl->config);
}

So if the chip or firmware power-on default of AX_GPHY_EEE_CTRL is
"enabled", does the MAC stay in LPI mode while phylink and ethtool
--show-eee report EEE as disabled?

The older MAC path in this driver does force the state explicitly:
ax88179_reset() calls ax88179_disable_eee().  lan78xx does the same by
clearing MAC_CR_EEE_EN_ during probe, and lan78xx_mac_enable_tx_lpi()
documents that it relies on that.  Would an explicit write of
AX_GPHY_EEE_CTRL to false in ax88179a_reset() be appropriate here?

> +
> +static int ax88179a_mac_enable_tx_lpi(struct phylink_config *config, u32 timer, bool tx_clk_stop)
> +{
> +	struct usbnet *dev = netdev_priv(to_net_dev(config->dev));
> +
> +	/* AX88179A does not provide LPI timer registers */
> +	return ax88179_write_cmd(dev, AX_GPHY_CTL, AX_GPHY_EEE_CTRL, true, 0, NULL);
> +}
> +
>  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 this commit applied on its own, does registering these two callbacks
have the opposite effect of what is intended?

ax88179a_phylink_setup() sets lpi_capabilities but leaves
phylink_config.lpi_interfaces zeroed (the config is zero-allocated in
ax88179a_bind()), and phylink gates all MAC EEE handling on that bitmap:

drivers/net/phy/phylink.c:phylink_create() {
	pl->mac_supports_eee = pl->mac_supports_eee_ops &&
			       pl->config->lpi_capabilities &&
			       !phy_interface_empty(pl->config->lpi_interfaces);
}

So mac_supports_eee_ops becomes true while mac_supports_eee stays false.
ax88179a_mac_enable_tx_lpi() is then unreachable, since phylink_link_up()
only calls phylink_activate_lpi() when pl->mac_supports_eee, and
phylink_bringup_phy() takes this branch instead:

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

This resolves later in the same series: the following patch,
"ax88179_178a: Add EEE configuration support for AX88179A PHYs", adds the
memcpy() of supported_interfaces into lpi_interfaces in
ax88179a_phylink_setup(), after which mac_supports_eee becomes true.  Is
it worth folding that lpi_interfaces initialisation into this patch so the
intermediate state does not force PHY EEE off?

[Severity: Low]
In the same vein, at this commit ax88179a_ethtool_ops has no .get_eee or
.set_eee, and eee_enabled_default is false, so nothing can set
pl->eee_cfg.tx_lpi_enabled and ethtool EEE requests are rejected by the
core.

The following patch in the series adds ax88179a_get_eee() and
ax88179a_set_eee() forwarding to phylink_ethtool_get_eee() and
phylink_ethtool_set_eee() and wires them into ax88179a_ethtool_ops, so the
user-visible path only appears there.  Would it read better to add the
ethtool plumbing together with the callbacks that it drives?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916-ax88179a-v12-0-60c04c9924a2%40birger-koblitz.de