RE: [RFC v5 net-next 0/5] Add RTNL interface for SyncE

-----Original Message-----
From: Ido Schimmel <redacted>
Sent: Wednesday, October 27, 2021 5:05 PM
To: Machnikowski, Maciej <redacted>
Subject: Re: [RFC v5 net-next 0/5] Add RTNL interface for SyncE

On Wed, Oct 27, 2021 at 01:21:58PM +0000, Machnikowski, Maciej wrote:

quoted

quoted

-----Original Message-----
From: Ido Schimmel <redacted>
Sent: Wednesday, October 27, 2021 8:50 AM
To: Machnikowski, Maciej <redacted>
Cc: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org;
richardcochran@gmail.com; abyagowi@fb.com; Nguyen, Anthony L
[off-list ref]; davem@davemloft.net;

kuba@kernel.org;

quoted

quoted

linux-kselftest@vger.kernel.org; mkubecek@suse.cz; saeed@kernel.org;
michael.chan@broadcom.com
Subject: Re: [RFC v5 net-next 0/5] Add RTNL interface for SyncE

On Tue, Oct 26, 2021 at 07:31:41PM +0200, Maciej Machnikowski wrote:

quoted

Synchronous Ethernet networks use a physical layer clock to syntonize
the frequency across different network elements.

Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
Equipment Clock (EEC) and have the ability to recover synchronization
from the synchronization inputs - either traffic interfaces or external
frequency sources.
The EEC can synchronize its frequency (syntonize) to any of those

sources.

quoted

quoted

quoted

It is also able to select synchronization source through priority tables
and synchronization status messaging. It also provides neccessary
filtering and holdover capabilities

This patch series introduces basic interface for reading the Ethernet
Equipment Clock (EEC) state on a SyncE capable device. This state gives
information about the source of the syntonization signal (ether my

port,

quoted

quoted

quoted

or any external one) and the state of EEC. This interface is required\
to implement Synchronization Status Messaging on upper layers.

v2:
- removed whitespace changes
- fix issues reported by test robot
v3:
- Changed naming from SyncE to EEC
- Clarify cover letter and commit message for patch 1
v4:
- Removed sync_source and pin_idx info
- Changed one structure to attributes
- Added EEC_SRC_PORT flag to indicate that the EEC is synchronized
  to the recovered clock of a port that returns the state
v5:
- add EEC source as an optiona attribute
- implement support for recovered clocks
- align states returned by EEC to ITU-T G.781

Hi,

Thanks for continuing to work on this.

I was under the impression (might be wrong) that the consensus last time
was to add a new ethtool message to query the mapping between the

port

quoted

quoted

and the EEC clock (similar to TSINFO_GET) and then use a new generic
netlink family to perform operations on the clock itself.

Hi!

I believe we finally agreed to continue with this implementations (for a
simplified devices) and when the DPLL subsystem is ready, plug it into this
API as well using the discovery mechanism. As there may be some

simplified

quoted

solutions that would not use the controllable DPLL and only provide the
status (i.e. using physical signals)

By "simplified solutions" you are referring to simple NEs that only
synchronize their frequency according to what they extract from the
physical layer as opposed to an external source such as in the PRC case?

quoted

quoted

At least in the case of RTM_GETEECSTATE and a multi-port adapter, you
would actually query the same state via each netdev, but without
realizing it's the same clock.

True, yet for a given port we need info whether we are locked or not,
so the interdependency wouldn't break anything.

But if two ports are using the same EEC, then it's not possible for them
to report a different EEC state, right? The only difference is that one
port can be the source and the other isn't, but this is also an
attribute of the EEC.

Basically, I'm saying that it seems that we report attributes of a
single object (the EEC) via multiple objects using it (the netdevs)
without making the relation clear to user space.

I'm not strictly against it, but rather wondering why.

Agreed. The reason to start with this approach is that it will be applicable
to some  devices in the future and we can easily fix that mapping later by 
adding EEC/DPLL ID to the attributes.

quoted

quoted

I think another reason to move to ethtool was that this stuff is
completely specific to Ethernet and not applicable to all logical
netdevs.

That was an open in previous discussion. Wanted to first show the
full API to discuss where it fits. I believe all other networks (like SONET)
may also benefit from having it in the netdev, but am open for discussion.

OK, didn't think about SONET. There is include/uapi/linux/sonet.h with a
few SONET specific ioctls and a couple of drivers implementing them. The
whole thing looks quite dead...

ethtool still seems like a better option for something that has
"Ethernet" in its name :)

For now we add it for SyncE, which indeed has the Ethernet in its name,
but I think the concept of recovering clock from the data stream is
common and applicable to other transport layers. I believe the same
API can be used in infiniband, fiber channel, xPON if they implement
PHYs supporting this capability. Hence if it's not a strong NO for putting
it in netdevs I'd rather keep it there to avoid challenges we found with
timing cards where we had to stretch the concept of Ethernet device
to use the APIs defined in the ethtool :)
Also the ITU-T recommendations G8261 and G.8262 are more generic
than just SyncE and they are applicable to any packet network.

Regards
Maciek