Re: [PATCH RESUBMIT net-next] r8169: simplify EEE handling

[Heiner Kallweit <hkallweit1@gmail.com>]

On 02.02.2024 14:12, Andrew Lunn wrote:
> On Fri, Feb 02, 2024 at 07:55:38AM +0100, Heiner Kallweit wrote:
>> On 02.02.2024 01:16, Andrew Lunn wrote:
>>>> @@ -5058,7 +5033,9 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
>>>>  	}
>>>>  
>>>>  	tp->phydev->mac_managed_pm = true;
>>>> -
>>>> +	if (rtl_supports_eee(tp))
>>>> +		linkmode_copy(tp->phydev->advertising_eee,
>>>> +			      tp->phydev->supported_eee);
>>>
>>> This looks odd. Does it mean something is missing on phylib?
>>>
>> Reason is that we treat "normal" advertising and EEE advertising differently
>> in phylib. See this code snippet from phy_probe().
>>
>>         phy_advertise_supported(phydev);
>>         /* Get PHY default EEE advertising modes and handle them as potentially
>>          * safe initial configuration.
>>          */
>>         err = genphy_c45_read_eee_adv(phydev, phydev->advertising_eee);
>>
>> For EEE we don't change the initial advertising to what's supported,
>> but preserve the EEE advertising at the time of phy probing.
>> So if I want to mimic the behavior of phy_advertise_supported() for EEE,
>> I have to populate advertising_eee in the driver.
> 
> So the device you are using is advertising less than what it supports?
> 
It may. If on a board with this chip version the BIOS for whatever reason
disabled EEE advertising, then users may complain that their system consumes
more power with r8169 than with r8125. Typical users don't necessarily
know that EEE exists and what it is, and how to enable it with ethtool.
Therefore I'd like to ensure that the supported EEE modes are also advertised.

>> Alternative would be to change phy_advertise_supported(), but this may
>> impact systems with PHY's with EEE flaws.
> 
> If i remember correctly, there was some worry enabling EEE by default
> could upset some low latency use cases, PTP accuracy etc. So lets
> leave it as it is. Maybe a helper would be useful
> phy_advertise_eee_all() with a comment about why it could be used.
> 
Yes, I think that's the way to go.
To minimize efforts I'd like to keep this patch here as it is, then I'll
add the helper and change this place in r8169 to use the new helper.

> 	Andrew
Heiner