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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 5

- [Low] Commit message of "ax88179_178a: Add EEE configuration support
  for AX88179A MACs" ends in a truncated/garbled sentence describing…
- [Low] Concern (validated as resolved later in the series): registering
  .mac_enable_tx_lpi/.mac_disable_tx_lpi makes phylink_create() set…
- [Low] Concern (validated as resolved later in the series):
  ax88179a_ethtool_ops has no .get_eee/.set_eee entries at this commit,…
- [Low] Concern (validated as not a defect): the LPI ops are added
  unconditionally to the single ax88179a_phylink_mac_ops table shared by…
- [Low] Concern (validated as speculative/non-actionable):
  AX_GPHY_EEE_CTRL is written only from the two new phylink LPI…

--

ax88179_178a: Add EEE configuration support for AX88179A MACs

This adds two phylink LPI callbacks, ax88179a_mac_enable_tx_lpi() and
ax88179a_mac_disable_tx_lpi(), which toggle the AX_GPHY_EEE_CTRL register,
and registers them in ax88179a_phylink_mac_ops.  The AX88179A has no LPI
timer registers, so the timer argument is ignored.

A few questions below, all minor.

>     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 truncated: "Add support for this EEE
enable/disable register and replace the EEE."

What is being replaced here?  The diff is purely additive: it adds
ax88179a_mac_disable_tx_lpi(), ax88179a_mac_enable_tx_lpi() and two
members of ax88179a_phylink_mac_ops, and deletes nothing.  The legacy
get_eee/set_eee and ax88179_enable_eee() code in ax88179_178a.c is left
untouched.

Could the sentence be completed so the changelog matches the change?

> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index 21ca71cba52ca..80cabe3557cde 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: Low]
AX_GPHY_EEE_CTRL is only ever written from these two new callbacks, so it is
never forced to a known value during ax88179a_bind()/ax88179a_reset().  If the
chip powers up with EEE enabled, does the hardware state match phylink's
bookkeeping until the first link up/down transition?

Would an explicit disable write in the reset path be worth adding?

On the ignored return value here: the phylink prototype is
void (*mac_disable_tx_lpi)(struct phylink_config *), so there is no way to
propagate a failed USB control transfer, and the resume path does re-resolve
the link (ax88179a_resume() -> ax88179a_reset() -> phylink_resume(), which
either calls phylink_start() or phylink_link_down() plus
phylink_mac_initial_config() and phylink_enable_and_run_resolve()), so the
register state is re-established on a link cycle.  Mentioning it only for
completeness.

> +
> +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]
Registering both LPI ops makes phylink_create() set
pl->mac_supports_eee_ops = true, but ax88179a_phylink_setup() does not
populate phylink_config.lpi_interfaces at this commit, so phylink_create()
leaves mac_supports_eee false:

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);
}

In that state phylink_bringup_phy() takes the branch that calls
phy_disable_eee(phy), phylink_ethtool_get_eee()/set_eee() return
-EOPNOTSUPP, and the two new callbacks are never invoked, since
phylink_link_up() gates on mac_supports_eee and phylink_activate_lpi()
additionally tests config->lpi_interfaces.

This is resolved by the following patch in the series, "ax88179_178a: Add EEE
configuration support for AX88179A PHYs", which adds the memcpy of
supported_interfaces into lpi_interfaces before phylink_create(), so it only
affects the intermediate state.  Would it be worth folding the
lpi_interfaces assignment into this patch so bisection points do not have EEE
forced off?

[Severity: Low]
Related to the above: at this commit ax88179a_ethtool_ops has no .get_eee or
.set_eee entries, and ax88179a_phylink_setup() sets
phylink_config.eee_enabled_default = false, so nothing can set
pl->eee_cfg.eee_enabled and ax88179a_mac_enable_tx_lpi() cannot be reached
from userspace.

The ethtool wrappers ax88179a_get_eee()/ax88179a_set_eee() arrive in the next
patch of the series, so this too is limited to the intermediate state.

[Severity: Low]
The ops are added unconditionally to the single mac_ops table shared by
AX88179A, AX88279 and AX88772D, while ax88179a_phylink_setup() sets
lpi_capabilities only for !is_ax88772d.

For AX88772D that combination looks intentional and is handled by phylink:
phylink_bringup_phy() documents ops present with empty lpi_capabilities as
"MAC supports phylink EEE, but wants EEE always disabled", and no
AX_GPHY_EEE_CTRL write happens for that chip.

Is the AX_GPHY_EEE_CTRL encoding the same on AX88279, or does that part need
a chip_version guard?

>  };
>  
>  static int ax88179a_phylink_setup(struct usbnet *dev)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914-ax88179a-v11-0-5ea7a925ba6d%40birger-koblitz.de