| Message ID | 20241115111151.183108-1-yong.liang.choong@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix 'ethtool --show-eee' during initial stage | expand |
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.
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.)
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);
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.
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;
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!
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! >
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(-)