diff mbox series

[net-next] net: tn40xx: add initial ethtool_ops support

Message ID 20240628134116.120209-1-fujita.tomonori@gmail.com (mailing list archive)
State Accepted
Commit 7433d034ac3c2ea745ee345228122dbfbeca5b20
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: tn40xx: add initial ethtool_ops support | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 839 this patch: 839
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 846 this patch: 846
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 846 this patch: 846
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 26 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-28--21-00 (tests: 665)

Commit Message

FUJITA Tomonori June 28, 2024, 1:41 p.m. UTC
Call phylink_ethtool_ksettings_get() for get_link_ksettings method and
ethtool_op_get_link() for get_link method.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 drivers/net/ethernet/tehuti/tn40.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)


base-commit: 94833addfaba89d12e5dbd82e350a692c00648ab

Comments

Andrew Lunn June 28, 2024, 2:14 p.m. UTC | #1
On Fri, Jun 28, 2024 at 10:41:16PM +0900, FUJITA Tomonori wrote:
> Call phylink_ethtool_ksettings_get() for get_link_ksettings method and
> ethtool_op_get_link() for get_link method.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  drivers/net/ethernet/tehuti/tn40.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/ethernet/tehuti/tn40.c b/drivers/net/ethernet/tehuti/tn40.c
> index 11db9fde11fe..565b72537efa 100644
> --- a/drivers/net/ethernet/tehuti/tn40.c
> +++ b/drivers/net/ethernet/tehuti/tn40.c
> @@ -1571,6 +1571,19 @@ static const struct net_device_ops tn40_netdev_ops = {
>  	.ndo_vlan_rx_kill_vid = tn40_vlan_rx_kill_vid,
>  };
>  
> +static int tn40_ethtool_get_link_ksettings(struct net_device *ndev,
> +					   struct ethtool_link_ksettings *cmd)
> +{
> +	struct tn40_priv *priv = netdev_priv(ndev);
> +
> +	return phylink_ethtool_ksettings_get(priv->phylink, cmd);
> +}

Have you tried implementing tn40_ethtool_set_link_ksettings() in the
same way?

This patch is however O.K. as is:

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
FUJITA Tomonori June 28, 2024, 2:29 p.m. UTC | #2
On Fri, 28 Jun 2024 16:14:44 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +static int tn40_ethtool_get_link_ksettings(struct net_device *ndev,
>> +					   struct ethtool_link_ksettings *cmd)
>> +{
>> +	struct tn40_priv *priv = netdev_priv(ndev);
>> +
>> +	return phylink_ethtool_ksettings_get(priv->phylink, cmd);
>> +}
> 
> Have you tried implementing tn40_ethtool_set_link_ksettings() in the
> same way?

Not yet. I'll try. The original driver doesn't support
ethtool_set_link_ksettings().


> This patch is however O.K. as is:
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Thanks!
Russell King (Oracle) June 28, 2024, 3:45 p.m. UTC | #3
On Fri, Jun 28, 2024 at 04:14:44PM +0200, Andrew Lunn wrote:
> On Fri, Jun 28, 2024 at 10:41:16PM +0900, FUJITA Tomonori wrote:
> > Call phylink_ethtool_ksettings_get() for get_link_ksettings method and
> > ethtool_op_get_link() for get_link method.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > ---
> >  drivers/net/ethernet/tehuti/tn40.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/tehuti/tn40.c b/drivers/net/ethernet/tehuti/tn40.c
> > index 11db9fde11fe..565b72537efa 100644
> > --- a/drivers/net/ethernet/tehuti/tn40.c
> > +++ b/drivers/net/ethernet/tehuti/tn40.c
> > @@ -1571,6 +1571,19 @@ static const struct net_device_ops tn40_netdev_ops = {
> >  	.ndo_vlan_rx_kill_vid = tn40_vlan_rx_kill_vid,
> >  };
> >  
> > +static int tn40_ethtool_get_link_ksettings(struct net_device *ndev,
> > +					   struct ethtool_link_ksettings *cmd)
> > +{
> > +	struct tn40_priv *priv = netdev_priv(ndev);
> > +
> > +	return phylink_ethtool_ksettings_get(priv->phylink, cmd);
> > +}
> 
> Have you tried implementing tn40_ethtool_set_link_ksettings() in the
> same way?

I did think about commenting on that, and the [sg]et_pauseparam
methods as well, but when one realises that the driver only supports
one speed and duplex (10G FD) but no pause it didn't seem to make
sense.

Not having pause effectively rules out pause-frame rate adaption
by the PHY, so the PHY probably only supports 10G link speeds,
and if I remember correctly, 10GBASE-T requires autoneg.

Deviating off from this topic a bit... 802.3 28D.5:

28D.5 Extensions required for Clause 40 (1000BASE-T)

  a)   Auto-Negotiation is mandatory for 1000BASE-T (see 40.5.1).

28D.6 Extensions required for Clause 55 (10GBASE-T)

  a)   Auto-Negotiation is mandatory for 10GBASE-T.

Now, delving into the PICS for 1000BASE-T, it states:

 Item            Feature                  Subclause   Status
 Support         Value/Comment
 AN        Support for Auto-Negotiation   40.5.1      M     Yes [ ] Required

which doesn't seem to mean that AN must be enabled, only support for
AN is required and it's possible to disable it. nothing states that
disabling AN for 1000base-T is not allowed.

The same seems to be true of 10GBASE-T.

However, wikipedia says:

The autonegotiation specification was improved in the 1998 release of
IEEE 802.3. This was followed by the release of the IEEE 802.3ab Gigabit
Ethernet standard in 1999 which specified mandatory autonegotiation for
1000BASE-T. Autonegotiation is also mandatory for 1000BASE-TX and
10GBASE-T implementations.

which is loose language - "mandatory autonegotiation" does that refer
to support for auto-negotiation or require auto-negotiation to be
always enabled?

We're already seeing some PHYs from some manufacturers that seem to be
following the "require auto-negotiation to be always enabled".

So why have I gone down what seems to be an unrelated rabbit hole?

If tn40 is connected to a 10GBASE-T PHY, implementing the
set_link_ksettings() method would give the user control over whether AN
is used on the media side.

If 802.3 requires AN to be supported but is not necessarily enabled,
then there is use in exposing the set_link_ksettings() method.

If 802.3 requires AN to be supproted and always enabled, then
implementing set_link_ksettings() in this case would not provide any
value.

Which it is... I have no idea. Nothing seems to be giving a clear
unambiguous definitive statement.

The last thing to point out, however, is phylib's behaviour. If autoneg
is disabled, we only allow 10, 100 and 1000M, half or full duplex.
Slightly worse, we allow those whether or not the PHY is even capable
of supporting them!
patchwork-bot+netdevbpf@kernel.org July 1, 2024, 10:20 a.m. UTC | #4
Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri, 28 Jun 2024 22:41:16 +0900 you wrote:
> Call phylink_ethtool_ksettings_get() for get_link_ksettings method and
> ethtool_op_get_link() for get_link method.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  drivers/net/ethernet/tehuti/tn40.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> [...]

Here is the summary with links:
  - [net-next] net: tn40xx: add initial ethtool_ops support
    https://git.kernel.org/netdev/net-next/c/7433d034ac3c

You are awesome, thank you!
FUJITA Tomonori July 3, 2024, 12:57 p.m. UTC | #5
On Fri, 28 Jun 2024 16:45:01 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

>> > +static int tn40_ethtool_get_link_ksettings(struct net_device *ndev,
>> > +					   struct ethtool_link_ksettings *cmd)
>> > +{
>> > +	struct tn40_priv *priv = netdev_priv(ndev);
>> > +
>> > +	return phylink_ethtool_ksettings_get(priv->phylink, cmd);
>> > +}
>> 
>> Have you tried implementing tn40_ethtool_set_link_ksettings() in the
>> same way?
> 
> I did think about commenting on that, and the [sg]et_pauseparam
> methods as well, but when one realises that the driver only supports
> one speed and duplex (10G FD) but no pause it didn't seem to make
> sense.

I have not been able to find register configuration to make
1000Base-SX work with QT2025 PHY.

> Not having pause effectively rules out pause-frame rate adaption
> by the PHY, so the PHY probably only supports 10G link speeds,
> and if I remember correctly, 10GBASE-T requires autoneg.

I suppose that the HW supports pause because a register named
regPAUSE_QUANT is set up during initialization. But the original
driver doesn't support [sg]et_pauseparam or give the details.

> The autonegotiation specification was improved in the 1998 release of
> IEEE 802.3. This was followed by the release of the IEEE 802.3ab Gigabit
> Ethernet standard in 1999 which specified mandatory autonegotiation for
> 1000BASE-T. Autonegotiation is also mandatory for 1000BASE-TX and
> 10GBASE-T implementations.
> 
> which is loose language - "mandatory autonegotiation" does that refer
> to support for auto-negotiation or require auto-negotiation to be
> always enabled?
> 
> We're already seeing some PHYs from some manufacturers that seem to be
> following the "require auto-negotiation to be always enabled".
> 
> So why have I gone down what seems to be an unrelated rabbit hole?
> 
> If tn40 is connected to a 10GBASE-T PHY, implementing the
> set_link_ksettings() method would give the user control over whether AN
> is used on the media side.
>
> If 802.3 requires AN to be supported but is not necessarily enabled,
> then there is use in exposing the set_link_ksettings() method.
> 
> If 802.3 requires AN to be supproted and always enabled, then
> implementing set_link_ksettings() in this case would not provide any
> value.

I've read your comment on stmmac patch:

https://lore.kernel.org/netdev/ZoQX1bqtJI2Zd9qH@shell.armlinux.org.uk/

When implementing BASE-T PHY support for tn40 driver, I'll check the
situation again.

Thanks a lot!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/tehuti/tn40.c b/drivers/net/ethernet/tehuti/tn40.c
index 11db9fde11fe..565b72537efa 100644
--- a/drivers/net/ethernet/tehuti/tn40.c
+++ b/drivers/net/ethernet/tehuti/tn40.c
@@ -1571,6 +1571,19 @@  static const struct net_device_ops tn40_netdev_ops = {
 	.ndo_vlan_rx_kill_vid = tn40_vlan_rx_kill_vid,
 };
 
+static int tn40_ethtool_get_link_ksettings(struct net_device *ndev,
+					   struct ethtool_link_ksettings *cmd)
+{
+	struct tn40_priv *priv = netdev_priv(ndev);
+
+	return phylink_ethtool_ksettings_get(priv->phylink, cmd);
+}
+
+static const struct ethtool_ops tn40_ethtool_ops = {
+	.get_link		= ethtool_op_get_link,
+	.get_link_ksettings	= tn40_ethtool_get_link_ksettings,
+};
+
 static int tn40_priv_init(struct tn40_priv *priv)
 {
 	int ret;
@@ -1599,6 +1612,7 @@  static struct net_device *tn40_netdev_alloc(struct pci_dev *pdev)
 	if (!ndev)
 		return NULL;
 	ndev->netdev_ops = &tn40_netdev_ops;
+	ndev->ethtool_ops = &tn40_ethtool_ops;
 	ndev->tx_queue_len = TN40_NDEV_TXQ_LEN;
 	ndev->mem_start = pci_resource_start(pdev, 0);
 	ndev->mem_end = pci_resource_end(pdev, 0);