Message-ID: <20210114172729.GQ1605@shell.armlinux.org.uk>
Date:   Thu, 14 Jan 2021 17:27:29 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Richard Cochran <richardcochran@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Subject: 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:
> > 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!

Powered by blists - more mailing lists