Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support

From: Vladimir Oltean <olteanv@gmail.com>
To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Cc: Richard Cochran <richardcochran@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	netdev@vger.kernel.org
Subject: Re: [PATCH RFC net-next] net: phy: add Marvell PHY PTP support
Date: Thu, 30 Jul 2020 01:53:30 +0300	[thread overview]
Message-ID: <20200729225330.3nnlpfvnbi3akdcv@skbuf> (raw)
In-Reply-To: <20200729220748.GW1605@shell.armlinux.org.uk>

On Wed, Jul 29, 2020 at 11:07:48PM +0100, Russell King - ARM Linux admin wrote:
> 
> The way PHY PTP support is handled is _really_ not nice.
> 
> If we have a phylib driver that supports timestamping, and a MAC driver
> that also supports timestamping, then what we end up with is very
> messed up.
> 
> 1. The SIOCGHWTSTAMP/SIOCSHWTSTAMP calls are directed through
>    ndo_do_ioctl().  The MAC driver gets first call on whether to
>    intercept these or not.  See dev_ifsioc().
> 
> 2. The ethtool ETHTOOL_GET_TS_INFO call, however, is given to the
>    PHY in preference to the MAC driver - there is no way for the MAC
>    driver to gain preference.  See __ethtool_get_ts_info().
> 
> So, if we have this situation (and yes, I do), then the SIOC*HWTSTAMP
> calls get implemented by the MAC driver, and takes effect at the MAC
> driver, while the ethtool ETHTOOL_GET_TS_INFO call returns results
> from the PHY driver.
> 
> That means the MAC driver's timestamping will be controllable from
> userspace, but userspace sees the abilities of the PHY driver's
> timestamping, and potentially directed to the wrong PTP clock
> instance.
> 
> What I see elsewhere in ethtool is that the MAC has the ability to
> override the phylib provided functionality - for example,
> __ethtool_get_sset_count(), __ethtool_get_strings(), and
> ethtool_get_phy_stats().  Would it be possible to do the same in
>  __ethtool_get_ts_info(), so at least a MAC driver can then decide
> whether to propagate the ethtool request to phylib or not, just like
> it can do with the SIOC*HWTSTAMP ioctls?  Essentially, reversing the
> order of:
> 
>         if (phy_has_tsinfo(phydev))
>                 return phy_ts_info(phydev, info);
>         if (ops->get_ts_info)
>                 return ops->get_ts_info(dev, info);
> 
> ?
> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

When we were discussing this in the other thread:
https://www.spinics.net/lists/netdev/msg669699.html
what I proposed was to introduce a NETIF_F_PHY_HWTSTAMP net_device flag
that could get set when the driver is capable of doing the right thing
with PHY timestamping. Then ethtool could look at that flag.
Additionally, if we could make this flag configurable as a netdev
feature with ethtool, then you could turn it off and even if you have an
attached PTP PHY, you'll use the PHC of the MAC.
You're free to leave your thoughts in that thread.

Thanks,
-Vladimir