| 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 |
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
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.
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
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/
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.
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 */
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(-)