Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
@ 2021-01-10 11:13 Russell King
  2021-01-10 16:35 ` Andrew Lunn
  2021-01-14 12:55 ` Richard Cochran
  0 siblings, 2 replies; 12+ messages in thread
From: Russell King @ 2021-01-10 11:13 UTC (permalink / raw)
  To: Richard Cochran, Andrew Lunn, Heiner Kallweit
  Cc: David S. Miller, Jakub Kicinski, netdev

Check whether the MAC driver has implemented the get_ts_info()
method first, and call it if present.  If this method returns
-EOPNOTSUPP, defer to the phylib or default implementation.

This allows network drivers such as mvpp2 to use their more accurate
timestamping implementation than using a less accurate implementation
in the PHY. Network drivers can opt to defer to phylib by returning
-EOPNOTSUPP.

This change will be needed if the Marvell PHY drivers add support for
PTP.

Note: this may cause a change for any drivers that use phylib and
provide get_ts_info(). It is not obvious if any such cases exist.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 net/ethtool/common.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 24036e3055a1..9ec93e24f239 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -385,14 +385,18 @@ int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
 {
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	struct phy_device *phydev = dev->phydev;
+	int ret;
 
 	memset(info, 0, sizeof(*info));
 	info->cmd = ETHTOOL_GET_TS_INFO;
 
+	if (ops->get_ts_info) {
+		ret = ops->get_ts_info(dev, info);
+		if (ret != -EOPNOTSUPP)
+			return ret;
+	}
 	if (phy_has_tsinfo(phydev))
 		return phy_ts_info(phydev, info);
-	if (ops->get_ts_info)
-		return ops->get_ts_info(dev, info);
 
 	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
 				SOF_TIMESTAMPING_SOFTWARE;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
  2021-01-10 11:13 [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info Russell King
@ 2021-01-10 16:35 ` Andrew Lunn
  2021-01-14  3:05   ` Jakub Kicinski
  2021-01-14 17:09   ` Russell King - ARM Linux admin
  2021-01-14 12:55 ` Richard Cochran
  1 sibling, 2 replies; 12+ messages in thread
From: Andrew Lunn @ 2021-01-10 16:35 UTC (permalink / raw)
  To: Russell King
  Cc: Richard Cochran, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, netdev

On Sun, Jan 10, 2021 at 11:13:44AM +0000, Russell King wrote:
> Check whether the MAC driver has implemented the get_ts_info()
> method first, and call it if present.  If this method returns
> -EOPNOTSUPP, defer to the phylib or default implementation.
> 
> This allows network drivers such as mvpp2 to use their more accurate
> timestamping implementation than using a less accurate implementation
> in the PHY. Network drivers can opt to defer to phylib by returning
> -EOPNOTSUPP.
> 
> This change will be needed if the Marvell PHY drivers add support for
> PTP.
> 
> Note: this may cause a change for any drivers that use phylib and
> provide get_ts_info(). It is not obvious if any such cases exist.

Hi Russell

We can detect that condition through? Call both, then do a WARN() if
we are changing the order? Maybe we should do that for a couple of
cycles?

For netlink ethtool, we can also provide an additional attribute. A
MAC, or PHY indicator we can do in the core. A string for the name of
the driver would need a bigger change.

	Andrew

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
  2021-01-10 16:35 ` Andrew Lunn
@ 2021-01-14  3:05   ` Jakub Kicinski
  2021-01-14 17:09   ` Russell King - ARM Linux admin
  1 sibling, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2021-01-14  3:05 UTC (permalink / raw)
  To: Andrew Lunn, Russell King
  Cc: Richard Cochran, Heiner Kallweit, David S. Miller, netdev

On Sun, 10 Jan 2021 17:35:25 +0100 Andrew Lunn wrote:
> On Sun, Jan 10, 2021 at 11:13:44AM +0000, Russell King wrote:
> > Check whether the MAC driver has implemented the get_ts_info()
> > method first, and call it if present.  If this method returns
> > -EOPNOTSUPP, defer to the phylib or default implementation.
> > 
> > This allows network drivers such as mvpp2 to use their more accurate
> > timestamping implementation than using a less accurate implementation
> > in the PHY. Network drivers can opt to defer to phylib by returning
> > -EOPNOTSUPP.
> > 
> > This change will be needed if the Marvell PHY drivers add support for
> > PTP.
> > 
> > Note: this may cause a change for any drivers that use phylib and
> > provide get_ts_info(). It is not obvious if any such cases exist.  
> 
> Hi Russell
> 
> We can detect that condition through? Call both, then do a WARN() if
> we are changing the order? Maybe we should do that for a couple of
> cycles?
> 
> For netlink ethtool, we can also provide an additional attribute. A
> MAC, or PHY indicator we can do in the core. A string for the name of
> the driver would need a bigger change.

I'm not seeing a response to this suggestion, did vger eat it?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
  2021-01-10 11:13 [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info Russell King
  2021-01-10 16:35 ` Andrew Lunn
@ 2021-01-14 12:55 ` Richard Cochran
  2021-01-14 13:22   ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 12+ messages in thread
From: Richard Cochran @ 2021-01-14 12:55 UTC (permalink / raw)
  To: Russell King
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski, netdev

On Sun, Jan 10, 2021 at 11:13:44AM +0000, Russell King wrote:
> This allows network drivers such as mvpp2 to use their more accurate
> timestamping implementation than using a less accurate implementation
> in the PHY. Network drivers can opt to defer to phylib by returning
> -EOPNOTSUPP.

My expectation is that PHY time stamping is more accurate than MAC
time stamping.
 
> This change will be needed if the Marvell PHY drivers add support for
> PTP.

Huh?  The mvpp2 appears to be a MAC.  If this device has integrated
PHYs then I don't see the issue.  If your board has the nvpp2 device
with the dp83640 PHYTER, then don't you want to actually use the
PHYTER?

From my observation of the product offerings, I have yet to see a new
PHY (besides the dp83640 PHYTER) that implement time stamping.  The
PHYTER is 100 megabit only, and my understanding that it is too
difficult or even impossible to provide time stamps from within a
gigabit+ PHY.  So you needn't fear new time stamping PHYs to spoil
your setup!

> Note: this may cause a change for any drivers that use phylib and
> provide get_ts_info(). It is not obvious if any such cases exist.

Up until now, the code always favored PHY devices and devices external
to the MAC that snoop on the MII bus.  The assumption is that anyone
who builds a board with such specialty devices really wants to use
them.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
  2021-01-14 12:55 ` Richard Cochran
@ 2021-01-14 13:22   ` Russell King - ARM Linux admin
  2021-01-14 13:32     ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 12+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-14 13:22 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski, netdev

On Thu, Jan 14, 2021 at 04:55:06AM -0800, Richard Cochran wrote:
> On Sun, Jan 10, 2021 at 11:13:44AM +0000, Russell King wrote:
> > This allows network drivers such as mvpp2 to use their more accurate
> > timestamping implementation than using a less accurate implementation
> > in the PHY. Network drivers can opt to defer to phylib by returning
> > -EOPNOTSUPP.
> 
> My expectation is that PHY time stamping is more accurate than MAC
> time stamping.

PHY time stamping may be a "more accurate" location to get timestamps,
in terms of the hardware, but when you consider the entire setup, that
is not necessarily the case.

> > This change will be needed if the Marvell PHY drivers add support for
> > PTP.
> 
> Huh? The mvpp2 appears to be a MAC.  If this device has integrated
> PHYs then I don't see the issue.  If your board has the nvpp2 device
> with the dp83640 PHYTER, then don't you want to actually use the
> PHYTER?

You seem to be adding more information way beyond what I'm saying.

No, there aren't integrated PHYs. There's an external PHY - a Marvell
88e151x which has what I would call rudimentary stamping abilities,
whereas the mvpp2 has advanced stamping abilities.

You implemented the Marvell timestamping in DSA, so you know what the
Marvell offering there looks like and what it is capable of. That same
hardware appears in some Marvell PHYs.

The mvpp2 hardware (which has support already merged after you acked
the TAI part, and failed to provide comments on the mvpp2 part - so
davem gave up waiting) is capable of:
- stamping every received packet irrespective of its type
- stamping any transmitted packet or only those we wish to stamp
- inserting a timestamp into the packet (aka one-step, although that
  isn't implemented that yet)
- correcting the hardware time counter tick rate

There is considerable noise in reading the hardware timestamp counter
from the PHYs - caused by latency over the MDIO bus, which makes the
achievable accuracy lower. That noise is very much reduced when reading
the hardware timestamp counter from mvpp2 - where we can implement
mvpp22_tai_gettimex64(). Therefore, the achieved accuracy from mvpp2 is
higher than from a PHY.

We had already discussed this patch last year, and you agreed with it
then. What has changed?

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
  2021-01-14 13:22   ` Russell King - ARM Linux admin
@ 2021-01-14 13:32     ` Russell King - ARM Linux admin
  2021-01-14 17:27       ` Richard Cochran
  0 siblings, 1 reply; 12+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-14 13:32 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski, netdev

On Thu, Jan 14, 2021 at 01:22:17PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Jan 14, 2021 at 04:55:06AM -0800, Richard Cochran wrote:
> > On Sun, Jan 10, 2021 at 11:13:44AM +0000, Russell King wrote:
> > > This allows network drivers such as mvpp2 to use their more accurate
> > > timestamping implementation than using a less accurate implementation
> > > in the PHY. Network drivers can opt to defer to phylib by returning
> > > -EOPNOTSUPP.
> > 
> > My expectation is that PHY time stamping is more accurate than MAC
> > time stamping.
> 
> PHY time stamping may be a "more accurate" location to get timestamps,
> in terms of the hardware, but when you consider the entire setup, that
> is not necessarily the case.
> 
> > > This change will be needed if the Marvell PHY drivers add support for
> > > PTP.
> > 
> > Huh? The mvpp2 appears to be a MAC.  If this device has integrated
> > PHYs then I don't see the issue.  If your board has the nvpp2 device
> > with the dp83640 PHYTER, then don't you want to actually use the
> > PHYTER?
> 
> You seem to be adding more information way beyond what I'm saying.
> 
> No, there aren't integrated PHYs. There's an external PHY - a Marvell
> 88e151x which has what I would call rudimentary stamping abilities,
> whereas the mvpp2 has advanced stamping abilities.
> 
> You implemented the Marvell timestamping in DSA, so you know what the
> Marvell offering there looks like and what it is capable of. That same
> hardware appears in some Marvell PHYs.
> 
> The mvpp2 hardware (which has support already merged after you acked
> the TAI part, and failed to provide comments on the mvpp2 part - so
> davem gave up waiting) is capable of:
> - stamping every received packet irrespective of its type
> - stamping any transmitted packet or only those we wish to stamp
> - inserting a timestamp into the packet (aka one-step, although that
>   isn't implemented that yet)
> - correcting the hardware time counter tick rate
> 
> There is considerable noise in reading the hardware timestamp counter
> from the PHYs - caused by latency over the MDIO bus, which makes the
> achievable accuracy lower. That noise is very much reduced when reading
> the hardware timestamp counter from mvpp2 - where we can implement
> mvpp22_tai_gettimex64(). Therefore, the achieved accuracy from mvpp2 is
> higher than from a PHY.
> 
> We had already discussed this patch last year, and you agreed with it
> then. What has changed?

See the discussion in this sub-thread:

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

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
  2021-01-10 16:35 ` Andrew Lunn
  2021-01-14  3:05   ` Jakub Kicinski
@ 2021-01-14 17:09   ` Russell King - ARM Linux admin
  2021-01-14 17:27     ` Russell King - ARM Linux admin
  1 sibling, 1 reply; 12+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-14 17:09 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Richard Cochran, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, netdev

On Sun, Jan 10, 2021 at 05:35:25PM +0100, Andrew Lunn wrote:
> On Sun, Jan 10, 2021 at 11:13:44AM +0000, Russell King wrote:
> > Check whether the MAC driver has implemented the get_ts_info()
> > method first, and call it if present.  If this method returns
> > -EOPNOTSUPP, defer to the phylib or default implementation.
> > 
> > This allows network drivers such as mvpp2 to use their more accurate
> > timestamping implementation than using a less accurate implementation
> > in the PHY. Network drivers can opt to defer to phylib by returning
> > -EOPNOTSUPP.
> > 
> > This change will be needed if the Marvell PHY drivers add support for
> > PTP.
> > 
> > Note: this may cause a change for any drivers that use phylib and
> > provide get_ts_info(). It is not obvious if any such cases exist.
> 
> Hi Russell
> 
> We can detect that condition through? Call both, then do a WARN() if
> we are changing the order? Maybe we should do that for a couple of
> cycles?

I guess we could do something, but IMHO this really does not justify
using heavy hammers like WARN(). It's pointless producing a backtrace.
If we want a large noisy multi-line message that stands out, we should
just do that.

I think we can detect with something like:

	if (ops->get_ts_info && phy_has_tsinfo(phydev)) {
		netdev_warn(dev, "Both the PHY and the MAC support PTP. Which you end up with may change.\n");
	}

That said, this is _actually_ a fix.

As the code stands today:

__ethtool_get_ts_info() checks phy_has_tsinfo() and uses phy_ts_info()
in preference to ops->get_ts_info(), giving the PHY code first dibs on
intercepting this call.

Meanwhile, the ioctl() code gives the network driver first dibs on
intercepting the SIOCSHWTSTAMP and SIOCGHWTSTAMP ioctls.

This means that if you have both a PHY supporting timestamping, and a
MAC driver, you end up with the ethtool get_ts_info() call giving a
response from the PHY implementation but SIOCSHWTSTAMP and
SIOCGHWTSTAMP being intercepted by the MAC implementation.

This is exactly what will happen today to mvpp2 if we merge my patches
adding PTP support to the Marvell 88e151x PHYs without this patch.

So, my patch merely brings consistency to this.

> For netlink ethtool, we can also provide an additional attribute. A
> MAC, or PHY indicator we can do in the core. A string for the name of
> the driver would need a bigger change.

Unfortunately, PTP is not solely controlled through ethtool - it's
also controlled via ioctl() where it's not so easy to direct the
calls to either the MAC or PHY.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
  2021-01-14 13:32     ` Russell King - ARM Linux admin
@ 2021-01-14 17:27       ` Richard Cochran
  2021-01-14 17:31         ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 12+ messages in thread
From: Richard Cochran @ 2021-01-14 17:27 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski, netdev

On Thu, Jan 14, 2021 at 01:32:35PM +0000, Russell King - ARM Linux admin wrote:
> > We had already discussed this patch last year, and you agreed with it
> > then. What has changed?
> 
> See the discussion in this sub-thread:
> 
> https://lore.kernel.org/netdev/20200729105807.GZ1551@shell.armlinux.org.uk/

Thanks for the reminder.  We ended up with having to review the MAC
drivers that support phydev.

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

There is at least the FEC that supports phydev.  I have a board that
combines the FEC with the dp83640 PHYTER, and your patch would break
this setup.  (In the case of this HW combination, the PHYTER is
superior in every way.)

Another combination that I have seen twice is the TI am335x with its
cpsw MAC and the PHYTER.  Unfortunately I don't have one of these
boards, but people made them because the cpsw MAC supports time
stamping in a way that is inadequate.

I *think* the cpsw/phyter combination would work with your patch, but
only if the users disable CONFIG_TI_CPTS at compile time.

Thanks,
Richard



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
  2021-01-14 17:09   ` Russell King - ARM Linux admin
@ 2021-01-14 17:27     ` Russell King - ARM Linux admin
  0 siblings, 0 replies; 12+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-14 17:27 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Richard Cochran, Heiner Kallweit, David S. Miller,
	Jakub Kicinski, netdev

On Thu, Jan 14, 2021 at 05:09:42PM +0000, Russell King - ARM Linux admin wrote:
> On Sun, Jan 10, 2021 at 05:35:25PM +0100, Andrew Lunn wrote:
> > On Sun, Jan 10, 2021 at 11:13:44AM +0000, Russell King wrote:
> > > Check whether the MAC driver has implemented the get_ts_info()
> > > method first, and call it if present.  If this method returns
> > > -EOPNOTSUPP, defer to the phylib or default implementation.
> > > 
> > > This allows network drivers such as mvpp2 to use their more accurate
> > > timestamping implementation than using a less accurate implementation
> > > in the PHY. Network drivers can opt to defer to phylib by returning
> > > -EOPNOTSUPP.
> > > 
> > > This change will be needed if the Marvell PHY drivers add support for
> > > PTP.
> > > 
> > > Note: this may cause a change for any drivers that use phylib and
> > > provide get_ts_info(). It is not obvious if any such cases exist.
> > 
> > Hi Russell
> > 
> > We can detect that condition through? Call both, then do a WARN() if
> > we are changing the order? Maybe we should do that for a couple of
> > cycles?
> 
> I guess we could do something, but IMHO this really does not justify
> using heavy hammers like WARN(). It's pointless producing a backtrace.
> If we want a large noisy multi-line message that stands out, we should
> just do that.
> 
> I think we can detect with something like:
> 
> 	if (ops->get_ts_info && phy_has_tsinfo(phydev)) {
> 		netdev_warn(dev, "Both the PHY and the MAC support PTP. Which you end up with may change.\n");
> 	}
> 
> That said, this is _actually_ a fix.
> 
> As the code stands today:
> 
> __ethtool_get_ts_info() checks phy_has_tsinfo() and uses phy_ts_info()
> in preference to ops->get_ts_info(), giving the PHY code first dibs on
> intercepting this call.
> 
> Meanwhile, the ioctl() code gives the network driver first dibs on
> intercepting the SIOCSHWTSTAMP and SIOCGHWTSTAMP ioctls.
> 
> This means that if you have both a PHY supporting timestamping, and a
> MAC driver, you end up with the ethtool get_ts_info() call giving a
> response from the PHY implementation but SIOCSHWTSTAMP and
> SIOCGHWTSTAMP being intercepted by the MAC implementation.
> 
> This is exactly what will happen today to mvpp2 if we merge my patches
> adding PTP support to the Marvell 88e151x PHYs without this patch.
> 
> So, my patch merely brings consistency to this.
> 
> > For netlink ethtool, we can also provide an additional attribute. A
> > MAC, or PHY indicator we can do in the core. A string for the name of
> > the driver would need a bigger change.
> 
> Unfortunately, PTP is not solely controlled through ethtool - it's
> also controlled via ioctl() where it's not so easy to direct the
> calls to either the MAC or PHY.

BTW, none of this is new informationm we're just re-covering the same
points that were already discussed back in July 2020:

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

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
  2021-01-14 17:27       ` Richard Cochran
@ 2021-01-14 17:31         ` Russell King - ARM Linux admin
  2021-01-14 22:38           ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 12+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-14 17:31 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski, netdev

On Thu, Jan 14, 2021 at 09:27:12AM -0800, Richard Cochran wrote:
> On Thu, Jan 14, 2021 at 01:32:35PM +0000, Russell King - ARM Linux admin wrote:
> > > We had already discussed this patch last year, and you agreed with it
> > > then. What has changed?
> > 
> > See the discussion in this sub-thread:
> > 
> > https://lore.kernel.org/netdev/20200729105807.GZ1551@shell.armlinux.org.uk/
> 
> Thanks for the reminder.  We ended up with having to review the MAC
> drivers that support phydev.
> 
>    https://lore.kernel.org/netdev/20200730194427.GE1551@shell.armlinux.org.uk/
> 
> There is at least the FEC that supports phydev.  I have a board that
> combines the FEC with the dp83640 PHYTER, and your patch would break
> this setup.  (In the case of this HW combination, the PHYTER is
> superior in every way.)
> 
> Another combination that I have seen twice is the TI am335x with its
> cpsw MAC and the PHYTER.  Unfortunately I don't have one of these
> boards, but people made them because the cpsw MAC supports time
> stamping in a way that is inadequate.
> 
> I *think* the cpsw/phyter combination would work with your patch, but
> only if the users disable CONFIG_TI_CPTS at compile time.

I think then the only solution is to move the decision how to handle
get_ts_info into each MAC driver and get rid of:

	if (phy_has_tsinfo(phydev))
	        return phy_ts_info(phydev, info);

in __ethtool_get_ts_info().

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
  2021-01-14 17:31         ` Russell King - ARM Linux admin
@ 2021-01-14 22:38           ` Russell King - ARM Linux admin
  2021-01-21  4:04             ` Richard Cochran
  0 siblings, 1 reply; 12+ messages in thread
From: Russell King - ARM Linux admin @ 2021-01-14 22:38 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski, netdev

On Thu, Jan 14, 2021 at 05:31:11PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Jan 14, 2021 at 09:27:12AM -0800, Richard Cochran wrote:
> > Thanks for the reminder.  We ended up with having to review the MAC
> > drivers that support phydev.
> > 
> >    https://lore.kernel.org/netdev/20200730194427.GE1551@shell.armlinux.org.uk/
> > 
> > There is at least the FEC that supports phydev.  I have a board that
> > combines the FEC with the dp83640 PHYTER, and your patch would break
> > this setup.  (In the case of this HW combination, the PHYTER is
> > superior in every way.)
> > 
> > Another combination that I have seen twice is the TI am335x with its
> > cpsw MAC and the PHYTER.  Unfortunately I don't have one of these
> > boards, but people made them because the cpsw MAC supports time
> > stamping in a way that is inadequate.
> > 
> > I *think* the cpsw/phyter combination would work with your patch, but
> > only if the users disable CONFIG_TI_CPTS at compile time.
> 
> I think then the only solution is to move the decision how to handle
> get_ts_info into each MAC driver and get rid of:
> 
> 	if (phy_has_tsinfo(phydev))
> 	        return phy_ts_info(phydev, info);
> 
> in __ethtool_get_ts_info().

Thinking about this more, that is an impossible task - there's no
obvious information around to suggest which ethernet drivers could
possibly be attached to a phylib PHY that supports PTP.

So, I think the only way to prevent a regression with the code as
it is today is that we _never_ support PTP on Marvell PHYs - because
doing so _will_ break the existing MVPP2 driver's implementation and
cause a regression.

Right now, there is no option: if a PHY supports PTP, then the only
option is to use the PHYs PTP. Which is utterly rediculous.

Unless you can see a way around it. Because I can't.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info
  2021-01-14 22:38           ` Russell King - ARM Linux admin
@ 2021-01-21  4:04             ` Richard Cochran
  0 siblings, 0 replies; 12+ messages in thread
From: Richard Cochran @ 2021-01-21  4:04 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, Jakub Kicinski, netdev

On Thu, Jan 14, 2021 at 10:38:00PM +0000, Russell King - ARM Linux admin wrote:

> So, I think the only way to prevent a regression with the code as
> it is today is that we _never_ support PTP on Marvell PHYs - because
> doing so _will_ break the existing MVPP2 driver's implementation and
> cause a regression.

The situation isn't as bad as it seems.

For one thing, mvpp2 incorrectly selects NETWORK_PHY_TIMESTAMPING.
It really shouldn't.  I'm submitting a fix soon.

As long as the new PHY driver (or at least the PTP bit) depends on
NETWORK_PHY_TIMESTAMPING, then that allows users who _really_ want
that to enable the option at compile time.  This option adds extra
checks into the networking hot path, and almost everyone should avoid
enabling it.

> Right now, there is no option: if a PHY supports PTP, then the only
> option is to use the PHYs PTP. Which is utterly rediculous.
> 
> Unless you can see a way around it. Because I can't.

I still think the original and best method is to hide the two (and
with your new driver, three) esoteric PHY time stamping drivers behind
a Kconfig option, and structure the code to favor PHY devices.

The idea to favor the MACs, from back in July, doesn't really change
the fundamental limitations that

- MAC and PHY time stamping cannot work simultaneously, and

- Users of PHY devices (representing a tiny minority) must enable the
  otherwise uninteresting NETWORK_PHY_TIMESTAMPING option at compile
  time.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-10 11:13 [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info Russell King
2021-01-10 16:35 ` Andrew Lunn
2021-01-14  3:05   ` Jakub Kicinski
2021-01-14 17:09   ` Russell King - ARM Linux admin
2021-01-14 17:27     ` Russell King - ARM Linux admin
2021-01-14 12:55 ` Richard Cochran
2021-01-14 13:22   ` Russell King - ARM Linux admin
2021-01-14 13:32     ` Russell King - ARM Linux admin
2021-01-14 17:27       ` Richard Cochran
2021-01-14 17:31         ` Russell King - ARM Linux admin
2021-01-14 22:38           ` Russell King - ARM Linux admin
2021-01-21  4:04             ` Richard Cochran

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git