Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 21 Jul 2023 09:53:33 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
Cc:     netdev@vger.kernel.org, davem@davemloft.net, kuba@kernel.org,
        linux-kernel@vger.kernel.org, bryan.whitehead@microchip.com,
        andrew@lunn.ch, UNGLinuxDriver@microchip.com
Subject: Re: [PATCH net-next 6/7] net: lan743x: Add support to ethtool
 phylink get and set settings
Message-ID: <ZLpHjQsBkC8SXQHy@shell.armlinux.org.uk>
References: <20230721060019.2737-1-Raju.Lakkaraju@microchip.com>
 <20230721060019.2737-7-Raju.Lakkaraju@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230721060019.2737-7-Raju.Lakkaraju@microchip.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Fri, Jul 21, 2023 at 11:30:18AM +0530, Raju Lakkaraju wrote:
> Add support to ethtool phylink get/set settings for the network interface via
> ethtool like speed, duplex etc.
> 
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> ---
>  .../net/ethernet/microchip/lan743x_ethtool.c  | 34 +++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> index 2db5949b4c7e..3004863bebf8 100644
> --- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
> +++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
> @@ -1134,6 +1134,36 @@ static int lan743x_ethtool_set_eee(struct net_device *netdev,
>  	return phy_ethtool_set_eee(phydev, eee);
>  }
>  
> +static int lan743x_ethtool_set_link_ksettings(struct net_device *netdev,
> +					      const struct ethtool_link_ksettings *cmd)
> +{
> +	struct lan743x_adapter *adapter = netdev_priv(netdev);
> +	int ret;
> +
> +	if (adapter->phylink) {
> +		ret = lan743x_lsd_update(cmd->base.speed, cmd->base.duplex,
> +					 cmd->base.master_slave_state);
> +		if (ret < 0)
> +			return ret;
> +
> +		adapter->sgmii_lsd = ret;

I guess this is how you are hacking SGMII to do what you want - using
the ethtool master_slave_state to control whether SGMII is operating as
a "host" or a "PHY", and I would say it's a hack to make stuff work the
way you want it to, so you can run SGMII over a DAC cable at speeds
such as 100M. I really don't see what the point of that is.

In any case, doing this is buggy. You update state, and then call
into phylink. What if phylink returns an error? You've updated the
state which may take effect if the link goes up/down but maybe the
settings were rejected by phylink.

Sorry, but no, this to me is a gross hack.

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