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

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

[Heiner Kallweit]

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

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

[Heiner Kallweit]

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

[PATCH net-next 2/2] r8169: rely on sanity checks in phy_ethtool_set_eee

[Heiner Kallweit]

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

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]

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

Re: [PATCH net-next 2/2] r8169: rely on sanity checks in phy_ethtool_set_eee

[Russell King - ARM Linux admin]

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

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

[Heiner Kallweit]

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

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]

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