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

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

[Oleksij Rempel]

changes v2:
- restore previous ethtool set logic for the case where advertisements
  are not provided by user space.
- use ethtool_convert_legacy_u32_to_link_mode() where possible
- genphy_c45_an_config_eee_aneg(): move adv initialization in to the if
  scope.

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    | 55 ++++++++++++++++++++++++++++--------
 drivers/net/phy/phy_device.c | 21 +++++++++++++-
 include/linux/phy.h          |  6 ++++
 3 files changed, 69 insertions(+), 13 deletions(-)

-- 
2.30.2

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

[Oleksij Rempel]

Make sure 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>
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 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 v2 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>
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 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 v2 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    | 24 +++++++++++++++++-------
 drivers/net/phy/phy_device.c | 19 +++++++++++++++++++
 include/linux/phy.h          |  5 +++++
 3 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 904f64818922..71671a07175f 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,13 @@ 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);
+	if (!phydev->eee_enabled) {
+		__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
+
+		return genphy_c45_write_eee_adv(phydev, adv);
+	}
+
+	return genphy_c45_write_eee_adv(phydev, phydev->advertising_eee);
 }
 
 /**
@@ -1431,17 +1436,22 @@ 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) {
 		if (data->advertised)
-			adv[0] = data->advertised;
+			ethtool_convert_legacy_u32_to_link_mode(phydev->advertising_eee,
+								data->advertised);
 		else
-			linkmode_copy(adv, phydev->supported_eee);
+			linkmode_copy(phydev->advertising_eee,
+				      phydev->supported_eee);
+
+		phydev->eee_enabled = true;
+	} 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 v2 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 | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 71671a07175f..f595acd0a895 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1439,12 +1439,23 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 	int ret;
 
 	if (data->eee_enabled) {
-		if (data->advertised)
+		if (data->advertised) {
+			__ETHTOOL_DECLARE_LINK_MODE_MASK(adv);
+
+			ethtool_convert_legacy_u32_to_link_mode(adv,
+								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;
+			}
+
 			ethtool_convert_legacy_u32_to_link_mode(phydev->advertising_eee,
 								data->advertised);
-		else
+		} else {
 			linkmode_copy(phydev->advertising_eee,
 				      phydev->supported_eee);
+		}
 
 		phydev->eee_enabled = true;
 	} else {
-- 
2.30.2

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

[Russell King (Oracle)]

On Tue, Feb 21, 2023 at 06:03:30AM +0100, Oleksij Rempel wrote:
> changes v2:
> - restore previous ethtool set logic for the case where advertisements
>   are not provided by user space.

I don't think the _kernel_ should be doing this - this introduces a
different behaviour to the kernel. As I already said, setting the
default advertisement in the case of ethtool -s is done by userspace
not by the kernel.

In fact, the kernel explicitly rejects an attempt to have autoneg
enabled with a zero advertising mask:

        linkmode_copy(advertising, cmd->link_modes.advertising);
        linkmode_and(advertising, advertising, phydev->supported);
        if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising))
                return -EINVAL;

and I think we should have a uniform behaviour with the same API,
rather than different behaviours, as that becomes quite messy.

-- 
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 v2 0/4] net: phy: EEE fixes

[Oleksij Rempel]

On Tue, Feb 21, 2023 at 09:36:35AM +0000, Russell King (Oracle) wrote:
> On Tue, Feb 21, 2023 at 06:03:30AM +0100, Oleksij Rempel wrote:
> > changes v2:
> > - restore previous ethtool set logic for the case where advertisements
> >   are not provided by user space.
> 
> I don't think the _kernel_ should be doing this - this introduces a
> different behaviour to the kernel. As I already said, setting the
> default advertisement in the case of ethtool -s is done by userspace
> not by the kernel.
> 
> In fact, the kernel explicitly rejects an attempt to have autoneg
> enabled with a zero advertising mask:
> 
>         linkmode_copy(advertising, cmd->link_modes.advertising);
>         linkmode_and(advertising, advertising, phydev->supported);
>         if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising))
>                 return -EINVAL;
> 
> and I think we should have a uniform behaviour with the same API,
> rather than different behaviours, as that becomes quite messy.

I decided to revert this part to not mix different issue. This logic
existed before my refactoring. So, it is better to handle it as
different patch. Current patch set should address only regressions.

-- 
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 v2 0/4] net: phy: EEE fixes

[Russell King (Oracle)]

On Tue, Feb 21, 2023 at 10:48:09AM +0100, Oleksij Rempel wrote:
> On Tue, Feb 21, 2023 at 09:36:35AM +0000, Russell King (Oracle) wrote:
> > On Tue, Feb 21, 2023 at 06:03:30AM +0100, Oleksij Rempel wrote:
> > > changes v2:
> > > - restore previous ethtool set logic for the case where advertisements
> > >   are not provided by user space.
> > 
> > I don't think the _kernel_ should be doing this - this introduces a
> > different behaviour to the kernel. As I already said, setting the
> > default advertisement in the case of ethtool -s is done by userspace
> > not by the kernel.
> > 
> > In fact, the kernel explicitly rejects an attempt to have autoneg
> > enabled with a zero advertising mask:
> > 
> >         linkmode_copy(advertising, cmd->link_modes.advertising);
> >         linkmode_and(advertising, advertising, phydev->supported);
> >         if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising))
> >                 return -EINVAL;
> > 
> > and I think we should have a uniform behaviour with the same API,
> > rather than different behaviours, as that becomes quite messy.
> 
> I decided to revert this part to not mix different issue. This logic
> existed before my refactoring. So, it is better to handle it as
> different patch. Current patch set should address only regressions.

Okay, let's keep it like that then. 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 v2 0/4] net: phy: EEE fixes

[Paolo Abeni]

On Tue, 2023-02-21 at 06:03 +0100, Oleksij Rempel wrote:
> changes v2:
> - restore previous ethtool set logic for the case where advertisements
>   are not provided by user space.
> - use ethtool_convert_legacy_u32_to_link_mode() where possible
> - genphy_c45_an_config_eee_aneg(): move adv initialization in to the if
>   scope.
> 
> 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    | 55 ++++++++++++++++++++++++++++--------
>  drivers/net/phy/phy_device.c | 21 +++++++++++++-
>  include/linux/phy.h          |  6 ++++
>  3 files changed, 69 insertions(+), 13 deletions(-)
> 
# Form letter - net-next is closed

The merge window for v6.3 has begun and therefore net-next is closed
for new drivers, features, code refactoring and optimizations.
We are currently accepting bug fixes only.

Please repost when net-next reopens after Mar 6th.

RFC patches sent for review only are obviously welcome at any time.

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

[Paolo Abeni]

On Tue, 2023-02-21 at 12:45 +0100, Paolo Abeni wrote:
> On Tue, 2023-02-21 at 06:03 +0100, Oleksij Rempel wrote:
> > changes v2:
> > - restore previous ethtool set logic for the case where advertisements
> >   are not provided by user space.
> > - use ethtool_convert_legacy_u32_to_link_mode() where possible
> > - genphy_c45_an_config_eee_aneg(): move adv initialization in to the if
> >   scope.
> > 
> > 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    | 55 ++++++++++++++++++++++++++++--------
> >  drivers/net/phy/phy_device.c | 21 +++++++++++++-
> >  include/linux/phy.h          |  6 ++++
> >  3 files changed, 69 insertions(+), 13 deletions(-)
> > 
> # Form letter - net-next is closed
> 
> The merge window for v6.3 has begun and therefore net-next is closed
> for new drivers, features, code refactoring and optimizations.
> We are currently accepting bug fixes only.
> 
> Please repost when net-next reopens after Mar 6th.
> 
> RFC patches sent for review only are obviously welcome at any time.

It looks like I was a little too hasty here; these are fixes for code
currently only on net-next. As such you can re-post (for -net) after
that Linus's net-next pull and subsequent merge into -net.

Thanks,

Paolo

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

[Paolo Abeni]

On Tue, 2023-02-21 at 06:03 +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>
> ---
>  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

This function does not have a second argument.

Cheers,

Paolo