[PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage

[PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage

[Choong Yong Liang]

From: Choong Yong Liang <yong.liang.choong@intel.com>

When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
the driver side is disabled. If we try to enable EEE through
'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
matches the setting required to enable EEE in ethnl_set_eee().

This patch series will remove phydev->eee_enabled and replace it with
eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
will follow the master configuration to have software and hardware in sync,
allowing 'ethtool --show-eee' to display the correct value during the
initial stage.

v2 changes:
 - Implement the prototype suggested by Russell
 - Check EEE before calling phy_support_eee()

Thanks to Russell for the proposed prototype in [1].

Reference:
[1] https://patchwork.kernel.org/comment/26121323/

Choong Yong Liang (2):
  net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
  net: stmmac: set initial EEE policy configuration

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
 drivers/net/phy/phy-c45.c                         | 11 +++++------
 drivers/net/phy/phy_device.c                      |  6 +++---
 include/linux/phy.h                               |  5 ++---
 4 files changed, 13 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH net v2 1/2] net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled

[Choong Yong Liang]

Not all PHYs have EEE enabled by default. For example, Marvell PHYs are
designed to have EEE hardware disabled during the initial state.

In the initial stage, phy_probe() sets phydev->eee_enabled to be disabled.
Then, the MAC calls phy_support_eee() to set eee_cfg.eee_enabled to be
enabled. However, when phy_start_aneg() is called,
genphy_c45_an_config_eee_aneg() still refers to phydev->eee_enabled.
This causes the 'ethtool --show-eee' command to show that EEE is enabled,
but in actuality, the driver side is disabled.

This patch will remove phydev->eee_enabled and replace it with
eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(),
it will follow the master configuration to have software and hardware
in sync.

Fixes: 3eeca4e199ce ("net: phy: do not force EEE support")
Cc: stable@vger.kernel.org
Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>
Suggested-by: Russell King <linux@armlinux.org.uk>
---
 drivers/net/phy/phy-c45.c    | 11 +++++------
 drivers/net/phy/phy_device.c |  6 +++---
 include/linux/phy.h          |  5 ++---
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 5695935fdce9..fa42158eff83 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -272,7 +272,7 @@ int genphy_c45_an_config_aneg(struct phy_device *phydev)
 	linkmode_and(phydev->advertising, phydev->advertising,
 		     phydev->supported);
 
-	ret = genphy_c45_an_config_eee_aneg(phydev);
+	ret = genphy_c45_an_config_eee_aneg(phydev, phydev->eee_cfg.eee_enabled);
 	if (ret < 0)
 		return ret;
 	else if (ret)
@@ -940,9 +940,10 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_eee_abilities);
  * genphy_c45_an_config_eee_aneg - configure EEE advertisement
  * @phydev: target phy_device struct
  */
-int genphy_c45_an_config_eee_aneg(struct phy_device *phydev)
+int genphy_c45_an_config_eee_aneg(struct phy_device *phydev,
+				  bool is_eee_enabled)
 {
-	if (!phydev->eee_enabled) {
+	if (!is_eee_enabled) {
 		__ETHTOOL_DECLARE_LINK_MODE_MASK(adv) = {};
 
 		return genphy_c45_write_eee_adv(phydev, adv);
@@ -1575,9 +1576,7 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 		linkmode_copy(phydev->advertising_eee, adv);
 	}
 
-	phydev->eee_enabled = data->eee_enabled;
-
-	ret = genphy_c45_an_config_eee_aneg(phydev);
+	ret = genphy_c45_an_config_eee_aneg(phydev, data->eee_enabled);
 	if (ret > 0) {
 		ret = phy_restart_aneg(phydev);
 		if (ret < 0)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 499797646580..97e835ad4544 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2421,7 +2421,7 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed)
 	unsigned long *advert;
 	int err;
 
-	err = genphy_c45_an_config_eee_aneg(phydev);
+	err = genphy_c45_an_config_eee_aneg(phydev, phydev->eee_cfg.eee_enabled);
 	if (err < 0)
 		return err;
 	else if (err)
@@ -3595,12 +3595,12 @@ static int phy_probe(struct device *dev)
 	/* There is no "enabled" flag. If PHY is advertising, assume it is
 	 * kind of enabled.
 	 */
-	phydev->eee_enabled = !linkmode_empty(phydev->advertising_eee);
+	phydev->eee_cfg.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)
+	if (phydev->eee_cfg.eee_enabled)
 		linkmode_and(phydev->advertising_eee, phydev->supported_eee,
 			     phydev->advertising_eee);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a98bc91a0cde..fde9f1f868bb 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -601,7 +601,6 @@ struct macsec_ops;
  * @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
  * @enable_tx_lpi: When True, MAC should transmit LPI to PHY
  * @eee_cfg: User configuration of EEE
  * @lp_advertising: Current link partner advertised linkmodes
@@ -721,7 +720,6 @@ struct phy_device {
 	/* used for eee validation and configuration*/
 	__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);
@@ -1952,7 +1950,8 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 			       struct ethtool_keee *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_an_config_eee_aneg(struct phy_device *phydev,
+				  bool is_eee_enabled);
 int genphy_c45_read_eee_adv(struct phy_device *phydev, unsigned long *adv);
 
 /* Generic C45 PHY driver */
-- 
2.34.1

Re: [PATCH net v2 1/2] net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled

[Russell King (Oracle)]

On Fri, Nov 15, 2024 at 07:11:50PM +0800, Choong Yong Liang wrote:
> Not all PHYs have EEE enabled by default. For example, Marvell PHYs are
> designed to have EEE hardware disabled during the initial state.
> 
> In the initial stage, phy_probe() sets phydev->eee_enabled to be disabled.
> Then, the MAC calls phy_support_eee() to set eee_cfg.eee_enabled to be
> enabled. However, when phy_start_aneg() is called,
> genphy_c45_an_config_eee_aneg() still refers to phydev->eee_enabled.
> This causes the 'ethtool --show-eee' command to show that EEE is enabled,
> but in actuality, the driver side is disabled.
> 
> This patch will remove phydev->eee_enabled and replace it with
> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(),
> it will follow the master configuration to have software and hardware
> in sync.

Hmm. I'm not happy with how you're handling my patch. I would've liked
some feedback on it (thanks for spotting that the set_eee case needed
to pass the state to genphy_c45_an_config_eee_aneg()).

However, what's worse is, that the bulk of this patch is my work, yet
you've effectively claimed complete authorship of it in the way you
are submitting this patch. Moreover, you are violating the kernel
submission rules, as the Signed-off-by does not include one for me
(which I need to explicitly give.) I was waiting for the results of
your testing before finalising the patch.

The patch needs to be authored by me, the first sign-off needs to be
me, then optionally Co-developed-by for you, and then your sign-off.

See Documentation/process/submitting-patches.rst

Thanks.

pw-bot: cr

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

Re: [PATCH net v2 1/2] net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled

[Choong Yong Liang]

On 15/11/2024 9:37 pm, Russell King (Oracle) wrote:
> On Fri, Nov 15, 2024 at 07:11:50PM +0800, Choong Yong Liang wrote:
>> Not all PHYs have EEE enabled by default. For example, Marvell PHYs are
>> designed to have EEE hardware disabled during the initial state.
>>
>> In the initial stage, phy_probe() sets phydev->eee_enabled to be disabled.
>> Then, the MAC calls phy_support_eee() to set eee_cfg.eee_enabled to be
>> enabled. However, when phy_start_aneg() is called,
>> genphy_c45_an_config_eee_aneg() still refers to phydev->eee_enabled.
>> This causes the 'ethtool --show-eee' command to show that EEE is enabled,
>> but in actuality, the driver side is disabled.
>>
>> This patch will remove phydev->eee_enabled and replace it with
>> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(),
>> it will follow the master configuration to have software and hardware
>> in sync.
> 
> Hmm. I'm not happy with how you're handling my patch. I would've liked
> some feedback on it (thanks for spotting that the set_eee case needed
> to pass the state to genphy_c45_an_config_eee_aneg()).
> 
> However, what's worse is, that the bulk of this patch is my work, yet
> you've effectively claimed complete authorship of it in the way you
> are submitting this patch. Moreover, you are violating the kernel
> submission rules, as the Signed-off-by does not include one for me
> (which I need to explicitly give.) I was waiting for the results of
> your testing before finalising the patch.
> 
> The patch needs to be authored by me, the first sign-off needs to be
> me, then optionally Co-developed-by for you, and then your sign-off.
> 
> See Documentation/process/submitting-patches.rst
> 
> Thanks.
> 
> pw-bot: cr
> 

Sorry for the late reply; I just got back from my sick leave. I wasn't 
aware that you had already submitted a patch. I thought I should include it 
in my patch series. However, I think I messed up the "Signed-off" part. 
Sorry about that.

The testing part actually took quite some time to complete, and I was 
already sick last Friday. I was only able to complete the patch series and 
resubmit the patch, and I thought we could discuss the test results from 
the patch series. The issue was initially found with EEE on GPY PHY working 
together with ptp4l, and it did not meet the expected results. There are 
many things that need to be tested, as it is not only Marvell PHY that has 
the issue.

With your patch, most of the issues were resolved based on the testing. 
However, the set_eee was using the old value of eee_enabled and was not 
able to turn EEE on or off. I think Heiner's patch already solved that part.

With all the solutions provided, I think the only patch left that I need to 
submit is the one calling 'phy_support_eee()' from stmmac.

Re: [PATCH net v2 1/2] net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled

[Oleksij Rempel]

On Tue, Nov 19, 2024 at 05:06:33PM +0800, Choong Yong Liang wrote:
> 
> 
> On 15/11/2024 9:37 pm, Russell King (Oracle) wrote:
> > On Fri, Nov 15, 2024 at 07:11:50PM +0800, Choong Yong Liang wrote:
> > > Not all PHYs have EEE enabled by default. For example, Marvell PHYs are
> > > designed to have EEE hardware disabled during the initial state.
> > > 
> > > In the initial stage, phy_probe() sets phydev->eee_enabled to be disabled.
> > > Then, the MAC calls phy_support_eee() to set eee_cfg.eee_enabled to be
> > > enabled. However, when phy_start_aneg() is called,
> > > genphy_c45_an_config_eee_aneg() still refers to phydev->eee_enabled.
> > > This causes the 'ethtool --show-eee' command to show that EEE is enabled,
> > > but in actuality, the driver side is disabled.
> > > 
> > > This patch will remove phydev->eee_enabled and replace it with
> > > eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(),
> > > it will follow the master configuration to have software and hardware
> > > in sync.
> > 
> > Hmm. I'm not happy with how you're handling my patch. I would've liked
> > some feedback on it (thanks for spotting that the set_eee case needed
> > to pass the state to genphy_c45_an_config_eee_aneg()).
> > 
> > However, what's worse is, that the bulk of this patch is my work, yet
> > you've effectively claimed complete authorship of it in the way you
> > are submitting this patch. Moreover, you are violating the kernel
> > submission rules, as the Signed-off-by does not include one for me
> > (which I need to explicitly give.) I was waiting for the results of
> > your testing before finalising the patch.
> > 
> > The patch needs to be authored by me, the first sign-off needs to be
> > me, then optionally Co-developed-by for you, and then your sign-off.
> > 
> > See Documentation/process/submitting-patches.rst
> > 
> > Thanks.
> > 
> > pw-bot: cr
> > 
> 
> Sorry for the late reply; I just got back from my sick leave. I wasn't aware
> that you had already submitted a patch. I thought I should include it in my
> patch series. However, I think I messed up the "Signed-off" part. Sorry
> about that.
> 
> The testing part actually took quite some time to complete, and I was
> already sick last Friday. I was only able to complete the patch series and
> resubmit the patch, and I thought we could discuss the test results from the
> patch series. The issue was initially found with EEE on GPY PHY working
> together with ptp4l, and it did not meet the expected results. There are
> many things that need to be tested, as it is not only Marvell PHY that has
> the issue.

Hm, the PTP issue with EEE is usually related to PHYs implementing the
EEE without MAC/LPI support. This PHYs are buffering frames and changing
the transmission time, so if the time stamp is made by MAC it will have
different real transmission time. So far i know, Atheros and Realtek
implement it, even if it is not always officially documented, it can be
tested.

Regards,
Oleksij
-- 
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 v2 1/2] net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled

[Choong Yong Liang]

On 19/11/2024 5:47 pm, Oleksij Rempel wrote:
>> Sorry for the late reply; I just got back from my sick leave. I wasn't aware
>> that you had already submitted a patch. I thought I should include it in my
>> patch series. However, I think I messed up the "Signed-off" part. Sorry
>> about that.
>>
>> The testing part actually took quite some time to complete, and I was
>> already sick last Friday. I was only able to complete the patch series and
>> resubmit the patch, and I thought we could discuss the test results from the
>> patch series. The issue was initially found with EEE on GPY PHY working
>> together with ptp4l, and it did not meet the expected results. There are
>> many things that need to be tested, as it is not only Marvell PHY that has
>> the issue.
> 
> Hm, the PTP issue with EEE is usually related to PHYs implementing the
> EEE without MAC/LPI support. This PHYs are buffering frames and changing
> the transmission time, so if the time stamp is made by MAC it will have
> different real transmission time. So far i know, Atheros and Realtek
> implement it, even if it is not always officially documented, it can be
> tested.
> 
> Regards,
> Oleksij

Thanks, Oleksij, for the suggestion.
The actual problem we are facing is that the software and hardware 
configuration is not in sync.

With the following patches applied, the issues were fixed:
- 
https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
- 
https://patchwork.kernel.org/project/netdevbpf/patch/a5efc274-ce58-49f3-ac8a-5384d9b41695@gmail.com/
- 
https://patchwork.kernel.org/project/netdevbpf/patch/20241120083818.1079456-1-yong.liang.choong@linux.intel.com/

Re: [PATCH net v2 1/2] net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled

[Russell King (Oracle)]

On Tue, Nov 19, 2024 at 10:47:24AM +0100, Oleksij Rempel wrote:
> On Tue, Nov 19, 2024 at 05:06:33PM +0800, Choong Yong Liang wrote:
> > Sorry for the late reply; I just got back from my sick leave. I wasn't aware
> > that you had already submitted a patch. I thought I should include it in my
> > patch series. However, I think I messed up the "Signed-off" part. Sorry
> > about that.
> > 
> > The testing part actually took quite some time to complete, and I was
> > already sick last Friday. I was only able to complete the patch series and
> > resubmit the patch, and I thought we could discuss the test results from the
> > patch series. The issue was initially found with EEE on GPY PHY working
> > together with ptp4l, and it did not meet the expected results. There are
> > many things that need to be tested, as it is not only Marvell PHY that has
> > the issue.
> 
> Hm, the PTP issue with EEE is usually related to PHYs implementing the
> EEE without MAC/LPI support.

I think you are referring to PHYs that implement EEE on their own,
without requiring support at the MAC, such as Atheros SmartEEE.

It wasn't clear that you aren't referring to a situation where the
PHY has EEE support, requiring the MAC to generate LPI but the MAC
does have that ability.

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

[PATCH net v2 2/2] net: stmmac: set initial EEE policy configuration

[Choong Yong Liang]

Set the initial eee_cfg values to have 'ethtool --show-eee ' display
the initial EEE configuration.

Fixes: 3eeca4e199ce ("net: phy: do not force EEE support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7bf275f127c9..766213ee82c1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1205,6 +1205,9 @@ static int stmmac_init_phy(struct net_device *dev)
 			return -ENODEV;
 		}
 
+		if (priv->dma_cap.eee)
+			phy_support_eee(phydev);
+
 		ret = phylink_connect_phy(priv->phylink, phydev);
 	} else {
 		fwnode_handle_put(phy_fwnode);
-- 
2.34.1

Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage

[Heiner Kallweit]

On 15.11.2024 12:11, Choong Yong Liang wrote:
> From: Choong Yong Liang <yong.liang.choong@intel.com>
> 
> When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
> the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
> the driver side is disabled. If we try to enable EEE through
> 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
> matches the setting required to enable EEE in ethnl_set_eee().
> 
> This patch series will remove phydev->eee_enabled and replace it with
> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
> will follow the master configuration to have software and hardware in sync,
> allowing 'ethtool --show-eee' to display the correct value during the
> initial stage.
> 
> v2 changes:
>  - Implement the prototype suggested by Russell
>  - Check EEE before calling phy_support_eee()
> 
> Thanks to Russell for the proposed prototype in [1].
> 
> Reference:
> [1] https://patchwork.kernel.org/comment/26121323/
> 
> Choong Yong Liang (2):
>   net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
>   net: stmmac: set initial EEE policy configuration
> 
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
>  drivers/net/phy/phy-c45.c                         | 11 +++++------
>  drivers/net/phy/phy_device.c                      |  6 +++---
>  include/linux/phy.h                               |  5 ++---
>  4 files changed, 13 insertions(+), 12 deletions(-)
> 

Russell submitted the proposed patch already:
https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
So there's no need for your patch 1.

Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage

[Russell King (Oracle)]

On Fri, Nov 15, 2024 at 02:41:54PM +0100, Heiner Kallweit wrote:
> On 15.11.2024 12:11, Choong Yong Liang wrote:
> > From: Choong Yong Liang <yong.liang.choong@intel.com>
> > 
> > When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
> > the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
> > the driver side is disabled. If we try to enable EEE through
> > 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
> > matches the setting required to enable EEE in ethnl_set_eee().
> > 
> > This patch series will remove phydev->eee_enabled and replace it with
> > eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
> > will follow the master configuration to have software and hardware in sync,
> > allowing 'ethtool --show-eee' to display the correct value during the
> > initial stage.
> > 
> > v2 changes:
> >  - Implement the prototype suggested by Russell
> >  - Check EEE before calling phy_support_eee()
> > 
> > Thanks to Russell for the proposed prototype in [1].
> > 
> > Reference:
> > [1] https://patchwork.kernel.org/comment/26121323/
> > 
> > Choong Yong Liang (2):
> >   net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
> >   net: stmmac: set initial EEE policy configuration
> > 
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
> >  drivers/net/phy/phy-c45.c                         | 11 +++++------
> >  drivers/net/phy/phy_device.c                      |  6 +++---
> >  include/linux/phy.h                               |  5 ++---
> >  4 files changed, 13 insertions(+), 12 deletions(-)
> > 
> 
> Russell submitted the proposed patch already:
> https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
> So there's no need for your patch 1.

Patch 1 is an updated version of that patch, minus my authorship and of
course no sign-off. I've already marked this series as requiring changes
in patchwork (hopefully, if I did it correctly.)

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

Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage

[Heiner Kallweit]

On 15.11.2024 15:12, Russell King (Oracle) wrote:
> On Fri, Nov 15, 2024 at 02:41:54PM +0100, Heiner Kallweit wrote:
>> On 15.11.2024 12:11, Choong Yong Liang wrote:
>>> From: Choong Yong Liang <yong.liang.choong@intel.com>
>>>
>>> When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
>>> the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
>>> the driver side is disabled. If we try to enable EEE through
>>> 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
>>> matches the setting required to enable EEE in ethnl_set_eee().
>>>
>>> This patch series will remove phydev->eee_enabled and replace it with
>>> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
>>> will follow the master configuration to have software and hardware in sync,
>>> allowing 'ethtool --show-eee' to display the correct value during the
>>> initial stage.
>>>
>>> v2 changes:
>>>  - Implement the prototype suggested by Russell
>>>  - Check EEE before calling phy_support_eee()
>>>
>>> Thanks to Russell for the proposed prototype in [1].
>>>
>>> Reference:
>>> [1] https://patchwork.kernel.org/comment/26121323/
>>>
>>> Choong Yong Liang (2):
>>>   net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
>>>   net: stmmac: set initial EEE policy configuration
>>>
>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
>>>  drivers/net/phy/phy-c45.c                         | 11 +++++------
>>>  drivers/net/phy/phy_device.c                      |  6 +++---
>>>  include/linux/phy.h                               |  5 ++---
>>>  4 files changed, 13 insertions(+), 12 deletions(-)
>>>
>>
>> Russell submitted the proposed patch already:
>> https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
>> So there's no need for your patch 1.
> 
> Patch 1 is an updated version of that patch, minus my authorship and of
> course no sign-off. I've already marked this series as requiring changes
> in patchwork (hopefully, if I did it correctly.)
> 

The updated version adds an argument to genphy_c45_an_config_eee_aneg(),
and I wonder whether we can do better, as this results in several calls
with the same argument. The following is an alternative, to be applied
on top of your original patch. I don't have a clear preference, though.

---
 drivers/net/phy/phy.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8876f3673..22c9bbebb 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1682,11 +1682,10 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
  * configuration.
  */
 static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
-				      struct ethtool_keee *data)
+				      struct ethtool_keee *old_cfg)
 {
-	if (phydev->eee_cfg.tx_lpi_enabled != data->tx_lpi_enabled ||
-	    phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
-		eee_to_eeecfg(&phydev->eee_cfg, data);
+	if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled ||
+	    phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
 		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
 		if (phydev->link) {
 			phydev->link = false;
@@ -1706,21 +1705,27 @@ static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
  */
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
 {
+	struct eee_config old_cfg;
 	int ret;
 
 	if (!phydev->drv)
 		return -EIO;
 
+	old_cfg = phydev->eee_cfg;
+	eee_to_eeecfg(&phydev->eee_cfg, data);
+
 	mutex_lock(&phydev->lock);
 	ret = genphy_c45_ethtool_set_eee(phydev, data);
-	if (ret >= 0) {
-		if (ret == 0)
-			phy_ethtool_set_eee_noneg(phydev, data);
-		eee_to_eeecfg(&phydev->eee_cfg, data);
-	}
+	if (ret == 0)
+		phy_ethtool_set_eee_noneg(phydev, data);
 	mutex_unlock(&phydev->lock);
 
-	return ret < 0 ? ret : 0;
+	if (ret < 0) {
+		phydev->eee_cfg = old_cfg;
+		return ret;
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL(phy_ethtool_set_eee);
 
-- 
2.47.0

Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage

[Russell King (Oracle)]

On Fri, Nov 15, 2024 at 09:35:25PM +0100, Heiner Kallweit wrote:
> On 15.11.2024 15:12, Russell King (Oracle) wrote:
> > On Fri, Nov 15, 2024 at 02:41:54PM +0100, Heiner Kallweit wrote:
> >> On 15.11.2024 12:11, Choong Yong Liang wrote:
> >>> From: Choong Yong Liang <yong.liang.choong@intel.com>
> >>>
> >>> When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
> >>> the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
> >>> the driver side is disabled. If we try to enable EEE through
> >>> 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
> >>> matches the setting required to enable EEE in ethnl_set_eee().
> >>>
> >>> This patch series will remove phydev->eee_enabled and replace it with
> >>> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
> >>> will follow the master configuration to have software and hardware in sync,
> >>> allowing 'ethtool --show-eee' to display the correct value during the
> >>> initial stage.
> >>>
> >>> v2 changes:
> >>>  - Implement the prototype suggested by Russell
> >>>  - Check EEE before calling phy_support_eee()
> >>>
> >>> Thanks to Russell for the proposed prototype in [1].
> >>>
> >>> Reference:
> >>> [1] https://patchwork.kernel.org/comment/26121323/
> >>>
> >>> Choong Yong Liang (2):
> >>>   net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
> >>>   net: stmmac: set initial EEE policy configuration
> >>>
> >>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
> >>>  drivers/net/phy/phy-c45.c                         | 11 +++++------
> >>>  drivers/net/phy/phy_device.c                      |  6 +++---
> >>>  include/linux/phy.h                               |  5 ++---
> >>>  4 files changed, 13 insertions(+), 12 deletions(-)
> >>>
> >>
> >> Russell submitted the proposed patch already:
> >> https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
> >> So there's no need for your patch 1.
> > 
> > Patch 1 is an updated version of that patch, minus my authorship and of
> > course no sign-off. I've already marked this series as requiring changes
> > in patchwork (hopefully, if I did it correctly.)
> > 
> 
> The updated version adds an argument to genphy_c45_an_config_eee_aneg(),
> and I wonder whether we can do better, as this results in several calls
> with the same argument. The following is an alternative, to be applied
> on top of your original patch. I don't have a clear preference, though.
> 
> ---
>  drivers/net/phy/phy.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 8876f3673..22c9bbebb 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1682,11 +1682,10 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
>   * configuration.
>   */
>  static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
> -				      struct ethtool_keee *data)
> +				      struct ethtool_keee *old_cfg)
>  {
> -	if (phydev->eee_cfg.tx_lpi_enabled != data->tx_lpi_enabled ||
> -	    phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
> -		eee_to_eeecfg(&phydev->eee_cfg, data);
> +	if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled ||
> +	    phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
>  		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
>  		if (phydev->link) {
>  			phydev->link = false;
> @@ -1706,21 +1705,27 @@ static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>   */
>  int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
>  {
> +	struct eee_config old_cfg;
>  	int ret;
>  
>  	if (!phydev->drv)
>  		return -EIO;
>  
> +	old_cfg = phydev->eee_cfg;
> +	eee_to_eeecfg(&phydev->eee_cfg, data);
> +

Hmm, don't we want to do this under phydev->lock, because network
drivers and phylib may be reading from phydev->eee_cfg? If we
update it outside the lock, and then revert, there's a chance that
the phylib state machine / network driver may see the changes
which then get reverted on failure, potentially leading to
inconsistent state.

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

Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage

[Heiner Kallweit]

On 16.11.2024 16:34, Russell King (Oracle) wrote:
> On Fri, Nov 15, 2024 at 09:35:25PM +0100, Heiner Kallweit wrote:
>> On 15.11.2024 15:12, Russell King (Oracle) wrote:
>>> On Fri, Nov 15, 2024 at 02:41:54PM +0100, Heiner Kallweit wrote:
>>>> On 15.11.2024 12:11, Choong Yong Liang wrote:
>>>>> From: Choong Yong Liang <yong.liang.choong@intel.com>
>>>>>
>>>>> When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
>>>>> the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
>>>>> the driver side is disabled. If we try to enable EEE through
>>>>> 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
>>>>> matches the setting required to enable EEE in ethnl_set_eee().
>>>>>
>>>>> This patch series will remove phydev->eee_enabled and replace it with
>>>>> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
>>>>> will follow the master configuration to have software and hardware in sync,
>>>>> allowing 'ethtool --show-eee' to display the correct value during the
>>>>> initial stage.
>>>>>
>>>>> v2 changes:
>>>>>  - Implement the prototype suggested by Russell
>>>>>  - Check EEE before calling phy_support_eee()
>>>>>
>>>>> Thanks to Russell for the proposed prototype in [1].
>>>>>
>>>>> Reference:
>>>>> [1] https://patchwork.kernel.org/comment/26121323/
>>>>>
>>>>> Choong Yong Liang (2):
>>>>>   net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
>>>>>   net: stmmac: set initial EEE policy configuration
>>>>>
>>>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
>>>>>  drivers/net/phy/phy-c45.c                         | 11 +++++------
>>>>>  drivers/net/phy/phy_device.c                      |  6 +++---
>>>>>  include/linux/phy.h                               |  5 ++---
>>>>>  4 files changed, 13 insertions(+), 12 deletions(-)
>>>>>
>>>>
>>>> Russell submitted the proposed patch already:
>>>> https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
>>>> So there's no need for your patch 1.
>>>
>>> Patch 1 is an updated version of that patch, minus my authorship and of
>>> course no sign-off. I've already marked this series as requiring changes
>>> in patchwork (hopefully, if I did it correctly.)
>>>
>>
>> The updated version adds an argument to genphy_c45_an_config_eee_aneg(),
>> and I wonder whether we can do better, as this results in several calls
>> with the same argument. The following is an alternative, to be applied
>> on top of your original patch. I don't have a clear preference, though.
>>
>> ---
>>  drivers/net/phy/phy.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 8876f3673..22c9bbebb 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -1682,11 +1682,10 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
>>   * configuration.
>>   */
>>  static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>> -				      struct ethtool_keee *data)
>> +				      struct ethtool_keee *old_cfg)
>>  {
>> -	if (phydev->eee_cfg.tx_lpi_enabled != data->tx_lpi_enabled ||
>> -	    phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
>> -		eee_to_eeecfg(&phydev->eee_cfg, data);
>> +	if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled ||
>> +	    phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
>>  		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
>>  		if (phydev->link) {
>>  			phydev->link = false;
>> @@ -1706,21 +1705,27 @@ static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>>   */
>>  int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
>>  {
>> +	struct eee_config old_cfg;
>>  	int ret;
>>  
>>  	if (!phydev->drv)
>>  		return -EIO;
>>  
>> +	old_cfg = phydev->eee_cfg;
>> +	eee_to_eeecfg(&phydev->eee_cfg, data);
>> +
> 
> Hmm, don't we want to do this under phydev->lock, because network
> drivers and phylib may be reading from phydev->eee_cfg? If we
> update it outside the lock, and then revert, there's a chance that
> the phylib state machine / network driver may see the changes
> which then get reverted on failure, potentially leading to
> inconsistent state.
> 

Good point, then the patch would look like this.
BTW: Saw that Jakub applied your patch already.

---
 drivers/net/phy/phy.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8876f3673..7f6594a66 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1682,11 +1682,10 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
  * configuration.
  */
 static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
-				      struct ethtool_keee *data)
+				      struct ethtool_keee *old_cfg)
 {
-	if (phydev->eee_cfg.tx_lpi_enabled != data->tx_lpi_enabled ||
-	    phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
-		eee_to_eeecfg(&phydev->eee_cfg, data);
+	if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled ||
+	    phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
 		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
 		if (phydev->link) {
 			phydev->link = false;
@@ -1706,18 +1705,23 @@ static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
  */
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
 {
+	struct eee_config old_cfg;
 	int ret;
 
 	if (!phydev->drv)
 		return -EIO;
 
 	mutex_lock(&phydev->lock);
+
+	old_cfg = phydev->eee_cfg;
+	eee_to_eeecfg(&phydev->eee_cfg, data);
+
 	ret = genphy_c45_ethtool_set_eee(phydev, data);
-	if (ret >= 0) {
-		if (ret == 0)
-			phy_ethtool_set_eee_noneg(phydev, data);
-		eee_to_eeecfg(&phydev->eee_cfg, data);
-	}
+	if (ret == 0)
+		phy_ethtool_set_eee_noneg(phydev, data);
+	else if (ret < 0)
+		phydev->eee_cfg = old_cfg;
+
 	mutex_unlock(&phydev->lock);
 
 	return ret < 0 ? ret : 0;
-- 
2.47.0

Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage

[Russell King (Oracle)]

On Sat, Nov 16, 2024 at 06:41:13PM +0100, Heiner Kallweit wrote:
> On 16.11.2024 16:34, Russell King (Oracle) wrote:
> > Hmm, don't we want to do this under phydev->lock, because network
> > drivers and phylib may be reading from phydev->eee_cfg? If we
> > update it outside the lock, and then revert, there's a chance that
> > the phylib state machine / network driver may see the changes
> > which then get reverted on failure, potentially leading to
> > inconsistent state.
> 
> Good point, then the patch would look like this.
> BTW: Saw that Jakub applied your patch already.

Yes indeed, so I hope Jakub will apply your follow-up patch soon!
This LGTM.

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! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage

[Heiner Kallweit]

On 16.11.2024 18:44, Russell King (Oracle) wrote:
> On Sat, Nov 16, 2024 at 06:41:13PM +0100, Heiner Kallweit wrote:
>> On 16.11.2024 16:34, Russell King (Oracle) wrote:
>>> Hmm, don't we want to do this under phydev->lock, because network
>>> drivers and phylib may be reading from phydev->eee_cfg? If we
>>> update it outside the lock, and then revert, there's a chance that
>>> the phylib state machine / network driver may see the changes
>>> which then get reverted on failure, potentially leading to
>>> inconsistent state.
>>
>> Good point, then the patch would look like this.
>> BTW: Saw that Jakub applied your patch already.
> 
> Yes indeed, so I hope Jakub will apply your follow-up patch soon!
> This LGTM.
> 
OK, then I'll submit the followup.

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