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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200511132258.GT1551@shell.armlinux.org.uk>
Date:   Mon, 11 May 2020 14:22:58 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/2] net: phy: check for aneg disabled and half
 duplex in phy_ethtool_set_eee

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