| 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 |
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
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!
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!
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!
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 --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);
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