From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
Florian Fainelli <f.fainelli@gmail.com>,
David Miller <davem@davemloft.net>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH net-next 1/2] net: phy: check for aneg disabled and half duplex in phy_ethtool_set_eee
Date: Mon, 11 May 2020 14:22:58 +0100 [thread overview]
Message-ID: <20200511132258.GT1551@shell.armlinux.org.uk> (raw)
In-Reply-To: <01a6a1b2-39cc-531a-18be-44a59a5e7441@gmail.com>
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
next prev parent reply other threads:[~2020-05-11 13:23 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-10 8:10 [PATCH net-next 0/2] net: phy: check for aneg disabled and half duplex in phy_ethtool_set_eee Heiner Kallweit
2020-05-10 8:11 ` [PATCH net-next 1/2] " Heiner Kallweit
2020-05-10 14:05 ` Russell King - ARM Linux admin
2020-05-11 12:50 ` Heiner Kallweit
2020-05-11 13:22 ` Russell King - ARM Linux admin [this message]
2020-05-10 8:12 ` [PATCH net-next 2/2] r8169: rely on sanity checks " Heiner Kallweit
2020-05-10 14:08 ` Russell King - ARM Linux admin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200511132258.GT1551@shell.armlinux.org.uk \
--to=linux@armlinux.org.uk \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=hkallweit1@gmail.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.