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

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

[FUJITA Tomonori]

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);
+}
+
+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);

base-commit: 94833addfaba89d12e5dbd82e350a692c00648ab
-- 
2.34.1

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

[Andrew Lunn]

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

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

[FUJITA Tomonori]

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!

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

[Russell King (Oracle)]

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!

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

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

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[FUJITA Tomonori]

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!