RE: [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status

Machnikowski, Maciej
2021-09-06T18:30:43Z

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Saturday, September 4, 2021 12:14 AM
> Subject: Re: [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE
> message to get SyncE status
>
> On Fri, 3 Sep 2021 17:14:35 +0200 Maciej Machnikowski wrote:
> > 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 state of EEC. This interface is required to
> > implement Synchronization Status Messaging on upper layers.
> >
> > Initial implementation returns SyncE EEC state and flags attributes.
> > The only flag currently implemented is the EEC_SRC_PORT. When it's set
> > the EEC is synchronized to the recovered clock recovered from the
> > current port.
> >
> > SyncE EEC state read needs to be implemented as a ndo_get_eec_state
> > function.
> >
> > Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com>
>
> Since we're talking SyncE-only now my intuition would be to put this
> op in ethtool. Was there a reason ethtool was not chosen? If not what
> do others think / if yes can the reason be included in the commit
> message?

Hmm. Main reason for netlink is that linuxptp already supports it,
and it was suggested by Richard.
Having an NDO would also make it easier to add a SyncE-related
files to the sysfs for easier operation (following the ideas from the ptp
pins subsystem).
But I'm open for suggestions.

>
>
> > +#define EEC_SRC_PORT (1 << 0) /* recovered clock from the
> port is
> > + * currently the source for the EEC
> > + */
>
> Why include it then? Just leave the value out and if the attr is not
> present user space should assume the source is port.

This bit has a different meaning. If it's set the port in question
is a frequency source for the multiport device, if it's cleared - some other
source is used as a source. This is needed to prevent setting invalid
configurations in the PHY (like setting the frequency source as a Master
in AN) or sending invalid messages. If the port is a frequency source
it must always send back QL-DNU messages to prevent synchronization
loops.

>
> or don't check the ifindex at all and let dev_get_by.. fail.
>
>
> Thanks for pushing this API forward!

Addressed all other comments - and thanks for giving a lot of helpful
suggestions!

Regards
Maciek