[PATCH] net: mana: Add get_link and get_link_ksettings in ethtool

[PATCH] net: mana: Add get_link and get_link_ksettings in ethtool

[Erni Sri Satya Vennela]

Add support for the ethtool get_link and get_link_ksettings
operations. Display standard port information using ethtool.

Before the change:
$ethtool enP30832s1
>No data available

After the change:
$ethtool enP30832s1
>Settings for enP30832s1:
        Supported ports: [  ]
        Supported link modes:   Not reported
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  Not reported
        Advertised pause frame use: No
        Advertised auto-negotiation: Yes
        Advertised FEC modes: Not reported
        Speed: Unknown!
        Duplex: Full
        Auto-negotiation: on
        Port: Direct Attach Copper
        PHYAD: 0
        Transceiver: internal
        Link detected: yes

Signed-off-by: Erni Sri Satya Vennela <ernis@linux.microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Reviewed-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
---
 .../ethernet/microsoft/mana/mana_ethtool.c    | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
index 146d5db1792f..c2716d6cad36 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
@@ -369,6 +369,24 @@ static int mana_set_channels(struct net_device *ndev,
 	return err;
 }
 
+static int mana_get_link_ksettings(struct net_device *ndev,
+				   struct ethtool_link_ksettings *cmd)
+{
+	cmd->base.duplex = DUPLEX_FULL;
+	cmd->base.autoneg = AUTONEG_ENABLE;
+	cmd->base.port = PORT_DA;
+
+	ethtool_link_ksettings_zero_link_mode(cmd, supported);
+	ethtool_link_ksettings_zero_link_mode(cmd, advertising);
+
+	ethtool_link_ksettings_add_link_mode(cmd, supported,
+					     Autoneg);
+	ethtool_link_ksettings_add_link_mode(cmd, advertising,
+					     Autoneg);
+
+	return 0;
+}
+
 const struct ethtool_ops mana_ethtool_ops = {
 	.get_ethtool_stats	= mana_get_ethtool_stats,
 	.get_sset_count		= mana_get_sset_count,
@@ -380,4 +398,6 @@ const struct ethtool_ops mana_ethtool_ops = {
 	.set_rxfh		= mana_set_rxfh,
 	.get_channels		= mana_get_channels,
 	.set_channels		= mana_set_channels,
+	.get_link_ksettings	= mana_get_link_ksettings,
+	.get_link		= ethtool_op_get_link,
 };
-- 
2.34.1

Re: [PATCH] net: mana: Add get_link and get_link_ksettings in ethtool

[Jakub Kicinski]

On Thu, 12 Sep 2024 00:44:43 -0700 Erni Sri Satya Vennela wrote:
> Add support for the ethtool get_link and get_link_ksettings
> operations. Display standard port information using ethtool.

Any reason why? Sometimes people add this callback for virtual
devices to expose some approximate speed, but you're not reporting
speed, so I'm curious.

> +static int mana_get_link_ksettings(struct net_device *ndev,
> +				   struct ethtool_link_ksettings *cmd)
> +{
> +	cmd->base.duplex = DUPLEX_FULL;

make sense

> +	cmd->base.autoneg = AUTONEG_ENABLE;

what's the point of autoneg if we show no link info?
DISABLE seems more suitable

> +	cmd->base.port = PORT_DA;

Any reason why DA? I'd think PORT_OTHER may be better?
-- 
pw-bot: cr

RE: [PATCH] net: mana: Add get_link and get_link_ksettings in ethtool

[Haiyang Zhang]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, September 13, 2024 11:24 PM
> To: Erni Sri Satya Vennela <ernis@linux.microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; davem@davemloft.net; edumazet@google.com;
> pabeni@redhat.com; shradhagupta@linux.microsoft.com;
> ahmed.zaki@intel.com; colin.i.king@gmail.com; linux-
> hyperv@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] net: mana: Add get_link and get_link_ksettings in
> ethtool
> 
> On Thu, 12 Sep 2024 00:44:43 -0700 Erni Sri Satya Vennela wrote:
> > Add support for the ethtool get_link and get_link_ksettings
> > operations. Display standard port information using ethtool.
> 
> Any reason why? Sometimes people add this callback for virtual
> devices to expose some approximate speed, but you're not reporting
> speed, so I'm curious.
Speed info isn't available from the HW yet. But we are requesting 
that from HW team. For now, we just add some minimal info, like 
duplex, etc.

> 
> > +static int mana_get_link_ksettings(struct net_device *ndev,
> > +				   struct ethtool_link_ksettings *cmd)
> > +{
> > +	cmd->base.duplex = DUPLEX_FULL;
> 
> make sense
> 
> > +	cmd->base.autoneg = AUTONEG_ENABLE;
> 
> what's the point of autoneg if we show no link info?
> DISABLE seems more suitable
We don't have strong opinion on this one.
@Vennela, you may remove the 3 items related to autoneg.

> 
> > +	cmd->base.port = PORT_DA;
> 
> Any reason why DA? I'd think PORT_OTHER may be better?
I'm OK with PORT_OTHER too :)

Thanks,
- Haiyang

Re: [PATCH] net: mana: Add get_link and get_link_ksettings in ethtool

[Jakub Kicinski]

On Tue, 17 Sep 2024 14:35:21 +0000 Haiyang Zhang wrote:
> > Any reason why? Sometimes people add this callback for virtual
> > devices to expose some approximate speed, but you're not reporting
> > speed, so I'm curious.  
> Speed info isn't available from the HW yet. But we are requesting 
> that from HW team. For now, we just add some minimal info, like 
> duplex, etc.

Unless I'm misreading I don't see the answer to the "why?" in your
reply.

What benefit does reporting duplex on a virtual device bring?
What kind of SW need this current patch?
etc.

RE: [PATCH] net: mana: Add get_link and get_link_ksettings in ethtool

[Haiyang Zhang]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, September 17, 2024 11:04 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Erni Sri Satya Vennela <ernis@linux.microsoft.com>; KY Srinivasan
> <kys@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
> <decui@microsoft.com>; davem@davemloft.net; edumazet@google.com;
> pabeni@redhat.com; shradhagupta@linux.microsoft.com;
> ahmed.zaki@intel.com; colin.i.king@gmail.com; linux-
> hyperv@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] net: mana: Add get_link and get_link_ksettings in
> ethtool
> 
> On Tue, 17 Sep 2024 14:35:21 +0000 Haiyang Zhang wrote:
> > > Any reason why? Sometimes people add this callback for virtual
> > > devices to expose some approximate speed, but you're not reporting
> > > speed, so I'm curious.
> > Speed info isn't available from the HW yet. But we are requesting
> > that from HW team. For now, we just add some minimal info, like
> > duplex, etc.
> 
> Unless I'm misreading I don't see the answer to the "why?" in your
> reply.
> 
> What benefit does reporting duplex on a virtual device bring?
> What kind of SW need this current patch?
> etc.

I'm not aware of any SW has such requirement either.
We just want the "ethtool <nic>" cmd can output something we already know.
Is this acceptable?

Thanks,
- Haiyang

Re: [PATCH] net: mana: Add get_link and get_link_ksettings in ethtool

[Jakub Kicinski]

On Tue, 17 Sep 2024 20:34:40 +0000 Haiyang Zhang wrote:
> > Unless I'm misreading I don't see the answer to the "why?" in your
> > reply.
> > 
> > What benefit does reporting duplex on a virtual device bring?
> > What kind of SW need this current patch?
> > etc.  
> 
> I'm not aware of any SW has such requirement either.
> We just want the "ethtool <nic>" cmd can output something we already know.
> Is this acceptable?

Yeah, that's fine, I was just curious. 

Re: [PATCH] net: mana: Add get_link and get_link_ksettings in ethtool

[Erni Sri Satya Vennela]

On Thu, Sep 19, 2024 at 09:45:53AM +0200, Jakub Kicinski wrote:
> On Tue, 17 Sep 2024 20:34:40 +0000 Haiyang Zhang wrote:
> > > Unless I'm misreading I don't see the answer to the "why?" in your
> > > reply.
> > > 
> > > What benefit does reporting duplex on a virtual device bring?
> > > What kind of SW need this current patch?
> > > etc.  
> > 
> > I'm not aware of any SW has such requirement either.
> > We just want the "ethtool <nic>" cmd can output something we already know.
> > Is this acceptable?
> 
> Yeah, that's fine, I was just curious. 
Thankyou for the feedback. I will update these changes in the next
version of the patch.