[PATCH net-next v1 0/4] net: phy: EEE fixes

[PATCH net-next v1 0/4] net: phy: EEE fixes

[Oleksij Rempel]

Different EEE related fixes.

Oleksij Rempel (4):
  net: phy: c45: use "supported_eee" instead of supported for access
    validation
  net: phy: c45: add genphy_c45_an_config_eee_aneg() function
  net: phy: do not force EEE support
  net: phy: c45: genphy_c45_ethtool_set_eee: validate EEE link modes

 drivers/net/phy/phy-c45.c    | 46 +++++++++++++++++++++++++++---------
 drivers/net/phy/phy_device.c | 21 +++++++++++++++-
 include/linux/phy.h          |  6 +++++
 3 files changed, 61 insertions(+), 12 deletions(-)

-- 
2.30.2

[PATCH net-next v1 1/4] net: phy: c45: use "supported_eee" instead of supported for access validation

[Oleksij Rempel]

Make use we use proper variable to validate access to potentially not
supported registers. Otherwise we will get false read/write errors.

Fixes: 022c3f87f88e ("net: phy: add genphy_c45_ethtool_get/set_eee() support")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/phy-c45.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index f9b128cecc3f..f23cce2c5199 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -674,7 +674,7 @@ int genphy_c45_write_eee_adv(struct phy_device *phydev, unsigned long *adv)
 {
 	int val, changed;
 
-	if (linkmode_intersects(phydev->supported, PHY_EEE_CAP1_FEATURES)) {
+	if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP1_FEATURES)) {
 		val = linkmode_to_mii_eee_cap1_t(adv);
 
 		/* In eee_broken_modes are stored MDIO_AN_EEE_ADV specific raw
@@ -726,7 +726,7 @@ static int genphy_c45_read_eee_adv(struct phy_device *phydev,
 {
 	int val;
 
-	if (linkmode_intersects(phydev->supported, PHY_EEE_CAP1_FEATURES)) {
+	if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP1_FEATURES)) {
 		/* IEEE 802.3-2018 45.2.7.13 EEE advertisement 1
 		 * (Register 7.60)
 		 */
@@ -762,7 +762,7 @@ static int genphy_c45_read_eee_lpa(struct phy_device *phydev,
 {
 	int val;
 
-	if (linkmode_intersects(phydev->supported, PHY_EEE_CAP1_FEATURES)) {
+	if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP1_FEATURES)) {
 		/* IEEE 802.3-2018 45.2.7.14 EEE link partner ability 1
 		 * (Register 7.61)
 		 */
-- 
2.30.2

[PATCH net-next v1 2/4] net: phy: c45: add genphy_c45_an_config_eee_aneg() function

[Oleksij Rempel]

Add new genphy_c45_an_config_eee_aneg() function and replace some of
genphy_c45_write_eee_adv() calls. This will be needed by the next patch.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/phy-c45.c    | 12 +++++++++++-
 drivers/net/phy/phy_device.c |  2 +-
 include/linux/phy.h          |  1 +
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index f23cce2c5199..904f64818922 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -262,7 +262,7 @@ int genphy_c45_an_config_aneg(struct phy_device *phydev)
 	linkmode_and(phydev->advertising, phydev->advertising,
 		     phydev->supported);
 
-	ret = genphy_c45_write_eee_adv(phydev, phydev->supported_eee);
+	ret = genphy_c45_an_config_eee_aneg(phydev);
 	if (ret < 0)
 		return ret;
 	else if (ret)
@@ -858,6 +858,16 @@ int genphy_c45_read_eee_abilities(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(genphy_c45_read_eee_abilities);
 
+/**
+ * genphy_c45_an_config_eee_aneg - write advertised EEE link modes
+ * @phydev: target phy_device struct
+ * @adv: the linkmode advertisement settings
+ */
+int genphy_c45_an_config_eee_aneg(struct phy_device *phydev)
+{
+	return genphy_c45_write_eee_adv(phydev, phydev->supported_eee);
+}
+
 /**
  * genphy_c45_pma_read_abilities - read supported link modes from PMA
  * @phydev: target phy_device struct
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8d927c5e3bf8..0c47665effaf 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2231,7 +2231,7 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed)
 {
 	int err;
 
-	err = genphy_c45_write_eee_adv(phydev, phydev->supported_eee);
+	err = genphy_c45_an_config_eee_aneg(phydev);
 	if (err < 0)
 		return err;
 	else if (err)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 727bff531a14..19d83e112beb 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1765,6 +1765,7 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 			       struct ethtool_eee *data);
 int genphy_c45_write_eee_adv(struct phy_device *phydev, unsigned long *adv);
+int genphy_c45_an_config_eee_aneg(struct phy_device *phydev);
 
 /* Generic C45 PHY driver */
 extern struct phy_driver genphy_c45_driver;
-- 
2.30.2

[PATCH net-next v1 3/4] net: phy: do not force EEE support

[Oleksij Rempel]

With following patches:
commit 9b01c885be36 ("net: phy: c22: migrate to genphy_c45_write_eee_adv()")
commit 5827b168125d ("net: phy: c45: migrate to genphy_c45_write_eee_adv()")

we set the advertisement to potentially supported values. This behavior
may introduce new regressions on systems where EEE was disabled by
default (BIOS or boot loader configuration or by other ways.)

At same time, with this patches, we would overwrite EEE advertisement
configuration made over ethtool.

To avoid this issues, we need to cache initial and ethtool advertisement
configuration and store it for later use.

Fixes: 9b01c885be36 ("net: phy: c22: migrate to genphy_c45_write_eee_adv()")
Fixes: 5827b168125d ("net: phy: c45: migrate to genphy_c45_write_eee_adv()")
Fixes: 022c3f87f88e ("net: phy: add genphy_c45_ethtool_get/set_eee() support")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/phy-c45.c    | 20 ++++++++++++--------
 drivers/net/phy/phy_device.c | 19 +++++++++++++++++++
 include/linux/phy.h          |  5 +++++
 3 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 904f64818922..974d6e96fc71 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -721,8 +721,7 @@ int genphy_c45_write_eee_adv(struct phy_device *phydev, unsigned long *adv)
  * @phydev: target phy_device struct
  * @adv: the linkmode advertisement status
  */
-static int genphy_c45_read_eee_adv(struct phy_device *phydev,
-				   unsigned long *adv)
+int genphy_c45_read_eee_adv(struct phy_device *phydev, unsigned long *adv)
 {
 	int val;
 
@@ -865,7 +864,12 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_eee_abilities);
  */
 int genphy_c45_an_config_eee_aneg(struct phy_device *phydev)
 {
-	return genphy_c45_write_eee_adv(phydev, phydev->supported_eee);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
+
+	if (!phydev->eee_enabled)
+		return genphy_c45_write_eee_adv(phydev, adv);
+
+	return genphy_c45_write_eee_adv(phydev, phydev->advertising_eee);
 }
 
 /**
@@ -1431,17 +1435,17 @@ EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
 int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 			       struct ethtool_eee *data)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
 	int ret;
 
 	if (data->eee_enabled) {
+		phydev->eee_enabled = true;
 		if (data->advertised)
-			adv[0] = data->advertised;
-		else
-			linkmode_copy(adv, phydev->supported_eee);
+			phydev->advertising_eee[0] = data->advertised;
+	} else {
+		phydev->eee_enabled = false;
 	}
 
-	ret = genphy_c45_write_eee_adv(phydev, adv);
+	ret = genphy_c45_an_config_eee_aneg(phydev);
 	if (ret < 0)
 		return ret;
 	if (ret > 0)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0c47665effaf..ef2a9f079916 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3139,6 +3139,25 @@ static int phy_probe(struct device *dev)
 	of_set_phy_supported(phydev);
 	phy_advertise_supported(phydev);
 
+	/* Get PHY default EEE advertising modes and handle them as potentially
+	 * safe initial configuration.
+	 */
+	err = genphy_c45_read_eee_adv(phydev, phydev->advertising_eee);
+	if (err)
+		return err;
+
+	/* There is no "enabled" flag. If PHY is advertising, assume it is
+	 * kind of enabled.
+	 */
+	phydev->eee_enabled = !linkmode_empty(phydev->advertising_eee);
+
+	/* Some PHYs may advertise, by default, not support EEE modes. So,
+	 * we need to clean them.
+	 */
+	if (phydev->eee_enabled)
+		linkmode_and(phydev->advertising_eee, phydev->supported_eee,
+			     phydev->advertising_eee);
+
 	/* Get the EEE modes we want to prohibit. We will ask
 	 * the PHY stop advertising these mode later on
 	 */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 19d83e112beb..36bf0bbc8efa 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -575,6 +575,8 @@ struct macsec_ops;
  * @advertising: Currently advertised linkmodes
  * @adv_old: Saved advertised while power saving for WoL
  * @supported_eee: supported PHY EEE linkmodes
+ * @advertising_eee: Currently advertised EEE linkmodes
+ * @eee_enabled: Flag indicating whether the EEE feature is enabled
  * @lp_advertising: Current link partner advertised linkmodes
  * @host_interfaces: PHY interface modes supported by host
  * @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
@@ -681,6 +683,8 @@ struct phy_device {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv_old);
 	/* used for eee validation */
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported_eee);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising_eee);
+	bool eee_enabled;
 
 	/* Host supported PHY interface types. Should be ignored if empty. */
 	DECLARE_PHY_INTERFACE_MASK(host_interfaces);
@@ -1766,6 +1770,7 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 			       struct ethtool_eee *data);
 int genphy_c45_write_eee_adv(struct phy_device *phydev, unsigned long *adv);
 int genphy_c45_an_config_eee_aneg(struct phy_device *phydev);
+int genphy_c45_read_eee_adv(struct phy_device *phydev, unsigned long *adv);
 
 /* Generic C45 PHY driver */
 extern struct phy_driver genphy_c45_driver;
-- 
2.30.2

[PATCH net-next v1 4/4] net: phy: c45: genphy_c45_ethtool_set_eee: validate EEE link modes

[Oleksij Rempel]

Currently, it is possible to let some PHYs to advertise not supported
EEE link modes. So, validate them before overwriting existing
configuration.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/phy-c45.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 974d6e96fc71..34efade0e2c3 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1439,8 +1439,18 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 
 	if (data->eee_enabled) {
 		phydev->eee_enabled = true;
-		if (data->advertised)
+		if (data->advertised) {
+			__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
+
+			adv[0] = data->advertised;
+			linkmode_andnot(adv, adv, phydev->supported_eee);
+			if (!linkmode_empty(adv)) {
+				phydev_warn(phydev, "At least some EEE link modes are not supported.\n");
+				return -EINVAL;
+			}
+
 			phydev->advertising_eee[0] = data->advertised;
+		}
 	} else {
 		phydev->eee_enabled = false;
 	}
-- 
2.30.2

Re: [PATCH net-next v1 1/4] net: phy: c45: use "supported_eee" instead of supported for access validation

[Russell King (Oracle)]

On Mon, Feb 20, 2023 at 02:56:02PM +0100, Oleksij Rempel wrote:
> Make use we use proper variable to validate access to potentially not
> supported registers. Otherwise we will get false read/write errors.
> 
> Fixes: 022c3f87f88e ("net: phy: add genphy_c45_ethtool_get/set_eee() support")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Looks good to me.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v1 2/4] net: phy: c45: add genphy_c45_an_config_eee_aneg() function

[Russell King (Oracle)]

On Mon, Feb 20, 2023 at 02:56:03PM +0100, Oleksij Rempel wrote:
> Add new genphy_c45_an_config_eee_aneg() function and replace some of
> genphy_c45_write_eee_adv() calls. This will be needed by the next patch.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v1 3/4] net: phy: do not force EEE support

[Russell King (Oracle)]

Hi,

A couple of minor points, but not sufficient not to prevent merging
this.

On Mon, Feb 20, 2023 at 02:56:04PM +0100, Oleksij Rempel wrote:
> @@ -865,7 +864,12 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_eee_abilities);
>   */
>  int genphy_c45_an_config_eee_aneg(struct phy_device *phydev)
>  {
> -	return genphy_c45_write_eee_adv(phydev, phydev->supported_eee);
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};

It would be nice to avoid this initialisation in the case where
eee_enabled is true, as this takes CPU cycles. However, not too
bothered about it as this isn't a fast path.

> +
> +	if (!phydev->eee_enabled)
> +		return genphy_c45_write_eee_adv(phydev, adv);
> +
> +	return genphy_c45_write_eee_adv(phydev, phydev->advertising_eee);
>  }
>  
>  /**
> @@ -1431,17 +1435,17 @@ EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
>  int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
>  			       struct ethtool_eee *data)
>  {
> -	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
>  	int ret;
>  
>  	if (data->eee_enabled) {
> +		phydev->eee_enabled = true;
>  		if (data->advertised)
> -			adv[0] = data->advertised;
> -		else
> -			linkmode_copy(adv, phydev->supported_eee);
> +			phydev->advertising_eee[0] = data->advertised;

Is there a reason not to use ethtool_convert_legacy_u32_to_link_mode()?
I'm guessing this will be more efficient.

Other than that, looks good.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v1 3/4] net: phy: do not force EEE support

[Russell King (Oracle)]

On Mon, Feb 20, 2023 at 02:56:04PM +0100, Oleksij Rempel wrote:
>  	if (data->eee_enabled) {
> +		phydev->eee_enabled = true;
>  		if (data->advertised)
> -			adv[0] = data->advertised;
> -		else
> -			linkmode_copy(adv, phydev->supported_eee);
> +			phydev->advertising_eee[0] = data->advertised;

There is a behavioural change here that isn't mentioned in the patch
description - if data->advertised was zero, you were setting the
link modes to the full EEE supported set. After this patch, you
appear to leave advertising_eee untouched (so whatever it was.)

Which is the correct behaviour for this interface?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v1 3/4] net: phy: do not force EEE support

[Oleksij Rempel]

On Mon, Feb 20, 2023 at 02:21:42PM +0000, Russell King (Oracle) wrote:
> On Mon, Feb 20, 2023 at 02:56:04PM +0100, Oleksij Rempel wrote:
> >  	if (data->eee_enabled) {
> > +		phydev->eee_enabled = true;
> >  		if (data->advertised)
> > -			adv[0] = data->advertised;
> > -		else
> > -			linkmode_copy(adv, phydev->supported_eee);
> > +			phydev->advertising_eee[0] = data->advertised;
> 
> There is a behavioural change here that isn't mentioned in the patch
> description - if data->advertised was zero, you were setting the
> link modes to the full EEE supported set. After this patch, you
> appear to leave advertising_eee untouched (so whatever it was.)
> 
> Which is the correct behaviour for this interface?

Hm.. ethtool do not provide enough information about expected behavior.
Here is my expectation:
- "ethtool --set-eee lan1 eee on" should enable EEE if it is disabled.
- "ethtool --set-eee lan1 advertise 0x10" should change set of
  advertised modes.
- a sequence of "..advertise 0x10", "..eee on", "eee off" should restore
  preconfigured advertise modes. advertising_eee instead of
  supported_eee.

Seems like there is still one missing use case: if advertising_eee is
zero, we should use supported_eee value for "...eee on" case.

Other ideas?
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 3/4] net: phy: do not force EEE support

[Oleksij Rempel]

On Mon, Feb 20, 2023 at 02:18:37PM +0000, Russell King (Oracle) wrote:
> Hi,
> 
> A couple of minor points, but not sufficient not to prevent merging
> this.
> 
> On Mon, Feb 20, 2023 at 02:56:04PM +0100, Oleksij Rempel wrote:
> > @@ -865,7 +864,12 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_eee_abilities);
> >   */
> >  int genphy_c45_an_config_eee_aneg(struct phy_device *phydev)
> >  {
> > -	return genphy_c45_write_eee_adv(phydev, phydev->supported_eee);
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
> 
> It would be nice to avoid this initialisation in the case where
> eee_enabled is true, as this takes CPU cycles. However, not too
> bothered about it as this isn't a fast path.
> 
> > +
> > +	if (!phydev->eee_enabled)
> > +		return genphy_c45_write_eee_adv(phydev, adv);
> > +
> > +	return genphy_c45_write_eee_adv(phydev, phydev->advertising_eee);
> >  }
> >  
> >  /**
> > @@ -1431,17 +1435,17 @@ EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
> >  int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
> >  			       struct ethtool_eee *data)
> >  {
> > -	__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
> >  	int ret;
> >  
> >  	if (data->eee_enabled) {
> > +		phydev->eee_enabled = true;
> >  		if (data->advertised)
> > -			adv[0] = data->advertised;
> > -		else
> > -			linkmode_copy(adv, phydev->supported_eee);
> > +			phydev->advertising_eee[0] = data->advertised;
> 
> Is there a reason not to use ethtool_convert_legacy_u32_to_link_mode()?
> I'm guessing this will be more efficient.

Or at leas more readable. I'll update it.
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 3/4] net: phy: do not force EEE support

[Andrew Lunn]

> > >  		if (data->advertised)
> > > -			adv[0] = data->advertised;
> > > -		else
> > > -			linkmode_copy(adv, phydev->supported_eee);
> > > +			phydev->advertising_eee[0] = data->advertised;
> > 
> > Is there a reason not to use ethtool_convert_legacy_u32_to_link_mode()?
> > I'm guessing this will be more efficient.
> 
> Or at leas more readable. I'll update it.

I read that and had a similar reaction to Russell. Please do use the
helper.

	Andrew

Re: [PATCH net-next v1 4/4] net: phy: c45: genphy_c45_ethtool_set_eee: validate EEE link modes

[Andrew Lunn]

> +			adv[0] = data->advertised;

Same here please.

> +			linkmode_andnot(adv, adv, phydev->supported_eee);
> +			if (!linkmode_empty(adv)) {
> +				phydev_warn(phydev, "At least some EEE link modes are not supported.\n");
> +				return -EINVAL;
> +			}
> +
>  			phydev->advertising_eee[0] = data->advertised;

...

	Andrew

Re: [PATCH net-next v1 3/4] net: phy: do not force EEE support

[Andrew Lunn]

On Mon, Feb 20, 2023 at 02:56:04PM +0100, Oleksij Rempel wrote:
> With following patches:
> commit 9b01c885be36 ("net: phy: c22: migrate to genphy_c45_write_eee_adv()")
> commit 5827b168125d ("net: phy: c45: migrate to genphy_c45_write_eee_adv()")
> 
> we set the advertisement to potentially supported values. This behavior
> may introduce new regressions on systems where EEE was disabled by
> default (BIOS or boot loader configuration or by other ways.)
> 
> At same time, with this patches, we would overwrite EEE advertisement
> configuration made over ethtool.
> 
> To avoid this issues, we need to cache initial and ethtool advertisement
> configuration and store it for later use.

This is good. I started adding phy_supports_eee() which MAC drivers
can call to say they support EEE. To help with that i also added
advertising_eee, which i initialise to 0. I've not get all the code in
place, but i was thinking to populate advertising_eee with
supported_eee when phy_supports_eee() is called, and add
genphy_c45_an_config_eee_aneg() or similar to be called during
phy_start(). So it look like you have implemented more than 1/2 of
what i wanted to do :-)

     Andrew

Re: [PATCH net-next v1 3/4] net: phy: do not force EEE support

[Andrew Lunn]

> Hm.. ethtool do not provide enough information about expected behavior.
> Here is my expectation:
> - "ethtool --set-eee lan1 eee on" should enable EEE if it is disabled.
> - "ethtool --set-eee lan1 advertise 0x10" should change set of
>   advertised modes.
> - a sequence of "..advertise 0x10", "..eee on", "eee off" should restore
>   preconfigured advertise modes. advertising_eee instead of
>   supported_eee.

I agree ethtool is not very well documented. However, i would follow
what -s does. You can pass link modes you want to advertise, and you
can turn auto-neg on and off. Does turning auto-neg off and on again
reset the links modes which are advertised? I don't actually know, but
i think the behaviour should be consistent for link modes and EEE
modes.

	Andrew

Re: [PATCH net-next v1 3/4] net: phy: do not force EEE support

[Oleksij Rempel]

On Mon, Feb 20, 2023 at 04:48:26PM +0100, Andrew Lunn wrote:
> > Hm.. ethtool do not provide enough information about expected behavior.
> > Here is my expectation:
> > - "ethtool --set-eee lan1 eee on" should enable EEE if it is disabled.
> > - "ethtool --set-eee lan1 advertise 0x10" should change set of
> >   advertised modes.
> > - a sequence of "..advertise 0x10", "..eee on", "eee off" should restore
> >   preconfigured advertise modes. advertising_eee instead of
> >   supported_eee.
> 
> I agree ethtool is not very well documented. However, i would follow
> what -s does. You can pass link modes you want to advertise, and you
> can turn auto-neg on and off. Does turning auto-neg off and on again
> reset the links modes which are advertised? I don't actually know, but
> i think the behaviour should be consistent for link modes and EEE
> modes.

Hm.. "ethtool -s lan1 autoneg on" will restore supported values. Even
without switching it off.
In this case "eee on", supported_eee should be used.
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v1 3/4] net: phy: do not force EEE support

[Russell King (Oracle)]

On Mon, Feb 20, 2023 at 05:13:39PM +0100, Oleksij Rempel wrote:
> On Mon, Feb 20, 2023 at 04:48:26PM +0100, Andrew Lunn wrote:
> > > Hm.. ethtool do not provide enough information about expected behavior.
> > > Here is my expectation:
> > > - "ethtool --set-eee lan1 eee on" should enable EEE if it is disabled.
> > > - "ethtool --set-eee lan1 advertise 0x10" should change set of
> > >   advertised modes.
> > > - a sequence of "..advertise 0x10", "..eee on", "eee off" should restore
> > >   preconfigured advertise modes. advertising_eee instead of
> > >   supported_eee.
> > 
> > I agree ethtool is not very well documented. However, i would follow
> > what -s does. You can pass link modes you want to advertise, and you
> > can turn auto-neg on and off. Does turning auto-neg off and on again
> > reset the links modes which are advertised? I don't actually know, but
> > i think the behaviour should be consistent for link modes and EEE
> > modes.
> 
> Hm.. "ethtool -s lan1 autoneg on" will restore supported values. Even
> without switching it off.

Remember that -s is for the media capabilities, not EEE. There is
specific handling of the advertising mask in the userspace
ethtool.c::do_sset() but that doesn't exist for EEE, which merely
goes a ETHTOOL_GEEE, modifies the structure according to the arguments
given, and then calls ETHTOOL_SEEE. So:

# ethtool --set-eee lan1 eee on

will merely change eeecmd.eee_enabled to be set if it wasn't already,
but will not change eeecmd.advertised in any way - it'll contain
whatever it did before, which may or may not be zero.

This is in contrast to:

# ethtool -s lan1 autoneg on

which will set the advertisement to be the those that are supported.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v1 3/4] net: phy: do not force EEE support

[Russell King (Oracle)]

On Mon, Feb 20, 2023 at 04:48:26PM +0100, Andrew Lunn wrote:
> > Hm.. ethtool do not provide enough information about expected behavior.
> > Here is my expectation:
> > - "ethtool --set-eee lan1 eee on" should enable EEE if it is disabled.
> > - "ethtool --set-eee lan1 advertise 0x10" should change set of
> >   advertised modes.
> > - a sequence of "..advertise 0x10", "..eee on", "eee off" should restore
> >   preconfigured advertise modes. advertising_eee instead of
> >   supported_eee.
> 
> I agree ethtool is not very well documented. However, i would follow
> what -s does. You can pass link modes you want to advertise, and you
> can turn auto-neg on and off. Does turning auto-neg off and on again
> reset the links modes which are advertised? I don't actually know, but
> i think the behaviour should be consistent for link modes and EEE
> modes.

Hi Andrew,

I don't think we can do that without modifying the userspace ethtool -
see my other reply in this thread. It seems ethtool has some specific
handling for "autoneg on" without an advertising mask, where it
explicitly updates the advertising mask to have the link modes from
the supported mask. That logic doesn't exist for the EEE path, and
as the EEE path does a read-modify-according-to-arguments-write,
we can't even use "is the advertising mask zero" to implement it
kernel side (not that I think kernel side is the right place for
that policy.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!