linux-kernel - Re: [PATCH net v1 1/2] net: phy: set 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] [day] [month] [year] [list]

Message-ID: <6c023200-5e81-432c-b21d-d7a9cf1bfc92@linux.intel.com>
Date: Fri, 15 Nov 2024 09:46:07 +0800
From: Choong Yong Liang <yong.liang.choong@...ux.intel.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Jose Abreu <joabreu@...opsys.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Oleksij Rempel <o.rempel@...gutronix.de>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH net v1 1/2] net: phy: set eee_cfg based on PHY
 configuration



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.