Re: [PATCH RFC 00/18] Rework MAC drivers EEE support

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Feb 17, 2023 at 04:42:12AM +0100, Andrew Lunn wrote:
> phy_init_eee() is supposed to be called once auto-neg has been
> completed to determine if EEE should be used with the current link
> mode. The MAC hardware should then be configured to either enable or
> disable EEE. Many drivers get this wrong, calling phy_init_eee() once,
> or only in the ethtool set_eee callback.

Looking at some of the recent EEE changes (not related to this patch
set) I've come across:

commit 9b01c885be364526d8c05794f8358b3e563b7ff8
Author: Oleksij Rempel <linux@rempel-privat.de>
Date:   Sat Feb 11 08:41:10 2023 +0100

    net: phy: c22: migrate to genphy_c45_write_eee_adv()

This part of the patch is wrong:

__genphy_config_aneg():
-       if (genphy_config_eee_advert(phydev))
+       err = genphy_c45_write_eee_adv(phydev, phydev->supported_eee);

The problem here is that these are not equivalent.

genphy_config_eee_advert() only clears the broken EEE modes in the
advertisement, it doesn't actually set the advertisement to anything
in particular.

The replacement code _configures_ the advertisement to whatever the
second argument is, which means each time the advertisement is
changed (and thus __genphy_config_aneg() is called) the EEE
advertisement will ignore whatever the user configured via the
set_eee() APIs, and be restored to the full EEE capabilities in the
supported mask.

This is an obvious regression that needs fixing, especially as the
merge window is potentially due to open this weekend.

Moreover, it looks like Oleksij's patch was not Cc'd to me (I can't
find it in my mailbox) and as I'm listed in MAINTAINERS for phylib,
this should have been brought up _before_ Oleksij's patch was
applied to net-next (despite me being unlikely to reply to it due
to covid, it still would be nice to have reviewed it, or even
reply to the damn patch about these concerns.) But I'm having to
pick some other damn series to bring up this concern.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!