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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2422c3c6-7ea4-4551-839b-7cbbdaadf499@linux.intel.com>
Date: Wed, 20 Nov 2024 11:11:44 +0800
From: Choong Yong Liang <yong.liang.choong@...ux.intel.com>
To: Heiner Kallweit <hkallweit1@...il.com>, Eric Dumazet
 <edumazet@...gle.com>, David Miller <davem@...emloft.net>,
 Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
 Russell King <rmk+kernel@...linux.org.uk>, Andrew Lunn <andrew@...n.ch>,
 Andrew Lunn <andrew+netdev@...n.ch>
Cc: Oleksij Rempel <o.rempel@...gutronix.de>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 net] net: phy: ensure that
 genphy_c45_an_config_eee_aneg() sees new value of phydev->eee_cfg.eee_enabled



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@...il.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.