netdev - Re: [PATCH net-next] net: tn40xx: add initial ethtool

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 28 Jun 2024 16:45:01 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com
Subject: Re: [PATCH net-next] net: tn40xx: add initial ethtool_ops support

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@...il.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!