Re: [PATCH net-next V3 4/4] net: lan743x: Add support to ethtool phylink get and set settings

[Russell King (Oracle)]

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@xxxxxxx>
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.

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.

>  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?

> @@ -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.

> +	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.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!