Re: [PATCH net v2] net: phy: fix phy_ethtool_set_eee() incorrectly enabling LPI

[Heiner Kallweit <hkallweit1@gmail.com>]

On 22.11.2024 21:04, Heiner Kallweit wrote:
> On 22.11.2024 13:22, Russell King (Oracle) wrote:
>> ---
>>  drivers/net/phy/phy-c45.c |  2 +-
>>  drivers/net/phy/phy.c     | 32 ++++++++++++++++++--------------
>>  include/linux/phy.h       |  1 +
>>  3 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>> index 96d0b3a5a9d3..944ae98ad110 100644
>> --- a/drivers/net/phy/phy-c45.c
>> +++ b/drivers/net/phy/phy-c45.c
>> @@ -1530,7 +1530,7 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
>>  		return ret;
>>  
>>  	data->eee_enabled = is_enabled;
>> -	data->eee_active = ret;
>> +	data->eee_active = phydev->eee_active;
>>  	linkmode_copy(data->supported, phydev->supported_eee);
>>  
>>  	return 0;
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 4f3e742907cb..e174107b96e2 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -990,14 +990,14 @@ static int phy_check_link_status(struct phy_device *phydev)
>>  		phydev->state = PHY_RUNNING;
>>  		err = genphy_c45_eee_is_active(phydev,
>>  					       NULL, NULL, NULL);
>> -		if (err <= 0)
>> -			phydev->enable_tx_lpi = false;
>> -		else
>> -			phydev->enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled;
>> +		phydev->eee_active = err > 0;
>> +		phydev->enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled &&
>> +					phydev->eee_active;
>>  
>>  		phy_link_up(phydev);
>>  	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
>>  		phydev->state = PHY_NOLINK;
>> +		phydev->eee_active = false;
>>  		phydev->enable_tx_lpi = false;
>>  		phy_link_down(phydev);
>>  	}
>> @@ -1685,16 +1685,20 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
>>  static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>>  				      struct ethtool_keee *data)
>>  {
>> -	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);
>> -		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
>> -		if (phydev->link) {
>> -			phydev->link = false;
>> -			phy_link_down(phydev);
>> -			phydev->link = true;
>> -			phy_link_up(phydev);
>> -		}
>> +	bool enable_tx_lpi = data->tx_lpi_enabled &&
>> +			     phydev->eee_active;
>> +
>> +	eee_to_eeecfg(&phydev->eee_cfg, data);
>> +
>> +	if ((phydev->enable_tx_lpi != enable_tx_lpi ||
>> +	     phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) &&
>> +	    phydev->link) {
>> +		phydev->enable_tx_lpi = false;
>> +		phydev->link = false;
>> +		phy_link_down(phydev);
>> +		phydev->enable_tx_lpi = enable_tx_lpi;
>> +		phydev->link = true;
>> +		phy_link_up(phydev);
> 
> This part collides with a pending patch:
> https://patchwork.kernel.org/project/netdevbpf/patch/a5efc274-ce58-49f3-ac8a-5384d9b41695@gmail.com/
> I think you have to rebase and resubmit once the pending patch has been applied.
> 

Merge of both changes should result in something like this:

static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
                                      const struct eee_config *old_cfg)
{
        bool enable_tx_lpi;

        if (!phydev->link)
                return;

        enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled && phydev->eee_active;

        if (enable_tx_lpi != old_cfg->tx_lpi_enabled ||
            phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
                phydev->enable_tx_lpi = false;
                phydev->link = false;
                phy_link_down(phydev);
                phydev->enable_tx_lpi = enable_tx_lpi;
                phydev->link = true;
                phy_link_up(phydev);
        }
}