| 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: <67c0777f-f66e-4293-af8b-08e0c4ab0acc@lunn.ch>
Date: Tue, 9 Apr 2024 23:37:11 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Florian Fainelli <florian.fainelli@...adcom.com>
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
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?
Andrew
Powered by blists - more mailing lists