Re: [PATCH net-next] net: ethtool: allow MAC drivers to override ethtool get_ts_info

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:

quoted

On Sun, Jan 10, 2021 at 11:13:44AM +0000, Russell King wrote:

quoted

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.

quoted

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/ (local)

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