diff mbox series

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

Message ID 20241115111151.183108-2-yong.liang.choong@linux.intel.com (mailing list archive)
State New
Headers show
Series Fix 'ethtool --show-eee' during initial stage | expand

Commit Message

Choong Yong Liang Nov. 15, 2024, 11:11 a.m. UTC
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(-)

Comments

Russell King (Oracle) Nov. 15, 2024, 1:37 p.m. UTC | #1
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
diff mbox series

Patch

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