Re: [PATCH net v1 1/2] net: phy: set eee_cfg based on PHY configuration

[Choong Yong Liang <yong.liang.choong@linux.intel.com>]

On 14/11/2024 6:16 pm, Russell King (Oracle) wrote:
> On Thu, Nov 14, 2024 at 10:05:52AM +0000, Russell King (Oracle) wrote:
>> On Thu, Nov 14, 2024 at 09:23:48AM +0000, Russell King (Oracle) wrote:
>>> On Thu, Nov 14, 2024 at 04:16:52PM +0800, Choong Yong Liang wrote:
>>>> Not all PHYs have EEE enabled by default. For example, Marvell PHYs are
>>>> designed to have EEE hardware disabled during the initial state, and it
>>>> needs to be configured to turn it on again.
>>>>
>>>> This patch reads the PHY configuration and sets it as the initial value for
>>>> eee_cfg.tx_lpi_enabled and eee_cfg.eee_enabled instead of having them set to
>>>> true by default.
>>>
>>> eee_cfg.tx_lpi_enabled is something phylib tracks, and it merely means
>>> that LPI needs to be enabled at the MAC if EEE was negotiated:
>>>
>>>   * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
>>>   *      that eee was negotiated.
>>>
>>> eee_cfg.eee_enabled means that EEE mode was enabled - which is user
>>> configuration:
>>>
>>>   * @eee_enabled: EEE configured mode (enabled/disabled).
>>>
>>> phy_probe() reads the initial PHY state and sets things up
>>> appropriately.
>>>
>>> However, there is a point where the EEE configuration (advertisement,
>>> and therefore eee_enabled state) is written to the PHY, and that should
>>> be config_aneg(). Looking at the Marvell driver, it's calling
>>> genphy_config_aneg() which eventually calls
>>> genphy_c45_an_config_eee_aneg() which does this (via
>>> __genphy_config_aneg()).
>>>
>>> Please investigate why the hardware state is going out of sync with the
>>> software state.
>>
>> I think I've found the issue.
>>
>> We have phydev->eee_enabled and phydev->eee_cfg.eee_enabled, which looks
>> like a bug to me. We write to phydev->eee_cfg.eee_enabled in
>> phy_support_eee(), leaving phydev->eee_enabled untouched.
>>
>> However, most other places are using phydev->eee_enabled.
>>
>> This is (a) confusing and (b) wrong, and having the two members leads
>> to this confusion, and makes the code more difficult to follow (unless
>> one has already clocked that there are these two different things both
>> called eee_enabled).
>>
>> This is my untested prototype patch to fix this - it may cause breakage
>> elsewhere:
> 
> As mentioned in the other thread:
> 
> Without a call to phy_support_eee():
> 
> EEE settings for eth2:
>          EEE status: disabled
>          Tx LPI: disabled
>          Supported EEE link modes:  100baseT/Full
>                                     1000baseT/Full
>          Advertised EEE link modes:  Not reported
>          Link partner advertised EEE link modes:  100baseT/Full
>                                                   1000baseT/Full
> 
> With a call to phy_support_eee():
> 
> EEE settings for eth2:
>          EEE status: enabled - active
>          Tx LPI: 0 (us)
>          Supported EEE link modes:  100baseT/Full
>                                     1000baseT/Full
>          Advertised EEE link modes:  100baseT/Full
>                                      1000baseT/Full
>          Link partner advertised EEE link modes:  100baseT/Full
>                                                   1000baseT/Full
> 
> So the EEE status is now behaving correctly, and the Marvell PHY is
> being programmed with the advertisement correctly.
> 

Thank you for all the suggestions, the provided prototype, and the tested 
results.

I will study the suggestions in depth, test the provided prototype, and 
provide more feedback.