[PATCH v2 net] net: phy: ensure that genphy_c45_an_config_eee_aneg() sees new value of phydev->eee_cfg.eee_enabled

[PATCH v2 net] net: phy: ensure that genphy_c45_an_config_eee_aneg() sees new value of phydev->eee_cfg.eee_enabled

[Heiner Kallweit]

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;
-- 
2.47.0

Re: [PATCH v2 net] net: phy: ensure that genphy_c45_an_config_eee_aneg() sees new value of phydev->eee_cfg.eee_enabled

[Choong Yong Liang]

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.

Re: [PATCH v2 net] net: phy: ensure that genphy_c45_an_config_eee_aneg() sees new value of phydev->eee_cfg.eee_enabled

[Heiner Kallweit]

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.

Re: [PATCH v2 net] net: phy: ensure that genphy_c45_an_config_eee_aneg() sees new value of phydev->eee_cfg.eee_enabled

[Russell King (Oracle)]

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!

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

Re: [PATCH v2 net] net: phy: ensure that genphy_c45_an_config_eee_aneg() sees new value of phydev->eee_cfg.eee_enabled

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Sat, 16 Nov 2024 21:52:15 +0100 you 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>
> 
> [...]

Here is the summary with links:
  - [v2,net] net: phy: ensure that genphy_c45_an_config_eee_aneg() sees new value of phydev->eee_cfg.eee_enabled
    https://git.kernel.org/netdev/net/c/f26a29a038ee

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html