| 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: <c2a16e3c-374c-4fd1-9ca7-bf0aeb5ed941@broadcom.com>
Date: Wed, 10 Apr 2024 10:48:26 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Doug Berger <opendmb@...il.com>,
Broadcom internal kernel review list
<bcm-kernel-feedback-list@...adcom.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Oleksij Rempel <o.rempel@...gutronix.de>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: genet: Fixup EEE
Hi Andrew,
On 4/9/2024 2:37 PM, Andrew Lunn wrote:
> On Tue, Apr 09, 2024 at 01:41:43PM -0700, Florian Fainelli wrote:
>> On 4/8/24 17:54, Andrew Lunn wrote:
>>> The enabling/disabling of EEE in the MAC should happen as a result of
>>> auto negotiation. So move the enable/disable into bcmgenet_mii_setup()
>>> which gets called by phylib when there is a change in link status.
>>>
>>> bcmgenet_set_eee() now just writes the LPI timer value to the
>>> hardware. Everything else is passed to phylib, so it can correctly
>>> setup the PHY.
>>>
>>> bcmgenet_get_eee() relies on phylib doing most of the work, the MAC
>>> driver just adds the LPI timer value from hardware.
>>>
>>> Signed-off-by: Andrew Lunn <andrew@...n.ch>
>>> ---
>>> drivers/net/ethernet/broadcom/genet/bcmgenet.c | 26 ++++++--------------------
>>> drivers/net/ethernet/broadcom/genet/bcmgenet.h | 6 +-----
>>> drivers/net/ethernet/broadcom/genet/bcmmii.c | 7 +------
>>> 3 files changed, 8 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> index b1f84b37032a..c090b519255a 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>>> @@ -1272,13 +1272,14 @@ static void bcmgenet_get_ethtool_stats(struct net_device *dev,
>>> }
>>> }
>>> -void bcmgenet_eee_enable_set(struct net_device *dev, bool enable,
>>> - bool tx_lpi_enabled)
>>> +void bcmgenet_eee_enable_set(struct net_device *dev, bool enable)
>>> {
>>> struct bcmgenet_priv *priv = netdev_priv(dev);
>>> - u32 off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
>>> + u32 off;
>>> u32 reg;
>>> + off = priv->hw_params->tbuf_offset + TBUF_ENERGY_CTRL;
>>> +
>>> if (enable && !priv->clk_eee_enabled) {
>>> clk_prepare_enable(priv->clk_eee);
>>> priv->clk_eee_enabled = true;
>>> @@ -1293,7 +1294,7 @@ void bcmgenet_eee_enable_set(struct net_device *dev, bool enable,
>>> /* Enable EEE and switch to a 27Mhz clock automatically */
>>> reg = bcmgenet_readl(priv->base + off);
>>> - if (tx_lpi_enabled)
>>> + if (enable)
>>> reg |= TBUF_EEE_EN | TBUF_PM_EN;
>>> else
>>> reg &= ~(TBUF_EEE_EN | TBUF_PM_EN);
>>> @@ -1311,15 +1312,11 @@ void bcmgenet_eee_enable_set(struct net_device *dev, bool enable,
>>> clk_disable_unprepare(priv->clk_eee);
>>> priv->clk_eee_enabled = false;
>>> }
>>> -
>>> - priv->eee.eee_enabled = enable;
>>> - priv->eee.tx_lpi_enabled = tx_lpi_enabled;
>>> }
>>> static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e)
>>> {
>>> struct bcmgenet_priv *priv = netdev_priv(dev);
>>> - struct ethtool_keee *p = &priv->eee;
>>> if (GENET_IS_V1(priv))
>>> return -EOPNOTSUPP;
>>> @@ -1327,7 +1324,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e)
>>> if (!dev->phydev)
>>> return -ENODEV;
>>> - e->tx_lpi_enabled = p->tx_lpi_enabled;
>>> e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);
>>> return phy_ethtool_get_eee(dev->phydev, e);
>>> @@ -1336,8 +1332,6 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e)
>>> static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e)
>>> {
>>> struct bcmgenet_priv *priv = netdev_priv(dev);
>>> - struct ethtool_keee *p = &priv->eee;
>>> - bool active;
>>> if (GENET_IS_V1(priv))
>>> return -EOPNOTSUPP;
>>> @@ -1345,15 +1339,7 @@ static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e)
>>> if (!dev->phydev)
>>> return -ENODEV;
>>> - p->eee_enabled = e->eee_enabled;
>>> -
>>> - if (!p->eee_enabled) {
>>> - bcmgenet_eee_enable_set(dev, false, false);
>>> - } else {
>>> - active = phy_init_eee(dev->phydev, false) >= 0;
>>> - bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
>>> - bcmgenet_eee_enable_set(dev, active, e->tx_lpi_enabled);
>>> - }
>>> + bcmgenet_umac_writel(priv, e->tx_lpi_timer, UMAC_EEE_LPI_TIMER);
>>> return phy_ethtool_set_eee(dev->phydev, e);
>>> }
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>> index 7523b60b3c1c..bb82ecb2e220 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
>>> @@ -644,8 +644,6 @@ struct bcmgenet_priv {
>>> bool wol_active;
>>> struct bcmgenet_mib_counters mib;
>>> -
>>> - struct ethtool_keee eee;
>>> };
>>> #define GENET_IO_MACRO(name, offset) \
>>> @@ -703,7 +701,5 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
>>> void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
>>> enum bcmgenet_power_mode mode);
>>> -void bcmgenet_eee_enable_set(struct net_device *dev, bool enable,
>>> - bool tx_lpi_enabled);
>>> -
>>> +void bcmgenet_eee_enable_set(struct net_device *dev, bool enable);
>>> #endif /* __BCMGENET_H__ */
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> index 9ada89355747..25eeea4c1965 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> @@ -30,7 +30,6 @@ static void bcmgenet_mac_config(struct net_device *dev)
>>> struct bcmgenet_priv *priv = netdev_priv(dev);
>>> struct phy_device *phydev = dev->phydev;
>>> u32 reg, cmd_bits = 0;
>>> - bool active;
>>> /* speed */
>>> if (phydev->speed == SPEED_1000)
>>> @@ -88,11 +87,6 @@ static void bcmgenet_mac_config(struct net_device *dev)
>>> reg |= CMD_TX_EN | CMD_RX_EN;
>>> }
>>> bcmgenet_umac_writel(priv, reg, UMAC_CMD);
>>> -
>>> - active = phy_init_eee(phydev, 0) >= 0;
>>> - bcmgenet_eee_enable_set(dev,
>>> - priv->eee.eee_enabled && active,
>>> - priv->eee.tx_lpi_enabled);
>>
>> You can keep the call to bcmgenet_eee_enable_set() where it currently is and
>> just change the arguments?
>
> Hi Florian
>
> bcmgenet_eee_enable_set() configures the hardware. We should only call
> that once auto-neg has completed and the adjust_link callback is
> called. Or in the case EEE autoneg is disabled, phylib will
> artificially down/up the link in order to call the adjust_link
> callback.
>
> bcmgenet_eee_enable_set() is currently called in bcmgenet_set_eee(),
> which is the .set_eee ethtool_op. So that is the wrong place to do
> this. That is what the last hunk in the patch does, it moves it to
> bcmgenet_mii_setup() which is passed to of_phy_connect() as the
> callback.
>
> Or am i missing thing?
You are not, I was missing the two call sites for bcmgenet_mac_config()
and indeed only the one in bcmgenet_mii_setup() is meaningful here.
I am seeing a functional difference with and without your patch however,
and also, there appears to be something wrong within the bcmgenet driver
after PHYLIB having absorbed the EEE configuration. Both cases we start
on boot with:
# ethtool --show-eee eth0
EEE settings for eth0:
EEE status: disabled
Tx LPI: disabled
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
I would expect the EEE status to be enabled, that's how I remember it
before. Now, with your patch, once I turn on EEE with:
# ethtool --set-eee eth0 eee on
# ethtool --show-eee eth0
EEE settings for eth0:
EEE status: enabled - active
Tx LPI: disabled
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
#
there is no change to the EEE_CTRL register to set the EEE_EN, this only
happens when doing:
# ethtool --set-eee eth0 eee on tx-lpi on
which is consistent with the patch, but I don't think this is quite
correct as I remembered that "eee on" meant enable EEE for the RX path,
and "tx-lpi on" meant enable EEE for the TX path?
--
Florian
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)
Powered by blists - more mailing lists