On 10.05.2020 16:05, Russell King - ARM Linux admin wrote: > On Sun, May 10, 2020 at 10:11:33AM +0200, Heiner Kallweit wrote: >> EEE requires aneg and full duplex, therefore return EPROTONOSUPPORT >> if aneg is disabled or aneg resulted in a half duplex mode. > > I think this is completely wrong. This is the ethtool configuration > interface for EEE that you're making fail. > You mentioned in a parallel response that you are aware of at least userspace tool / use case that would be broken by this change. Can you please point me to this tool / use case?
> Why should you not be able to configure EEE parameters if the link > happens to negotiated a half-duplex? Why should you not be able to > adjust the EEE advertisment via ethtool if the link has negotiated > half-duplex? > > Why should any of this configuration depend on the current state? > If EEE settings change, then phy_ethtool_set_eee() eventually calls genphy_restart_aneg() which sets bits BMCR_ANENABLE in the chip. Means if we enter the function with phydev->autoneg being cleared, then we'll end up with an inconsistent state (phydev->autoneg not reflecting chip aneg setting). As alternative to throwing an error we could skip triggering an aneg, what would you prefer? > Why should we force people to negotiate a FD link before they can > then configure EEE, and then have to perform a renegotiation? > If being in a HD mode and setting EEE returns with a success return code, then users may expect EEE to be active (what it is not). > Sorry, but to me this patch seems to be a completely wrong approach, > and I really don't get what problem it is trying to fix. > >> >> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com> >> --- >> drivers/net/phy/phy.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >> index 8c22d02b4..891bb6929 100644 >> --- a/drivers/net/phy/phy.c >> +++ b/drivers/net/phy/phy.c >> @@ -1110,6 +1110,9 @@ int phy_ethtool_set_eee(struct phy_device *phydev, >> struct ethtool_eee *data) >> if (!phydev->drv) >> return -EIO; >> >> + if (phydev->autoneg == AUTONEG_DISABLE || phydev->duplex == DUPLEX_HALF) >> + return -EPROTONOSUPPORT; >> + >> /* Get Supported EEE */ >> cap = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE); >> if (cap < 0) >> -- >> 2.26.2 >> >> >> >
