| Message ID | 1726127083-28538-1-git-send-email-ernis@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mana: Add get_link and get_link_ksettings in ethtool | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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?
> -----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
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.
> -----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
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, };