Message-ID: <55f9df6241d052a91dfde950af04c70969ea28b2.camel@infradead.org>
Date: Tue, 01 Apr 2025 09:02:15 +0100
From: David Woodhouse <dwmw2@...radead.org>
To: Andrew Lunn <andrew@...n.ch>, David Arinzon <darinzon@...zon.com>
Cc: David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
 netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
 <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Richard Cochran
 <richardcochran@...il.com>, "Woodhouse, David" <dwmw@...zon.com>, 
 "Machulsky, Zorik" <zorik@...zon.com>, "Matushevsky, Alexander"
 <matua@...zon.com>, Saeed Bshara <saeedb@...zon.com>, "Wilson, Matt"
 <msw@...zon.com>, "Liguori, Anthony" <aliguori@...zon.com>, "Bshara, Nafea"
 <nafea@...zon.com>, "Schmeilin, Evgeny" <evgenys@...zon.com>, "Belgazal,
 Netanel" <netanel@...zon.com>, "Saidi, Ali" <alisaidi@...zon.com>,
 "Herrenschmidt, Benjamin" <benh@...zon.com>,  "Kiyanovski, Arthur"
 <akiyano@...zon.com>, "Dagan, Noam" <ndagan@...zon.com>, "Bernstein, Amit"
 <amitbern@...zon.com>, "Agroskin, Shay" <shayagr@...zon.com>, "Ostrovsky,
 Evgeny" <evostrov@...zon.com>, "Tabachnik, Ofir" <ofirt@...zon.com>,
 "Machnikowski, Maciek" <maciek@...hnikowski.net>, Rahul Rameshbabu
 <rrameshbabu@...dia.com>, Gal Pressman <gal@...dia.com>, Vadim Fedorenko
 <vadim.fedorenko@...ux.dev>
Subject: Re: [PATCH v8 net-next 5/5] net: ena: Add PHC documentation

On Tue, 2025-03-04 at 22:10 +0100, Andrew Lunn wrote:
> > +The feature is turned off by default, in order to turn the feature
> > on,
> > +please use the following:
> > +
> > +- sysfs (during runtime):
> > +
> > +.. code-block:: shell
> > +
> > +  echo 1 >
> > /sys/bus/pci/devices/<domain:bus:slot.function>/phc_enable
> > +
> > +All available PTP clock sources can be tracked here:
> > +
> > +.. code-block:: shell
> > +
> > +  ls /sys/class/ptp
> > +
> > +PHC support and capabilities can be verified using ethtool:
> > +
> > +.. code-block:: shell
> > +
> > +  ethtool -T <interface>
> > +
> > +**PHC timestamp**
> > +
> > +To retrieve PHC timestamp, use `ptp-userspace-api`_, usage example
> > using `testptp`_:
> > +
> > +.. code-block:: shell
> > +
> > +  testptp -d /dev/ptp$(ethtool -T <interface> | awk '/PTP Hardware
> > Clock:/ {print $NF}') -k 1
> 
> Why is not opening /dev/ptpX sufficient to enable the PHC?

There's currently no hook from ptp_open() and ptp_release() into the
implementation to do anything like that. It could be added, I suppose,
but it isn't a great experience because the act of opening it for the
first time would then take down the netdev.

None of the alternative bikeshed options that David is being bombarded
with here make sense to me, like auxdev or implicit opening, because
it's tied to the netdev.

In a future revision of the device itself, perhaps we can enable the
PHC completely independently of the network device. And then of course
we can just do it on ptp_open(), and we'll add the required hooks in
struct ptp_clock_info. But for now, that doesn't make sense.

I've been a maintainer. I remember looking at submissions and thinking
"there must be a better way to do this", then attempting five different
options and concluding that the original submission is the one I
dislike least after all. This seems like one of those times, with a
bunch of suggestions which ultimately give a worse user experience.
And a contributor caught in the cross-fire with no coherent guidance
about how to proceed.

I think the sysfs control is the best option here.

On the device side, I am internally applying the cluebat because this
whole thing was *predictable*. The PHC feature *should* have been
orthogonal, in the device, to the actual networking. So it can be
enabled or disabled at will. Eventually I hope a later version of the
device should fix that, and we can have a nice clean user interface —
the /dev/ptpX node can always exist, but it does nothing at all if it
isn't being used. In the interim, a sysfs knob to explicitly turn it on
seems like a reasonable approach to me.

I am ambivalent about *also* using auxdev, so that when the netdev is
told to enable the PHC feature, the auxdev 'appears' and a separate PTP
driver binds to it. It's kind of cute, but probably overkill, and not
what any other NIC driver does, is it? CONFIG_PTP_1588_CLOCK_OPTIONAL
exists to handle the case of NICs with optional PTP support.
(Actually it turns out I'm less ambivalent by the end of that paragraph
than I was at the start... :)

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5069 bytes)

Powered by blists - more mailing lists