| 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: <Zqtr/ezTMkButA8j@HYD-DK-UNGSW21.microchip.com>
Date: Thu, 1 Aug 2024 16:35:33 +0530
From: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
CC: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>, <netdev@...r.kernel.org>,
<davem@...emloft.net>, <kuba@...nel.org>, <andrew@...n.ch>,
<horms@...nel.org>, <hkallweit1@...il.com>, <richardcochran@...il.com>,
<rdunlap@...radead.org>, <Bryan.Whitehead@...rochip.com>,
<edumazet@...gle.com>, <pabeni@...hat.com>, <linux-kernel@...r.kernel.org>,
<UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net-next V3 4/4] net: lan743x: Add support to ethtool
phylink get and set settings
Hi Russell King,
The 07/30/2024 16:11, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Tue, Jul 30, 2024 at 07:36:19PM +0530, Raju Lakkaraju wrote:
> > diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > index 3a63ec091413..a649ea7442a4 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> > @@ -1058,61 +1058,48 @@ static int lan743x_ethtool_get_eee(struct net_device *netdev,
> > struct ethtool_keee *eee)
> > {
> > struct lan743x_adapter *adapter = netdev_priv(netdev);
> > - struct phy_device *phydev = netdev->phydev;
> > - u32 buf;
> > - int ret;
> > -
> > - if (!phydev)
> > - return -EIO;
> > - if (!phydev->drv) {
> > - netif_err(adapter, drv, adapter->netdev,
> > - "Missing PHY Driver\n");
> > - return -EIO;
> > - }
> >
> > - ret = phy_ethtool_get_eee(phydev, eee);
> > - if (ret < 0)
> > - return ret;
> > -
> > - buf = lan743x_csr_read(adapter, MAC_CR);
> > - if (buf & MAC_CR_EEE_EN_) {
> > - /* EEE_TX_LPI_REQ_DLY & tx_lpi_timer are same uSec unit */
> > - buf = lan743x_csr_read(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT);
> > - eee->tx_lpi_timer = buf;
> > - } else {
> > - eee->tx_lpi_timer = 0;
> > - }
> > + eee->tx_lpi_timer = lan743x_csr_read(adapter,
> > + MAC_EEE_TX_LPI_REQ_DLY_CNT);
> > + eee->eee_enabled = adapter->eee_enabled;
> > + eee->eee_active = adapter->eee_active;
> > + eee->tx_lpi_enabled = adapter->tx_lpi_enabled;
>
> You really need to start paying attention to the commits other people
> make as a result of development to other parts of the kernel.
>
> First, see:
>
> commit ef460a8986fa0dae1cdcb158a06127f7af27c92d
> Author: Andrew Lunn <andrew@...n.ch>
> Date: Sat Apr 6 15:16:00 2024 -0500
>
> net: lan743x: Fixup EEE
>
> and note that the assignment of eee->eee_enabled, eee->eee_active, and
> eee->tx_lpi_enabled were all removed.
>
Accepted. I will fix
> Next...
>
> >
> > - return 0;
> > + return phylink_ethtool_get_eee(adapter->phylink, eee);
>
> phylink_ethtool_get_eee() will call phy_ethtool_get_eee(), which
> in turn calls genphy_c45_ethtool_get_eee() and eeecfg_to_eee().
>
> genphy_c45_ethtool_get_eee() will do this:
>
> data->eee_enabled = is_enabled;
> data->eee_active = ret;
>
> thus overwriting your assignment to eee->eee_enabled and
> eee->eee_active.
>
> eeecfg_to_eee() will overwrite eee->tx_lpi_enabled and
> eee->eee_enabled.
>
> Thus, writing to these fields and then calling
> phylink_ethtool_get_eee() results in these fields being overwritten.
>
Ok. I will fix.
> > static int lan743x_ethtool_set_eee(struct net_device *netdev,
> > struct ethtool_keee *eee)
> > {
> > - struct lan743x_adapter *adapter;
> > - struct phy_device *phydev;
> > - u32 buf = 0;
> > + struct lan743x_adapter *adapter = netdev_priv(netdev);
> >
> > - if (!netdev)
> > - return -EINVAL;
> > - adapter = netdev_priv(netdev);
> > - if (!adapter)
> > - return -EINVAL;
> > - phydev = netdev->phydev;
> > - if (!phydev)
> > - return -EIO;
> > - if (!phydev->drv) {
> > - netif_err(adapter, drv, adapter->netdev,
> > - "Missing PHY Driver\n");
> > - return -EIO;
> > - }
> > + if (eee->tx_lpi_enabled)
> > + lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT,
> > + eee->tx_lpi_timer);
> > + else
> > + lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, 0);
> >
> > - if (eee->eee_enabled) {
> > - buf = (u32)eee->tx_lpi_timer;
> > - lan743x_csr_write(adapter, MAC_EEE_TX_LPI_REQ_DLY_CNT, buf);
> > - }
> > + adapter->eee_enabled = eee->eee_enabled;
> > + adapter->tx_lpi_enabled = eee->tx_lpi_enabled;
>
> Given that phylib stores these and overwrites your copies in the get_eee
> method above, do you need to store these?
>
OK
> > @@ -3013,7 +3025,12 @@ static void lan743x_phylink_mac_link_down(struct phylink_config *config,
> > unsigned int link_an_mode,
> > phy_interface_t interface)
> > {
> > + struct net_device *netdev = to_net_dev(config->dev);
> > + struct lan743x_adapter *adapter = netdev_priv(netdev);
> > +
> > netif_tx_stop_all_queues(to_net_dev(config->dev));
> > + adapter->eee_active = false;
>
> phylib tracks this for you.
>
Ok
> > + lan743x_set_eee(adapter, false);
> > }
> >
> > static void lan743x_phylink_mac_link_up(struct phylink_config *config,
> > @@ -3055,6 +3072,14 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config,
> > cap & FLOW_CTRL_TX,
> > cap & FLOW_CTRL_RX);
> >
> > + if (phydev && adapter->eee_enabled) {
> > + bool enable;
> > +
> > + adapter->eee_active = phy_init_eee(phydev, false) >= 0;
> > + enable = adapter->eee_active && adapter->tx_lpi_enabled;
>
> No need. Your enable should be conditional on phydev->enable_tx_lpi
> here. See Andrew's commit and understand his changes, rather than
> just ignoring Andrew's work and continuing with your old pattern of
> EEE support. Things have moved forward.
Ok. I will fix
>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
--
Thanks,
Raju
Powered by blists - more mailing lists