From: "Machnikowski, Maciej" <maciej.machnikowski@intel.com> To: Jakub Kicinski <kuba@kernel.org> Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "intel-wired-lan@lists.osuosl.org" <intel-wired-lan@lists.osuosl.org>, "richardcochran@gmail.com" <richardcochran@gmail.com>, "abyagowi@fb.com" <abyagowi@fb.com>, "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>, "davem@davemloft.net" <davem@davemloft.net>, "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>, "Andrew Lunn" <andrew@lunn.ch>, Michal Kubecek <mkubecek@suse.cz> Subject: RE: [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Date: Mon, 6 Sep 2021 18:30:40 +0000 [thread overview] Message-ID: <PH0PR11MB4951623918C9BA8769C10E50EAD29@PH0PR11MB4951.namprd11.prod.outlook.com> (raw) In-Reply-To: <20210903151425.0bea0ce7@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> > -----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
WARNING: multiple messages have this Message-ID (diff)
From: Machnikowski, Maciej <maciej.machnikowski@intel.com> To: intel-wired-lan@osuosl.org Subject: [Intel-wired-lan] [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Date: Mon, 6 Sep 2021 18:30:40 +0000 [thread overview] Message-ID: <PH0PR11MB4951623918C9BA8769C10E50EAD29@PH0PR11MB4951.namprd11.prod.outlook.com> (raw) In-Reply-To: <20210903151425.0bea0ce7@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> > -----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
next prev parent reply other threads:[~2021-09-06 18:30 UTC|newest] Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-03 15:14 [RFC v4 net-next 0/2] Add RTNL interface for SyncE Maciej Machnikowski 2021-09-03 15:14 ` [Intel-wired-lan] " Maciej Machnikowski 2021-09-03 15:14 ` [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Maciej Machnikowski 2021-09-03 15:14 ` [Intel-wired-lan] " Maciej Machnikowski 2021-09-03 16:18 ` Stephen Hemminger 2021-09-03 16:18 ` [Intel-wired-lan] " Stephen Hemminger 2021-09-03 22:20 ` Machnikowski, Maciej 2021-09-03 22:20 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-03 22:14 ` Jakub Kicinski 2021-09-03 22:14 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-06 18:30 ` Machnikowski, Maciej [this message] 2021-09-06 18:30 ` Machnikowski, Maciej 2021-09-06 18:39 ` Jakub Kicinski 2021-09-06 18:39 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-06 19:01 ` Machnikowski, Maciej 2021-09-06 19:01 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-07 1:01 ` Jakub Kicinski 2021-09-07 1:01 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-07 8:50 ` Machnikowski, Maciej 2021-09-07 8:50 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-07 14:55 ` Jakub Kicinski 2021-09-07 14:55 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-07 15:47 ` Machnikowski, Maciej 2021-09-07 15:47 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-07 19:47 ` Jakub Kicinski 2021-09-07 19:47 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-08 8:03 ` Machnikowski, Maciej 2021-09-08 8:03 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-08 16:21 ` Jakub Kicinski 2021-09-08 16:21 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-08 17:30 ` Machnikowski, Maciej 2021-09-08 17:30 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-08 19:34 ` Andrew Lunn 2021-09-08 19:34 ` [Intel-wired-lan] " Andrew Lunn 2021-09-08 20:27 ` Machnikowski, Maciej 2021-09-08 20:27 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-08 22:20 ` Jakub Kicinski 2021-09-08 22:20 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-08 22:59 ` Andrew Lunn 2021-09-08 22:59 ` [Intel-wired-lan] " Andrew Lunn 2021-09-09 2:09 ` Richard Cochran 2021-09-09 2:09 ` [Intel-wired-lan] " Richard Cochran 2021-09-09 8:18 ` Machnikowski, Maciej 2021-09-09 8:18 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-10 14:14 ` Richard Cochran 2021-09-10 14:14 ` [Intel-wired-lan] " Richard Cochran 2021-09-08 22:18 ` Jakub Kicinski 2021-09-08 22:18 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-08 23:14 ` Andrew Lunn 2021-09-08 23:14 ` [Intel-wired-lan] " Andrew Lunn 2021-09-08 23:58 ` Jakub Kicinski 2021-09-08 23:58 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-09 8:26 ` Machnikowski, Maciej 2021-09-09 8:26 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-09 9:24 ` Machnikowski, Maciej 2021-09-09 9:24 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-09 10:15 ` David Miller 2021-09-09 10:15 ` [Intel-wired-lan] " David Miller 2021-09-09 8:11 ` Machnikowski, Maciej 2021-09-09 8:11 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-13 8:50 ` Ido Schimmel 2021-09-13 8:50 ` [Intel-wired-lan] " Ido Schimmel 2021-09-21 13:36 ` Ido Schimmel 2021-09-21 13:36 ` [Intel-wired-lan] " Ido Schimmel 2021-09-21 13:15 ` Ido Schimmel 2021-09-21 13:15 ` [Intel-wired-lan] " Ido Schimmel 2021-09-21 13:37 ` Machnikowski, Maciej 2021-09-21 13:37 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-21 14:58 ` Ido Schimmel 2021-09-21 14:58 ` [Intel-wired-lan] " Ido Schimmel 2021-09-21 21:14 ` Jakub Kicinski 2021-09-21 21:14 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-22 6:22 ` Ido Schimmel 2021-09-22 6:22 ` [Intel-wired-lan] " Ido Schimmel 2021-09-03 15:14 ` [PATCH net-next 2/2] ice: add support for reading SyncE DPLL state Maciej Machnikowski 2021-09-03 15:14 ` [Intel-wired-lan] " Maciej Machnikowski 2021-09-03 22:06 ` Jakub Kicinski 2021-09-03 22:06 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-21 13:25 ` Ido Schimmel 2021-09-21 13:25 ` [Intel-wired-lan] " Ido Schimmel -- strict thread matches above, loose matches on Subject: below -- 2021-08-31 11:52 [PATCH net-next 0/2] Add RTNL interface for SyncE EEC state Maciej Machnikowski 2021-08-31 11:52 ` [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Maciej Machnikowski 2021-08-31 13:44 ` Jakub Kicinski
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=PH0PR11MB4951623918C9BA8769C10E50EAD29@PH0PR11MB4951.namprd11.prod.outlook.com \ --to=maciej.machnikowski@intel.com \ --cc=abyagowi@fb.com \ --cc=andrew@lunn.ch \ --cc=anthony.l.nguyen@intel.com \ --cc=davem@davemloft.net \ --cc=intel-wired-lan@lists.osuosl.org \ --cc=kuba@kernel.org \ --cc=linux-kselftest@vger.kernel.org \ --cc=mkubecek@suse.cz \ --cc=netdev@vger.kernel.org \ --cc=richardcochran@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.