| Message ID | a5efc274-ce58-49f3-ac8a-5384d9b41695@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: phy: ensure that genphy_c45_an_config_eee_aneg() sees new value of phydev->eee_cfg.eee_enabled | expand |
On 17/11/2024 4:52 am, Heiner Kallweit wrote: > This is a follow-up to 41ffcd95015f ("net: phy: fix phylib's dual > eee_enabled") and resolves an issue with genphy_c45_an_config_eee_aneg() > (called from genphy_c45_ethtool_set_eee) not seeing the new value of > phydev->eee_cfg.eee_enabled. > > Fixes: 49168d1980e2 ("net: phy: Add phy_support_eee() indicating MAC support EEE") > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > v2: > - change second arg of phy_ethtool_set_eee_noneg to pass the old settings > - reflect argument change in kdoc > --- > drivers/net/phy/phy.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 8876f3673..2ae0e3a67 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -1671,7 +1671,7 @@ EXPORT_SYMBOL(phy_ethtool_get_eee); > * phy_ethtool_set_eee_noneg - Adjusts MAC LPI configuration without PHY > * renegotiation > * @phydev: pointer to the target PHY device structure > - * @data: pointer to the ethtool_keee structure containing the new EEE settings > + * @old_cfg: pointer to the eee_config structure containing the old EEE settings > * > * This function updates the Energy Efficient Ethernet (EEE) configuration > * for cases where only the MAC's Low Power Idle (LPI) configuration changes, > @@ -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) > + const struct eee_config *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, &old_cfg); > + else if (ret < 0) > + phydev->eee_cfg = old_cfg; > + > mutex_unlock(&phydev->lock); > > return ret < 0 ? ret : 0; Hi Heiner, I hope this message finds you well. I noticed that the recent patch you submitted appears to be based on the previous work I did in this patch series: https://patchwork.kernel.org/project/netdevbpf/cover/20241115111151.183108-1-yong.liang.choong@linux.intel.com/. Would you mind including my name as "Reported-by" in the commit message? I believe this would appropriately acknowledge my role in identifying and reporting the issue. Thank you for your understanding and for the work you have done to improve the solution.
On Wed, Nov 20, 2024 at 4:11 AM Choong Yong Liang <yong.liang.choong@linux.intel.com> wrote: > > > > On 17/11/2024 4:52 am, Heiner Kallweit wrote: > > This is a follow-up to 41ffcd95015f ("net: phy: fix phylib's dual > > eee_enabled") and resolves an issue with genphy_c45_an_config_eee_aneg() > > (called from genphy_c45_ethtool_set_eee) not seeing the new value of > > phydev->eee_cfg.eee_enabled. > > > > Fixes: 49168d1980e2 ("net: phy: Add phy_support_eee() indicating MAC support EEE") > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > --- > > v2: > > - change second arg of phy_ethtool_set_eee_noneg to pass the old settings > > - reflect argument change in kdoc > > --- > > drivers/net/phy/phy.c | 24 ++++++++++++++---------- > > 1 file changed, 14 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > > index 8876f3673..2ae0e3a67 100644 > > --- a/drivers/net/phy/phy.c > > +++ b/drivers/net/phy/phy.c > > @@ -1671,7 +1671,7 @@ EXPORT_SYMBOL(phy_ethtool_get_eee); > > * phy_ethtool_set_eee_noneg - Adjusts MAC LPI configuration without PHY > > * renegotiation > > * @phydev: pointer to the target PHY device structure > > - * @data: pointer to the ethtool_keee structure containing the new EEE settings > > + * @old_cfg: pointer to the eee_config structure containing the old EEE settings > > * > > * This function updates the Energy Efficient Ethernet (EEE) configuration > > * for cases where only the MAC's Low Power Idle (LPI) configuration changes, > > @@ -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) > > + const struct eee_config *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, &old_cfg); > > + else if (ret < 0) > > + phydev->eee_cfg = old_cfg; > > + > > mutex_unlock(&phydev->lock); > > > > return ret < 0 ? ret : 0; > > Hi Heiner, > > I hope this message finds you well. > > I noticed that the recent patch you submitted appears to be based on the > previous work I did in this patch series: > https://patchwork.kernel.org/project/netdevbpf/cover/20241115111151.183108-1-yong.liang.choong@linux.intel.com/. > > Would you mind including my name as "Reported-by" in the commit message? I > believe this would appropriately acknowledge my role in identifying and > reporting the issue. > Reported-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> Hope this is enough for patchwork and/or the maintainers to pick up this tag, w/o resubmitting the patch.
On Sat, Nov 16, 2024 at 09:52:15PM +0100, Heiner Kallweit wrote: > This is a follow-up to 41ffcd95015f ("net: phy: fix phylib's dual > eee_enabled") and resolves an issue with genphy_c45_an_config_eee_aneg() > (called from genphy_c45_ethtool_set_eee) not seeing the new value of > phydev->eee_cfg.eee_enabled. > > Fixes: 49168d1980e2 ("net: phy: Add phy_support_eee() indicating MAC support EEE") > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Thanks!
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 8876f3673..2ae0e3a67 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -1671,7 +1671,7 @@ EXPORT_SYMBOL(phy_ethtool_get_eee); * phy_ethtool_set_eee_noneg - Adjusts MAC LPI configuration without PHY * renegotiation * @phydev: pointer to the target PHY device structure - * @data: pointer to the ethtool_keee structure containing the new EEE settings + * @old_cfg: pointer to the eee_config structure containing the old EEE settings * * This function updates the Energy Efficient Ethernet (EEE) configuration * for cases where only the MAC's Low Power Idle (LPI) configuration changes, @@ -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) + const struct eee_config *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, &old_cfg); + else if (ret < 0) + phydev->eee_cfg = old_cfg; + mutex_unlock(&phydev->lock); return ret < 0 ? ret : 0;
This is a follow-up to 41ffcd95015f ("net: phy: fix phylib's dual eee_enabled") and resolves an issue with genphy_c45_an_config_eee_aneg() (called from genphy_c45_ethtool_set_eee) not seeing the new value of phydev->eee_cfg.eee_enabled. Fixes: 49168d1980e2 ("net: phy: Add phy_support_eee() indicating MAC support EEE") Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- v2: - change second arg of phy_ethtool_set_eee_noneg to pass the old settings - reflect argument change in kdoc --- drivers/net/phy/phy.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)