Re: [PATCH net-next 1/2] net: phy: check for aneg disabled and half duplex in phy_ethtool_set_eee

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Mon, May 11, 2020 at 02:50:23PM +0200, Heiner Kallweit wrote:
> 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?

ethtool with a debian interfaces file.  I have systems which are
configured thusly:

iface eno0 inet dhcp
	pre-up ip link set $IFACE up
	pre-up ethtool --set-eee $IFACE advertise 0x28

So, if you decide to fail the call ethtool makes to configure EEE
because the link happens to have negotiated half-duplex mode, the
second command will fail, which prevent Debian bringing up this
interface.  That will be a userspace regression over how it behaves
today.

> > 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?

If we want to change EEE configuration, and autoneg is disabled, why
should we forcefully re-enable it?  How are these different scenarios?

ethtool --set-eee $IFACE advertise 0x28
ethtool -s $IFACE autoneg off speed 100 duplex full
ethtool -s $IFACE autoneg on

vs

ethtool -s $IFACE autoneg off speed 100 duplex full
ethtool --set-eee $IFACE advertise 0x28
ethtool -s $IFACE autoneg on

Why should we fail in this case when all we are doing is configuring
the advertisment?

> > 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).

I think you grossly misunderstand this interface.  This interface is
to configure the _circumstances_ under which EEE _may_ be enabled.
It doesn't say "I want EEE to be active right this damn nanosecond."

Hence, I'm NAKing this patch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up