| Message ID | 0c8429c2-7498-efe8-c223-da3d17b1e8e6@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: check for aneg disabled and half duplex in phy_ethtool_set_eee | expand |
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 > > >
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 >> >> >> >
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.
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)
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(+)