| 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
| ||
|
Message-ID: <3915908d-d70b-4fbe-b80b-990d02211965@gmail.com>
Date: Sat, 16 Nov 2024 18:41:13 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Choong Yong Liang <yong.liang.choong@...ux.intel.com>,
Andrew Lunn <andrew@...n.ch>, "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 v2 0/2] Fix 'ethtool --show-eee' during initial stage
On 16.11.2024 16:34, Russell King (Oracle) wrote:
> On Fri, Nov 15, 2024 at 09:35:25PM +0100, Heiner Kallweit wrote:
>> On 15.11.2024 15:12, Russell King (Oracle) wrote:
>>> On Fri, Nov 15, 2024 at 02:41:54PM +0100, Heiner Kallweit wrote:
>>>> On 15.11.2024 12:11, Choong Yong Liang wrote:
>>>>> From: Choong Yong Liang <yong.liang.choong@...el.com>
>>>>>
>>>>> When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
>>>>> the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
>>>>> the driver side is disabled. If we try to enable EEE through
>>>>> 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
>>>>> matches the setting required to enable EEE in ethnl_set_eee().
>>>>>
>>>>> This patch series will remove phydev->eee_enabled and replace it with
>>>>> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
>>>>> will follow the master configuration to have software and hardware in sync,
>>>>> allowing 'ethtool --show-eee' to display the correct value during the
>>>>> initial stage.
>>>>>
>>>>> v2 changes:
>>>>> - Implement the prototype suggested by Russell
>>>>> - Check EEE before calling phy_support_eee()
>>>>>
>>>>> Thanks to Russell for the proposed prototype in [1].
>>>>>
>>>>> Reference:
>>>>> [1] https://patchwork.kernel.org/comment/26121323/
>>>>>
>>>>> Choong Yong Liang (2):
>>>>> net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
>>>>> net: stmmac: set initial EEE policy configuration
>>>>>
>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
>>>>> drivers/net/phy/phy-c45.c | 11 +++++------
>>>>> drivers/net/phy/phy_device.c | 6 +++---
>>>>> include/linux/phy.h | 5 ++---
>>>>> 4 files changed, 13 insertions(+), 12 deletions(-)
>>>>>
>>>>
>>>> Russell submitted the proposed patch already:
>>>> https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
>>>> So there's no need for your patch 1.
>>>
>>> Patch 1 is an updated version of that patch, minus my authorship and of
>>> course no sign-off. I've already marked this series as requiring changes
>>> in patchwork (hopefully, if I did it correctly.)
>>>
>>
>> The updated version adds an argument to genphy_c45_an_config_eee_aneg(),
>> and I wonder whether we can do better, as this results in several calls
>> with the same argument. The following is an alternative, to be applied
>> on top of your original patch. I don't have a clear preference, though.
>>
>> ---
>> drivers/net/phy/phy.c | 25 +++++++++++++++----------
>> 1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 8876f3673..22c9bbebb 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -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)
>> + struct ethtool_keee *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,21 +1705,27 @@ 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;
>>
>> + old_cfg = phydev->eee_cfg;
>> + eee_to_eeecfg(&phydev->eee_cfg, data);
>> +
>
> Hmm, don't we want to do this under phydev->lock, because network
> drivers and phylib may be reading from phydev->eee_cfg? If we
> update it outside the lock, and then revert, there's a chance that
> the phylib state machine / network driver may see the changes
> which then get reverted on failure, potentially leading to
> inconsistent state.
>
Good point, then the patch would look like this.
BTW: Saw that Jakub applied your patch already.
---
drivers/net/phy/phy.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8876f3673..7f6594a66 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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)
+ struct ethtool_keee *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, data);
+ else if (ret < 0)
+ phydev->eee_cfg = old_cfg;
+
mutex_unlock(&phydev->lock);
return ret < 0 ? ret : 0;
--
2.47.0
Powered by blists - more mailing lists