Date:   Mon, 13 Jul 2020 02:15:46 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Sergey Organov <sorganov@...il.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Fugang Duan <fugang.duan@....com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH v2 net] net: fec: fix hardware time stamping by external
 devices

On Mon, Jul 13, 2020 at 01:32:09AM +0300, Sergey Organov wrote:
> >
> > If you run "ethtool -T eth0" on a FEC network interface, it will always
> > report its own PHC, and never the PHC of a PHY. So, you cannot claim
> > that you are fixing PHY timestamping, since PHY timestamping is not
> > advertised. That's not what a bug fix is, at least not around here, with
> > its associated backporting efforts.
>
> You can't actually try it as you don't have the hardware, right? As for
> me, rather than running exactly ethtool, I do corresponding ioctl() in
> my program, and the kernel does report features of my external PTP PHY,
> not of internal one of the FEC, without my patches!
>
> > The only way you could have claimed that this was fixing PHY
> > timestamping was if "ethtool -T eth0" was reporting a PHY PHC, however
> > timestamps were not coming from the PHY.
>
> That's /exactly/ the case! Moreover, my original work is on 4.9.146
> kernel, so ethtool works correctly at least since then. Here is quote from
> my original question that I already gave reference to:
>
> <quote>
> Almost everything works fine out of the box, except hardware
> timestamping. The problems are that I apparently get timestamps from fec
> built-in PTP instead of external PHY, and that
>
>   ioctl(fd, SIOCSHWTSTAMP, &ifr)
>
> ends up being executed by fec1 built-in PTP code instead of being
> forwarded to the external PHY, and that this happens despite the call to
>
>    info.cmd = ETHTOOL_GET_TS_INFO;
>    ioctl(fd, SIOCETHTOOL, &ifr);
>
> returning phc_index = 1 that corresponds to external PHY, and reports
> features of the external PHY, leading to major inconsistency as seen
> from user-space.
> </quote>
>
> You see? This is exactly the case where I could claim fixing PHY time
> stamping even according to your own expertise!
>
> > From the perspective of the mainline kernel, that can never happen.
>
> Yet in happened to me, and in some way because of the UAPI deficiencies
> I've mentioned, as ethtool has entirely separate code path, that happens
> to be correct for a long time already.
>

Yup, you are right:

int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info)
{
	const struct ethtool_ops *ops = dev->ethtool_ops;
	struct phy_device *phydev = dev->phydev;

	memset(info, 0, sizeof(*info));
	info->cmd = ETHTOOL_GET_TS_INFO;

	if (phy_has_tsinfo(phydev))
		return phy_ts_info(phydev, info);
	if (ops->get_ts_info)
		return ops->get_ts_info(dev, info);

	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
				SOF_TIMESTAMPING_SOFTWARE;
	info->phc_index = -1;

	return 0;
}

Very bad design choice indeed...
Given the fact that the PHY timestamping needs massaging from MAC driver
for plenty of other reasons, now of all things, ethtool just decided
it's not going to consult the MAC driver about the PHC it intends to
expose to user space, and just say "here's the PHY, deal with it". This
is a structural bug, I would say.

> > From your perspective as a developer, in your private work tree, where
> > _you_ added the necessary wiring for PHY timestamping, I fully
> > understand that this is exactly what happened _to_you_.
> > I am not saying that PHY timestamping doesn't need this issue fixed. It
> > does, and if it weren't for DSA, it would have simply been a "new
> > feature", and it would have been ok to have everything in the same
> > patch.
>
> Except that it's not a "new feature", but a bug-fix of an existing one,
> as I see it.
>

See above. It's clear that the intention of the PHY timestamping support
is for MAC drivers to opt-in, otherwise some mechanism would have been
devised such that not every single one of them would need to check for
phy_has_hwtstamp() in .ndo_do_ioctl(). That simply doesn't scale. Also,
it seems that automatically calling phy_ts_info from
__ethtool_get_ts_info is not coherent with that intention.

I need to think more about this. Anyway, if your aim is to "reduce
confusion" for others walking in your foot steps, I think this is much
worthier of your time: avoiding the inconsistent situation where the MAC
driver is obviously not ready for PHY timestamping, however not all
parts of the kernel are in agreement with that, and tell the user
something else.

Thanks,
-Vladimir

Powered by blists - more mailing lists