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

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: <20200729220748.GW1605@shell.armlinux.org.uk>
Date:   Wed, 29 Jul 2020 23:07:48 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Richard Cochran <richardcochran@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...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 RFC net-next] net: phy: add Marvell PHY PTP support

On Wed, Jul 29, 2020 at 02:28:32PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Jul 29, 2020 at 06:19:32AM -0700, Richard Cochran wrote:
> > On Wed, Jul 29, 2020 at 11:58:07AM +0100, Russell King - ARM Linux admin wrote:
> > > How do we deal with this situation - from what I can see from the
> > > ethtool API, we have to make a choice about which to use.  How do we
> > > make that choice?
> > 
> > Unfortunately the stack does not implement simultaneous MAC + PHY time
> > stamping.  If your board has both, then you make the choice to use the
> > PHY by selecting NETWORK_PHY_TIMESTAMPING at kernel compile time.
> 
> Which is more or less what I said in my email.  However, the important
> question about how to select between the two, which is really what I'm
> after, has not been addressed.
> 
> > (Also some MAC drivers do not defer to the PHY properly.  Sometimes
> > you can work around that by de-selecting the MAC's PTP function in the
> > Kconfig if possible, but otherwise you need to patch the MAC driver.)
> 
> ... which really doesn't work if you have a board where only some
> network interfaces have a PHY with PTP support, but all have PTP
> support in the MAC.
> 
> If all MACs or the majority of MACs use a common PTP clock, it seems
> to me that you would want to use the MACs rather than the PHY,
> especially if the PHY doesn't offer as good a quality PTP clock as
> is available from the MAC.
> 
> Randomly patching the kernel is out of the question, for arm based
> systems we want one kernel that works correctly across as many
> platforms as possible, and Kconfig choices to set platform specific
> details are basically unacceptable, let alone patching the kernel to
> make those decisions.

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!