* [PATCH net-next 0/2] net: phy: check for aneg disabled and half duplex in phy_ethtool_set_eee @ 2020-05-10 8:10 Heiner Kallweit 2020-05-10 8:11 ` [PATCH net-next 1/2] " Heiner Kallweit 2020-05-10 8:12 ` [PATCH net-next 2/2] r8169: rely on sanity checks " Heiner Kallweit 0 siblings, 2 replies; 7+ messages in thread From: Heiner Kallweit @ 2020-05-10 8:10 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, Russell King - ARM Linux, David Miller Cc: netdev EEE requires aneg and full duplex, therefore return EPROTONOSUPPORT if aneg is disabled or aneg resulted in a half duplex mode. Heiner Kallweit (2): net: phy: check for aneg disabled and half duplex in phy_ethtool_set_eee r8169: rely on sanity checks in phy_ethtool_set_eee drivers/net/ethernet/realtek/r8169_main.c | 6 ------ drivers/net/phy/phy.c | 3 +++ 2 files changed, 3 insertions(+), 6 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next 1/2] net: phy: check for aneg disabled and half duplex in phy_ethtool_set_eee 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 ` Heiner Kallweit 2020-05-10 14:05 ` Russell King - ARM Linux admin 2020-05-10 8:12 ` [PATCH net-next 2/2] r8169: rely on sanity checks " Heiner Kallweit 1 sibling, 1 reply; 7+ messages in thread From: Heiner Kallweit @ 2020-05-10 8:11 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, Russell King - ARM Linux, David Miller Cc: netdev EEE requires aneg and full duplex, therefore return EPROTONOSUPPORT if aneg is disabled or aneg resulted in a half duplex mode. Signed-off-by: Heiner Kallweit <hkallweit1@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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: check for aneg disabled and half duplex in phy_ethtool_set_eee 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 0 siblings, 1 reply; 7+ messages in thread From: Russell King - ARM Linux admin @ 2020-05-10 14:05 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev 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. 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? Why should we force people to negotiate a FD link before they can then configure EEE, and then have to perform a renegotiation? 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 <hkallweit1@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 > > > -- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: check for aneg disabled and half duplex in phy_ethtool_set_eee 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 0 siblings, 1 reply; 7+ messages in thread From: Heiner Kallweit @ 2020-05-11 12:50 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev 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 <hkallweit1@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 >> >> >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: check for aneg disabled and half duplex in phy_ethtool_set_eee 2020-05-11 12:50 ` Heiner Kallweit @ 2020-05-11 13:22 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 7+ messages in thread From: Russell King - ARM Linux admin @ 2020-05-11 13:22 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next 2/2] r8169: rely on sanity checks in phy_ethtool_set_eee 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 8:12 ` Heiner Kallweit 2020-05-10 14:08 ` Russell King - ARM Linux admin 1 sibling, 1 reply; 7+ messages in thread From: Heiner Kallweit @ 2020-05-10 8:12 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, Russell King - ARM Linux, David Miller Cc: netdev These checks are integrated in phy_ethtool_set_eee() now, therefore we can remove them from the driver. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 0e96d0de2..966192ef0 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -1919,12 +1919,6 @@ static int rtl8169_set_eee(struct net_device *dev, struct ethtool_eee *data) goto out; } - if (dev->phydev->autoneg == AUTONEG_DISABLE || - dev->phydev->duplex != DUPLEX_FULL) { - ret = -EPROTONOSUPPORT; - goto out; - } - ret = phy_ethtool_set_eee(tp->phydev, data); if (!ret) -- 2.26.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/2] r8169: rely on sanity checks in phy_ethtool_set_eee 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 0 siblings, 0 replies; 7+ messages in thread From: Russell King - ARM Linux admin @ 2020-05-10 14:08 UTC (permalink / raw) To: Heiner Kallweit; +Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev On Sun, May 10, 2020 at 10:12:40AM +0200, Heiner Kallweit wrote: > These checks are integrated in phy_ethtool_set_eee() now, therefore we > can remove them from the driver. Ah, so one NIC driver has a problem with this, so we have to apply that pain to all NIC drivers? No thanks. Have you looked at how the driver decides whether EEE should be enabled? Does it check that the link negotiated FD prior to enabling EEE? That is likely where the problem is. Throwing big hammers to prevent the user configuring EEE settings on any driver just because one driver has a problem is really not a sane approach, and in fact I can tell you now that I will be pointing out a userspace regression as a result of your patches - I can tell you now, your patch 1 _will_ regress my userspace. -- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-11 13:23 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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.