[net-next,6/7] net: lan743x: Add support to ethtool phylink get and set settings
Commit Message
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(-)
Comments
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.
@@ -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;
+ return phylink_ethtool_ksettings_set(adapter->phylink, cmd);
+ }
+
+ return phy_ethtool_set_link_ksettings(netdev, cmd);
+}
+
+static int lan743x_ethtool_get_link_ksettings(struct net_device *netdev,
+ struct ethtool_link_ksettings *cmd)
+{
+ struct lan743x_adapter *adapter = netdev_priv(netdev);
+
+ if (adapter->phylink)
+ return phylink_ethtool_ksettings_get(adapter->phylink, cmd);
+
+ return phy_ethtool_get_link_ksettings(netdev, cmd);
+}
+
#ifdef CONFIG_PM
static void lan743x_ethtool_get_wol(struct net_device *netdev,
struct ethtool_wolinfo *wol)
@@ -1400,8 +1430,8 @@ const struct ethtool_ops lan743x_ethtool_ops = {
.get_ts_info = lan743x_ethtool_get_ts_info,
.get_eee = lan743x_ethtool_get_eee,
.set_eee = lan743x_ethtool_set_eee,
- .get_link_ksettings = phy_ethtool_get_link_ksettings,
- .set_link_ksettings = phy_ethtool_set_link_ksettings,
+ .get_link_ksettings = lan743x_ethtool_get_link_ksettings,
+ .set_link_ksettings = lan743x_ethtool_set_link_ksettings,
.get_regs_len = lan743x_get_regs_len,
.get_regs = lan743x_get_regs,
.get_pauseparam = lan743x_get_pauseparam,