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

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20210114170942.GW1551@shell.armlinux.org.uk>
Date:   Thu, 14 Jan 2021 17:09:42 +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 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!