linuxptp-devel

Re: [Linuxptp-devel] [RFC PATCH v2 4/9] Update the PdelayReq/Res flows for CMLDS Link Ports

[Richard C. <ric...@gm...>]

On Wed, Nov 15, 2023 at 08:58:58PM -0800, Richard Cochran wrote:
> The text of 1588 strongly suggests that the CMLDS service be stand
> alone daemon.  However, we can provide the same functionality without
> the extra complexity, by simply letting ptp4l serve the measured peer
> delay to any local client.

And, if you _really_ want to have a stand alone CMLDS server with its
own clockIdentity etc, then that is easy:

ptp4l \
	--clientOnly=1 \
	--clockIdentity=100000.2000.300000 \
	--domainNumber=0 \
	--free_running=1 \
	--delay_mechanism=P2P \
	-i eth0

Thanks,
Richard

Re: [Linuxptp-devel] [RFC PATCH v2 6/9] Add plumbing for interacting with the CMLDS

[Richard C. <ric...@gm...>]

On Mon, May 15, 2023 at 06:26:09PM -0400, Kishen Maloor wrote:
> This change furnishes the plumbing for interacting with CMLDS
> Link Ports (i.e. ports with "run_cmlds=1").
> 
> It adds internal functions for PTP Ports which employ the
> COMMON_P2P delay mechanism to issue requests to a CMLDS Link Port at
> MID_CMLDS_INFO_NP and handle the response.

This is too complicated.

Let's keep it simple.

I'd like to have this:

1. provide the new TLV via the PUSH mechanism.

2. let clients using DM_COMMON_P2P subscribe to the new TLV.

3. let the client update their peer delay values based on an incoming TLV.

Thanks,
Richard

Re: [Linuxptp-devel] [RFC PATCH v2 6/9] Add plumbing for interacting with the CMLDS

[Richard C. <ric...@gm...>]

On Wed, Nov 15, 2023 at 09:05:45PM -0800, Richard Cochran wrote:

> I'd like to have this:
> 
> 1. provide the new TLV via the PUSH mechanism.
> 
> 2. let clients using DM_COMMON_P2P subscribe to the new TLV.
> 
> 3. let the client update their peer delay values based on an incoming TLV.

For clients to do this, maybe the cleanest way is to add a new UDS
client FD into the 'struct fdarray' (see fd.h)

Thanks,
Richard

Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

[Richard C. <ric...@gm...>]

On Mon, May 15, 2023 at 06:26:04PM -0400, Kishen Maloor wrote:

> @@ -473,6 +476,14 @@ struct msg_interface_rate_tlv {
>         UInteger16    numberOfBitsAfterTimestamp;
>  } PACKED;
>  
> +struct cmlds_info_np {
> +	Integer8     serviceMeasurementValid;
> +	TimeInterval meanLinkDelay;
> +	Integer32    scaledNeighborRateRatio;

I think you need one more field, like "source_port_index"

If a client subscribes to TLVs from multiple ports, then it needs a
way to tell which is which.

> +	Integer64    egress_ts;
> +	Integer64    rx_ts;
> +} PACKED;

Thanks,
Richard

Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

[Richard C. <ric...@gm...>]

On Wed, Nov 15, 2023 at 09:45:52PM -0800, Richard Cochran wrote:
> On Mon, May 15, 2023 at 06:26:04PM -0400, Kishen Maloor wrote:
> 
> > @@ -473,6 +476,14 @@ struct msg_interface_rate_tlv {
> >         UInteger16    numberOfBitsAfterTimestamp;
> >  } PACKED;
> >  
> > +struct cmlds_info_np {
> > +	Integer8     serviceMeasurementValid;
> > +	TimeInterval meanLinkDelay;
> > +	Integer32    scaledNeighborRateRatio;
> 
> I think you need one more field, like "source_port_index"
> 
> If a client subscribes to TLVs from multiple ports, then it needs a
> way to tell which is which.

Or maybe the subscription will cause the CMLDS server to forward all
p2p delay measurements from all p2p ports.

Still the TLVs needs to include the port index.

Thanks,
Richard

Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

[Andrew Z. <and...@in...>]

On Thu, 16 Nov 2023 at 05:27, Richard Cochran <ric...@gm...> wrote:
> On Mon, May 15, 2023 at 06:26:04PM -0400, Kishen Maloor wrote:
> > @@ -1129,6 +1130,27 @@ static int port_management_fill_response(struct port *target,
> >               memcpy(pwr, &target->pwr, sizeof(*pwr));
> >               datalen = sizeof(*pwr);
> >               break;
> > +     case MID_CMLDS_INFO_NP:
> > +             cmlds = (struct cmlds_info_np *)tlv->data;
> > +             /* IEEE1588-2019 16.6.3.2 h) 1) && nrate.ratio_valid because
> > +              * we have no extra field to convey that separately.
> > +              */
> > +             cmlds->serviceMeasurementValid =
> > +                     target->peer_portid_valid && !target->pdr_missing &&
> > +                     !target->multiple_pdr_detected &&
> > +                     target->nrate.ratio_valid;
> > +             cmlds->meanLinkDelay = target->peerMeanPathDelay;
> > +             cmlds->scaledNeighborRateRatio =
> > +                     (Integer32) (target->nrate.ratio * POW2_41 - POW2_41);
>
> > +             /* 16.6.3.2: "Upon receipt of a request for information, the
> > +              * Common Mean Link Delay Service may in addition return the
> > +              * raw measurement data gathered by the service for use in
> > +              * estimating the <meanLinkDelay> and <neighborRateRatio>."
> > +              */
> > +             cmlds->egress_ts = tmv_to_nanoseconds(target->peer_delay_t1);
> > +             cmlds->rx_ts = tmv_to_nanoseconds(target->peer_delay_t2);
>
> Please drop these two fields.  They don't provide any benefit.  If the
> clients don't trust the CMLDS values, then they are free to measure
> the p2p delay themselves!

The two timestamps are passed to clock_peer_delay() by the receiving
port and stored in c->tsproc.  Then they're accessed by
get_raw_delay() which is used in the filter logic.  I'm not sure how
much value that has, we can possibly pass 0s to clock_peer_delay().

Regarding "source_port_index", I assume that would contain the ifindex
of the interface?  With virtual clocks I believe the PHC indices may
differ between the PTP ports on one physical port ("link port").

Best regards

Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

[Richard C. <ric...@gm...>]

On Thu, Nov 16, 2023 at 11:11:50PM +0100, Andrew Zaborowski wrote:

> The two timestamps are passed to clock_peer_delay() by the receiving
> port and stored in c->tsproc.  Then they're accessed by
> get_raw_delay() which is used in the filter logic.  I'm not sure how
> much value that has, we can possibly pass 0s to clock_peer_delay().

If the client uses CMLDS, then it doesn't need tsproc logic at all.
It simply take the delay value from the CMLDS server.
 
> Regarding "source_port_index", I assume that would contain the ifindex
> of the interface?  With virtual clocks I believe the PHC indices may
> differ between the PTP ports on one physical port ("link port").

No, I mean the PTP port number.  These are taken from the order of the
interfaces on the command line and in the configuration file.

Thanks,
Richard

Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

[Andrew Z. <and...@in...>]

On Thu, 16 Nov 2023 at 23:46, Richard Cochran <ric...@gm...> wrote:
> On Thu, Nov 16, 2023 at 11:11:50PM +0100, Andrew Zaborowski wrote:
> > The two timestamps are passed to clock_peer_delay() by the receiving
> > port and stored in c->tsproc.  Then they're accessed by
> > get_raw_delay() which is used in the filter logic.  I'm not sure how
> > much value that has, we can possibly pass 0s to clock_peer_delay().
>
> If the client uses CMLDS, then it doesn't need tsproc logic at all.
> It simply take the delay value from the CMLDS server.

Make sense, the CMLDS would already be filtering the timestamps once.

>
> > Regarding "source_port_index", I assume that would contain the ifindex
> > of the interface?  With virtual clocks I believe the PHC indices may
> > differ between the PTP ports on one physical port ("link port").
>
> No, I mean the PTP port number.  These are taken from the order of the
> interfaces on the command line and in the configuration file.

Won't this be the same as the UDS message's sourcePortIdentity.portNumber?

Do you want to require the user to enforce that the port numbering is
the same between the ptp4l processes?  In our use case spec compliance
is the priority, including the unique clockIdentity for CMLDS
requirement, so we'd definitely need to be running a separate ptp4l
process for CMLDS in this schema.  I'm asking because the port
numbering thing is unobvious and the user effort to configure a
compliant setup with ptp4l is already very high.

So one solution would be to let the user configure a custom portNumber
under port settings.  My colleague even had a local change to allow
passing -i <ifname>,<portNumber>.  This would also remove the
(possibly unobvious) requirement to run all ports sharing the
clockIdentity as one ptp4l process.

Another option would be to pass the ifindex as I thought you were suggesting.

Best regards

Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

[Richard C. <ric...@gm...>]

On Fri, Nov 17, 2023 at 01:41:19AM +0100, Andrew Zaborowski wrote:
> On Thu, 16 Nov 2023 at 23:46, Richard Cochran <ric...@gm...> wrote:
> > No, I mean the PTP port number.  These are taken from the order of the
> > interfaces on the command line and in the configuration file.
> 
> Won't this be the same as the UDS message's sourcePortIdentity.portNumber?

Ah, yes, you are right.  Even simpler!

> Do you want to require the user to enforce that the port numbering is
> the same between the ptp4l processes?

No.

> In our use case spec compliance
> is the priority, including the unique clockIdentity for CMLDS
> requirement, so we'd definitely need to be running a separate ptp4l
> process for CMLDS in this schema.

The clockIdentity is not exposed, so what do you mean by "compliance"?

> I'm asking because the port
> numbering thing is unobvious and the user effort to configure a
> compliant setup with ptp4l is already very high.

That is just par for the course.  I'm not the one making up tons of
crazy new options.

The strength of linuxptp is modularity and fine grained, configurable
options.  This means that a) ptp4l works even with equipment that
fails to follow the standards and/or profiles, and b) ptp4l can be
really hard to configure.

> So one solution would be to let the user configure a custom portNumber
> under port settings.

Port numbers start with 1 and increase in the order of the configured
interfaces.  If it really is required for CMLDS ports to have
*different* numbers than the normal ports, we can add a "port index
offset" option that would start the numbering at some given value.

Thanks,
Richard

Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

[Andrew Z. <and...@in...>]

On Fri, 17 Nov 2023 at 05:50, Richard Cochran <ric...@gm...> wrote:
> On Fri, Nov 17, 2023 at 01:41:19AM +0100, Andrew Zaborowski wrote:
> > Do you want to require the user to enforce that the port numbering is
> > the same between the ptp4l processes?
>
> No.

(I meant: do you want to require that the user takes care to maintain
the order of interfaces on the command line / in the config file,
which I think you do -- Ok)

>
> > In our use case spec compliance
> > is the priority, including the unique clockIdentity for CMLDS
> > requirement, so we'd definitely need to be running a separate ptp4l
> > process for CMLDS in this schema.
>
> The clockIdentity is not exposed, so what do you mean by "compliance"?

It is exposed on the wire in the Pdelay messages.  Compliance tests
look at this.  They also simulate a few hypothetical scenarios like a
domain 0 PTP port trying to communicate with a CMLDS link port since
1588 talks about this.

Best regards

Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

[Richard C. <ric...@gm...>]

On Fri, Nov 17, 2023 at 01:51:18PM +0100, Andrew Zaborowski wrote:

> It is exposed on the wire in the Pdelay messages.  Compliance tests
> look at this.  They also simulate a few hypothetical scenarios like a
> domain 0 PTP port trying to communicate with a CMLDS link port since
> 1588 talks about this.

How does this requirement improve synchronization?

What benefit does it bring to users of the PTP?

Thanks,
Richard

Re: [Linuxptp-devel] [RFC PATCH v2 1/9] Add new TLV for CommonMeanLinkDelayInformation

[Richard C. <ric...@gm...>]

On Fri, Nov 17, 2023 at 08:27:43PM -0800, Richard Cochran wrote:
> How does this requirement improve synchronization?
> 
> What benefit does it bring to users of the PTP?

rhetorical questions  :^(

[Linuxptp-devel] [PATCH v1 0/5] Common Mean Link Delay -- proof of concept

[Richard C. <ric...@gm...>]

This is how CMLD should be done, IMHO.

Currently, because of the quirks of the UDS module, a program can be a
PMC server or PMC client, but not both.  Patches 1 and 2 address this
issue, allowing ptp4l program to be both a UDS server and client.

Still TODO:

- replace hard coded 1-hour one shot subscription with renewal logic
- man page update for new cmlds options

Please review and test to shake out the bugs.

Thanks,
Richard


Kishen Maloor (1):
  Implement the COMMON_P2P delay mechanism.

Richard Cochran (4):
  interface: Add an optional remote address for use by the UDS transport.
  pmc/uds: Configure the remote server address using the interface API.
  Introduce the Common Mean Link Delay Information TLV.
  Add a push notification for the CMLDS TLV.

 clock.c        |   5 +-
 config.c       |   6 +-
 dm.h           |   3 +
 fd.h           |   1 +
 interface.c    |  12 +++-
 interface.h    |  10 ++-
 makefile       |   4 +-
 notification.h |   1 +
 pmc.c          |  21 ++++--
 pmc_agent.c    |   3 +-
 pmc_common.c   |  23 ++++---
 pmc_common.h   |   6 +-
 port.c         | 179 +++++++++++++++++++++++++++++++++++++++++++++++--
 port.h         |   2 +
 port_private.h |   3 +
 tlv.c          |  14 ++++
 tlv.h          |   6 ++
 tz2alt.c       |   3 +-
 uds.c          |   6 +-
 19 files changed, 275 insertions(+), 33 deletions(-)

-- 
2.39.2

[Linuxptp-devel] [PATCH v1 2/5] pmc/uds: Configure the remote server address using the interface API.

[Richard C. <ric...@gm...>]

Signed-off-by: Richard Cochran <ric...@gm...>
---
 pmc.c        | 6 ++++--
 pmc_agent.c  | 3 ++-
 pmc_common.c | 8 ++++----
 pmc_common.h | 6 +++---
 tz2alt.c     | 3 ++-
 uds.c        | 6 ++++--
 6 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/pmc.c b/pmc.c
index 9faf790..d18fea0 100644
--- a/pmc.c
+++ b/pmc.c
@@ -823,8 +823,10 @@ int main(int argc, char *argv[])
 	print_set_syslog(1);
 	print_set_verbose(1);
 
-	pmc = pmc_create(cfg, transport_type, iface_name, boundary_hops,
-			 domain_number, transport_specific, zero_datalen);
+	pmc = pmc_create(cfg, transport_type, iface_name,
+			 config_get_string(cfg, NULL, "uds_address"),
+			 boundary_hops, domain_number, transport_specific,
+			 zero_datalen);
 	if (!pmc) {
 		fprintf(stderr, "failed to create pmc\n");
 		config_destroy(cfg);
diff --git a/pmc_agent.c b/pmc_agent.c
index 494c174..fec6dea 100644
--- a/pmc_agent.c
+++ b/pmc_agent.c
@@ -232,7 +232,8 @@ int run_pmc_wait_sync(struct pmc_agent *node, int timeout)
 int init_pmc_node(struct config *cfg, struct pmc_agent *node, const char *uds,
 		  pmc_node_recv_subscribed_t *recv_subscribed, void *context)
 {
-	node->pmc = pmc_create(cfg, TRANS_UDS, uds, 0,
+	node->pmc = pmc_create(cfg, TRANS_UDS, uds,
+			       config_get_string(cfg, NULL, "uds_address"), 0,
 			       config_get_int(cfg, NULL, "domainNumber"),
 			       config_get_int(cfg, NULL, "transportSpecific") << 4, 1);
 	if (!node->pmc) {
diff --git a/pmc_common.c b/pmc_common.c
index 5092c09..fca16c6 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -488,9 +488,9 @@ struct pmc {
 };
 
 struct pmc *pmc_create(struct config *cfg, enum transport_type transport_type,
-		       const char *iface_name, UInteger8 boundary_hops,
-		       UInteger8 domain_number, UInteger8 transport_specific,
-		       int zero_datalen)
+		       const char *iface_name, const char *remote_address,
+		       UInteger8 boundary_hops, UInteger8 domain_number,
+		       UInteger8 transport_specific, int zero_datalen)
 {
 	struct pmc *pmc;
 	UInteger32 proc_id;
@@ -524,7 +524,7 @@ struct pmc *pmc_create(struct config *cfg, enum transport_type transport_type,
 		goto failed;
 	}
 
-	pmc->iface = interface_create(iface_name, NULL);
+	pmc->iface = interface_create(iface_name, remote_address);
 	if (!pmc->iface) {
 		pr_err("failed to create interface");
 		goto failed;
diff --git a/pmc_common.h b/pmc_common.h
index 6fb2fae..355b2c0 100644
--- a/pmc_common.h
+++ b/pmc_common.h
@@ -29,9 +29,9 @@
 struct pmc;
 
 struct pmc *pmc_create(struct config *cfg, enum transport_type transport_type,
-		       const char *iface_name, UInteger8 boundary_hops,
-		       UInteger8 domain_number, UInteger8 transport_specific,
-		       int zero_datalen);
+		       const char *iface_name, const char *remote_address,
+		       UInteger8 boundary_hops, UInteger8 domain_number,
+		       UInteger8 transport_specific, int zero_datalen);
 
 void pmc_destroy(struct pmc *pmc);
 
diff --git a/tz2alt.c b/tz2alt.c
index feb77a5..65b5835 100644
--- a/tz2alt.c
+++ b/tz2alt.c
@@ -181,7 +181,8 @@ static int update_ptp_serivce(struct tzinfo *tz, struct tzinfo *next)
 	struct pmc *pmc;
 	int err;
 
-	pmc = pmc_create(cfg, TRANS_UDS, uds_local, 0,
+	pmc = pmc_create(cfg, TRANS_UDS, uds_local,
+			 config_get_string(cfg, NULL, "uds_address"), 0,
 			 config_get_int(cfg, NULL, "domainNumber"),
 			 config_get_int(cfg, NULL, "transportSpecific") << 4, 1);
 	if (!pmc) {
diff --git a/uds.c b/uds.c
index 6d39dc8..fafb100 100644
--- a/uds.c
+++ b/uds.c
@@ -54,7 +54,7 @@ static int uds_open(struct transport *t, struct interface *iface, struct fdarray
 		    enum timestamp_type tt)
 {
 	char *uds_ro_path = config_get_string(t->cfg, NULL, "uds_ro_address");
-	char *uds_path = config_get_string(t->cfg, NULL, "uds_address");
+	const char *uds_path = interface_remote(iface);
 	struct uds *uds = container_of(t, struct uds, t);
 	const char *name = interface_name(iface);
 	const char* file_mode_cfg;
@@ -89,7 +89,9 @@ static int uds_open(struct transport *t, struct interface *iface, struct fdarray
 	/* For client use, pre load the server path. */
 	memset(&sa, 0, sizeof(sa));
 	sa.sun_family = AF_LOCAL;
-	strncpy(sa.sun_path, uds_path, sizeof(sa.sun_path) - 1);
+	if (uds_path) {
+		strncpy(sa.sun_path, uds_path, sizeof(sa.sun_path) - 1);
+	}
 	uds->address.sun = sa;
 	uds->address.len = sizeof(sa);
 
-- 
2.39.2

[Linuxptp-devel] [PATCH v1 1/5] interface: Add an optional remote address for use by the UDS transport.

[Richard C. <ric...@gm...>]

Signed-off-by: Richard Cochran <ric...@gm...>
---
 clock.c      |  4 ++--
 config.c     |  2 +-
 interface.c  | 12 ++++++++++--
 interface.h  | 10 +++++++++-
 pmc_common.c |  2 +-
 5 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/clock.c b/clock.c
index b66dda5..6f7722c 100644
--- a/clock.c
+++ b/clock.c
@@ -1242,7 +1242,7 @@ struct clock *clock_create(enum clock_type type, struct config *config,
 	/* Configure the UDS. */
 
 	uds_ifname = config_get_string(config, NULL, "uds_address");
-	c->uds_rw_if = interface_create(uds_ifname);
+	c->uds_rw_if = interface_create(uds_ifname, NULL);
 	if (config_set_section_int(config, interface_name(c->uds_rw_if),
 				   "announceReceiptTimeout", 0)) {
 		return NULL;
@@ -1261,7 +1261,7 @@ struct clock *clock_create(enum clock_type type, struct config *config,
 	}
 
 	uds_ifname = config_get_string(config, NULL, "uds_ro_address");
-	c->uds_ro_if = interface_create(uds_ifname);
+	c->uds_ro_if = interface_create(uds_ifname, NULL);
 	if (config_set_section_int(config, interface_name(c->uds_ro_if),
 				   "announceReceiptTimeout", 0)) {
 		return NULL;
diff --git a/config.c b/config.c
index ad675c8..fe65b76 100644
--- a/config.c
+++ b/config.c
@@ -896,7 +896,7 @@ struct interface *config_create_interface(const char *name, struct config *cfg)
 			return iface;
 	}
 
-	iface = interface_create(name);
+	iface = interface_create(name, NULL);
 	if (!iface) {
 		fprintf(stderr, "cannot allocate memory for a port\n");
 		return NULL;
diff --git a/interface.c b/interface.c
index 9a83c36..e088e07 100644
--- a/interface.c
+++ b/interface.c
@@ -12,12 +12,13 @@ struct interface {
 	STAILQ_ENTRY(interface) list;
 	char name[MAX_IFNAME_SIZE + 1];
 	char ts_label[MAX_IFNAME_SIZE + 1];
+	char remote[MAX_IFNAME_SIZE + 1];
 	struct sk_ts_info ts_info;
 	struct sk_if_info if_info;
 	int vclock;
 };
 
-struct interface *interface_create(const char *name)
+struct interface *interface_create(const char *name, const char *remote)
 {
 	struct interface *iface;
 
@@ -27,6 +28,9 @@ struct interface *interface_create(const char *name)
 	}
 	strncpy(iface->name, name, MAX_IFNAME_SIZE);
 	strncpy(iface->ts_label, name, MAX_IFNAME_SIZE);
+	if (remote) {
+		strncpy(iface->remote, remote, MAX_IFNAME_SIZE);
+	}
 	iface->vclock = -1;
 
 	return iface;
@@ -57,7 +61,6 @@ bool interface_ifinfo_valid(struct interface *iface)
        return iface->if_info.valid ? true : false;
 }
 
-
 const char *interface_name(struct interface *iface)
 {
 	return iface->name;
@@ -68,6 +71,11 @@ int interface_phc_index(struct interface *iface)
 	return iface->ts_info.phc_index;
 }
 
+const char *interface_remote(struct interface *iface)
+{
+	return iface->remote;
+}
+
 void interface_set_label(struct interface *iface, const char *label)
 {
 	strncpy(iface->ts_label, label, MAX_IFNAME_SIZE);
diff --git a/interface.h b/interface.h
index 0873bba..b56adc5 100644
--- a/interface.h
+++ b/interface.h
@@ -23,9 +23,10 @@ struct interface;
 /**
  * Creates an instance of an interface.
  * @param name  The device which indentifies this interface.
+ * @param remote  For UDS interfaces, the address of the remote server, possibly NULL.
  * @return      A pointer to an interface instance on success, NULL otherwise.
  */
-struct interface *interface_create(const char *name);
+struct interface *interface_create(const char *name, const char *remote);
 
 /**
  * Destroys an instance of an interface.
@@ -70,6 +71,13 @@ const char *interface_name(struct interface *iface);
  */
 int interface_phc_index(struct interface *iface);
 
+/**
+ * Obtains the remote address from a UDS interface.
+ * @param iface  The interface of interest.
+ * @return       The device name of the network interface.
+ */
+const char *interface_remote(struct interface *iface);
+
 /**
  * Set the time stamping label of a given interface.
  * @param iface  The interface of interest.
diff --git a/pmc_common.c b/pmc_common.c
index 62e34a6..5092c09 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -524,7 +524,7 @@ struct pmc *pmc_create(struct config *cfg, enum transport_type transport_type,
 		goto failed;
 	}
 
-	pmc->iface = interface_create(iface_name);
+	pmc->iface = interface_create(iface_name, NULL);
 	if (!pmc->iface) {
 		pr_err("failed to create interface");
 		goto failed;
-- 
2.39.2

[Linuxptp-devel] [PATCH v1 3/5] Introduce the Common Mean Link Delay Information TLV.

[Richard C. <ric...@gm...>]

Add a new TLV to convey link delay measurements by the Common Mean
Link Delay Service (CMLDS) (as specified in IEEE 1588/16.6.3) over the
management interface.

Co-authored-by: Andrew Zaborowski <and...@in...>
Signed-off-by: Kishen Maloor <kis...@in...>
Signed-off-by: Richard Cochran <ric...@gm...>
---
 clock.c      |  1 -
 pmc.c        |  9 +++++++++
 pmc_common.c |  1 +
 port.c       |  8 ++++++++
 port.h       |  2 ++
 tlv.c        | 14 ++++++++++++++
 tlv.h        |  6 ++++++
 7 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/clock.c b/clock.c
index 6f7722c..c999c83 100644
--- a/clock.c
+++ b/clock.c
@@ -47,7 +47,6 @@
 #include "util.h"
 
 #define N_CLOCK_PFD (N_POLLFD + 1) /* one extra per port, for the fault timer */
-#define POW2_41 ((double)(1ULL << 41))
 
 struct interface {
 	STAILQ_ENTRY(interface) list;
diff --git a/pmc.c b/pmc.c
index d18fea0..c29e3b7 100644
--- a/pmc.c
+++ b/pmc.c
@@ -169,6 +169,7 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
 	struct subscribe_events_np *sen;
 	struct port_properties_np *ppn;
 	struct port_hwclock_np *phn;
+	struct cmlds_info_np *cmlds;
 	struct timePropertiesDS *tp;
 	struct management_tlv *mgt;
 	struct time_status_np *tsn;
@@ -623,6 +624,14 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
 			pwr->networkTimeInaccuracy,
 			pwr->totalTimeInaccuracy);
 		break;
+	case MID_CMLDS_INFO_NP:
+		cmlds = (struct cmlds_info_np *) mgt->data;
+		fprintf(fp, "CMLDS_INFO_NP "
+			IFMT "meanLinkDelay           %" PRId64
+			IFMT "scaledNeighborRateRatio %" PRId32,
+			cmlds->meanLinkDelay >> 16,
+			cmlds->scaledNeighborRateRatio);
+		break;
 	case MID_LOG_ANNOUNCE_INTERVAL:
 		mtd = (struct management_tlv_datum *) mgt->data;
 		fprintf(fp, "LOG_ANNOUNCE_INTERVAL "
diff --git a/pmc_common.c b/pmc_common.c
index fca16c6..1d537f2 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -156,6 +156,7 @@ struct management_id idtab[] = {
 	{ "UNICAST_MASTER_TABLE_NP", MID_UNICAST_MASTER_TABLE_NP, do_get_action },
 	{ "PORT_HWCLOCK_NP", MID_PORT_HWCLOCK_NP, do_get_action },
 	{ "POWER_PROFILE_SETTINGS_NP", MID_POWER_PROFILE_SETTINGS_NP, do_set_action },
+	{ "CMLDS_INFO_NP", MID_CMLDS_INFO_NP, do_get_action },
 };
 
 static void do_get_action(struct pmc *pmc, int action, int index, char *str)
diff --git a/port.c b/port.c
index 136d036..aab9d3f 100644
--- a/port.c
+++ b/port.c
@@ -883,6 +883,7 @@ static int port_management_fill_response(struct port *target,
 	struct clock_description *desc;
 	struct port_properties_np *ppn;
 	struct port_hwclock_np *phn;
+	struct cmlds_info_np *cmlds;
 	struct management_tlv *tlv;
 	struct port_stats_np *psn;
 	struct foreign_clock *fc;
@@ -1125,6 +1126,13 @@ static int port_management_fill_response(struct port *target,
 		memcpy(pwr, &target->pwr, sizeof(*pwr));
 		datalen = sizeof(*pwr);
 		break;
+	case MID_CMLDS_INFO_NP:
+		cmlds = (struct cmlds_info_np *)tlv->data;
+		cmlds->meanLinkDelay = target->peerMeanPathDelay;
+		cmlds->scaledNeighborRateRatio =
+			(Integer32) (target->nrate.ratio * POW2_41 - POW2_41);
+		datalen = sizeof(*cmlds);
+		break;
 	default:
 		/* The caller should *not* respond to this message. */
 		tlv_extra_recycle(extra);
diff --git a/port.h b/port.h
index 57c8c2f..cc03859 100644
--- a/port.h
+++ b/port.h
@@ -26,6 +26,8 @@
 #include "notification.h"
 #include "transport.h"
 
+#define POW2_41 ((double)(1ULL << 41))
+
 /* forward declarations */
 struct interface;
 struct clock;
diff --git a/tlv.c b/tlv.c
index 9b82bd9..8fbeb1a 100644
--- a/tlv.c
+++ b/tlv.c
@@ -176,6 +176,7 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
 	struct port_properties_np *ppn;
 	struct port_hwclock_np *phn;
 	struct timePropertiesDS *tp;
+	struct cmlds_info_np *cmlds;
 	struct time_status_np *tsn;
 	struct port_stats_np *psn;
 	int extra_len = 0, i, len;
@@ -481,6 +482,13 @@ static int mgt_post_recv(struct management_tlv *m, uint16_t data_len,
 		NTOHL(pwr->networkTimeInaccuracy);
 		NTOHL(pwr->totalTimeInaccuracy);
 		break;
+	case MID_CMLDS_INFO_NP:
+		if (data_len < sizeof(struct cmlds_info_np))
+			goto bad_length;
+		cmlds = (struct cmlds_info_np *)m->data;
+		net2host64_unaligned(&cmlds->meanLinkDelay);
+		NTOHL(cmlds->scaledNeighborRateRatio);
+		break;
 	case MID_SAVE_IN_NON_VOLATILE_STORAGE:
 	case MID_RESET_NON_VOLATILE_STORAGE:
 	case MID_INITIALIZE:
@@ -514,6 +522,7 @@ static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)
 	struct subscribe_events_np *sen;
 	struct port_properties_np *ppn;
 	struct port_hwclock_np *phn;
+	struct cmlds_info_np *cmlds;
 	struct timePropertiesDS *tp;
 	struct time_status_np *tsn;
 	struct port_stats_np *psn;
@@ -672,6 +681,11 @@ static void mgt_pre_send(struct management_tlv *m, struct tlv_extra *extra)
 		HTONL(pwr->networkTimeInaccuracy);
 		HTONL(pwr->totalTimeInaccuracy);
 		break;
+	case MID_CMLDS_INFO_NP:
+		cmlds = (struct cmlds_info_np *)m->data;
+		host2net64_unaligned(&cmlds->meanLinkDelay);
+		HTONL(cmlds->scaledNeighborRateRatio);
+		break;
 	}
 }
 
diff --git a/tlv.h b/tlv.h
index 8b51ffd..6287333 100644
--- a/tlv.h
+++ b/tlv.h
@@ -129,6 +129,7 @@ enum management_action {
 #define MID_UNICAST_MASTER_TABLE_NP			0xC008
 #define MID_PORT_HWCLOCK_NP				0xC009
 #define MID_POWER_PROFILE_SETTINGS_NP			0xC00A
+#define MID_CMLDS_INFO_NP				0xC00B
 
 /* Management error ID values */
 #define MID_RESPONSE_TOO_BIG				0x0001
@@ -323,6 +324,11 @@ typedef struct Integer96 {
 	uint16_t fractional_nanoseconds;
 } PACKED ScaledNs;
 
+struct cmlds_info_np {
+	TimeInterval meanLinkDelay;
+	Integer32    scaledNeighborRateRatio;
+} PACKED;
+
 struct follow_up_info_tlv {
 	Enumeration16 type;
 	UInteger16    length;
-- 
2.39.2

[Linuxptp-devel] [PATCH v1 4/5] Add a push notification for the CMLDS TLV.

[Richard C. <ric...@gm...>]

Signed-off-by: Richard Cochran <ric...@gm...>
---
 notification.h |  1 +
 pmc.c          |  6 ++++--
 pmc_common.c   | 14 ++++++++++----
 port.c         |  5 +++++
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/notification.h b/notification.h
index c1a6395..7a8f641 100644
--- a/notification.h
+++ b/notification.h
@@ -45,6 +45,7 @@ enum notification {
 	NOTIFY_PORT_STATE,
 	NOTIFY_TIME_SYNC,
 	NOTIFY_PARENT_DATA_SET,
+	NOTIFY_CMLDS,
 };
 
 #endif
diff --git a/pmc.c b/pmc.c
index c29e3b7..e733cf0 100644
--- a/pmc.c
+++ b/pmc.c
@@ -453,11 +453,13 @@ static void pmc_show(struct ptp_message *msg, FILE *fp)
 			IFMT "duration               %hu"
 			IFMT "NOTIFY_PORT_STATE      %s"
 			IFMT "NOTIFY_TIME_SYNC       %s"
-			IFMT "NOTIFY_PARENT_DATA_SET %s",
+			IFMT "NOTIFY_PARENT_DATA_SET %s"
+			IFMT "NOTIFY_CMLDS           %s",
 			sen->duration,
 			event_bitmask_get(sen->bitmask, NOTIFY_PORT_STATE) ? "on" : "off",
 			event_bitmask_get(sen->bitmask, NOTIFY_TIME_SYNC) ? "on" : "off",
-			event_bitmask_get(sen->bitmask, NOTIFY_PARENT_DATA_SET) ? "on" : "off");
+			event_bitmask_get(sen->bitmask, NOTIFY_PARENT_DATA_SET) ? "on" : "off",
+			event_bitmask_get(sen->bitmask, NOTIFY_CMLDS) ? "on" : "off");
 		break;
 	case MID_SYNCHRONIZATION_UNCERTAIN_NP:
 		mtd = (struct management_tlv_datum *) mgt->data;
diff --git a/pmc_common.c b/pmc_common.c
index 1d537f2..a549af5 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -180,6 +180,7 @@ static void do_set_action(struct pmc *pmc, int action, int index, char *str)
 	char onoff_port_state[4] = "off";
 	char onoff_time_status[4] = "off";
 	char onoff_parent_data_set[4] = "off";
+	char onoff_cmlds[4] = "off";
 	char display_name[11] = {0};
 	uint64_t jump;
 	uint8_t key;
@@ -306,13 +307,15 @@ static void do_set_action(struct pmc *pmc, int action, int index, char *str)
 			     "duration          %hu "
 			     "NOTIFY_PORT_STATE %3s "
 			     "NOTIFY_TIME_SYNC  %3s "
-			     "NOTIFY_PARENT_DATA_SET %3s ",
+			     "NOTIFY_PARENT_DATA_SET %3s "
+			     "NOTIFY_CMLDS %3s ",
 			     &sen.duration,
 			     onoff_port_state,
 			     onoff_time_status,
-			     onoff_parent_data_set);
-		if (cnt != 4) {
-			fprintf(stderr, "%s SET needs 4 values\n",
+			     onoff_parent_data_set,
+			     onoff_cmlds);
+		if (cnt != 5) {
+			fprintf(stderr, "%s SET needs 5 values\n",
 				idtab[index].name);
 			break;
 		}
@@ -326,6 +329,9 @@ static void do_set_action(struct pmc *pmc, int action, int index, char *str)
 			event_bitmask_set(sen.bitmask, NOTIFY_PARENT_DATA_SET,
 					  TRUE);
 		}
+		if (!strcasecmp(onoff_cmlds, "on")) {
+			event_bitmask_set(sen.bitmask, NOTIFY_CMLDS, TRUE);
+		}
 		pmc_send_set_action(pmc, code, &sen, sizeof(sen));
 		break;
 	case MID_SYNCHRONIZATION_UNCERTAIN_NP:
diff --git a/port.c b/port.c
index aab9d3f..8afe1b2 100644
--- a/port.c
+++ b/port.c
@@ -2466,6 +2466,8 @@ calc:
 
 	msg_put(p->peer_delay_req);
 	p->peer_delay_req = NULL;
+
+	port_notify_event(p, NOTIFY_CMLDS);
 }
 
 int process_pdelay_resp(struct port *p, struct ptp_message *m)
@@ -3263,6 +3265,9 @@ void port_notify_event(struct port *p, enum notification event)
 	case NOTIFY_PORT_STATE:
 		id = MID_PORT_DATA_SET;
 		break;
+	case NOTIFY_CMLDS:
+		id = MID_CMLDS_INFO_NP;
+		break;
 	default:
 		return;
 	}
-- 
2.39.2

[Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.

[Richard C. <ric...@gm...>]

From: Kishen Maloor <kis...@in...>

Signed-off-by: Richard Cochran <ric...@gm...>
---
 config.c       |   4 ++
 dm.h           |   3 +
 fd.h           |   1 +
 makefile       |   4 +-
 port.c         | 166 +++++++++++++++++++++++++++++++++++++++++++++++--
 port_private.h |   3 +
 6 files changed, 174 insertions(+), 7 deletions(-)

diff --git a/config.c b/config.c
index fe65b76..05ffd3c 100644
--- a/config.c
+++ b/config.c
@@ -171,6 +171,7 @@ static struct config_enum delay_filter_enu[] = {
 
 static struct config_enum delay_mech_enu[] = {
 	{ "Auto", DM_AUTO },
+	{ "COMMON_P2P", DM_COMMON_P2P },
 	{ "E2E",  DM_E2E },
 	{ "P2P",  DM_P2P },
 	{ "NONE", DM_NO_MECHANISM },
@@ -249,6 +250,9 @@ struct config_item config_tab[] = {
 	GLOB_ITEM_INT("clock_class_threshold", CLOCK_CLASS_THRESHOLD_DEFAULT, 6, CLOCK_CLASS_THRESHOLD_DEFAULT),
 	GLOB_ITEM_ENU("clock_servo", CLOCK_SERVO_PI, clock_servo_enu),
 	GLOB_ITEM_ENU("clock_type", CLOCK_TYPE_ORDINARY, clock_type_enu),
+	PORT_ITEM_STR("cmlds_client_address", "/var/run/cmlds_cleint"),
+	PORT_ITEM_INT("cmlds_port", 1, 1, UINT16_MAX),
+	PORT_ITEM_STR("cmlds_server_address", "/var/run/cmlds_server"),
 	GLOB_ITEM_ENU("dataset_comparison", DS_CMP_IEEE1588, dataset_comp_enu),
 	PORT_ITEM_INT("delayAsymmetry", 0, INT_MIN, INT_MAX),
 	PORT_ITEM_ENU("delay_filter", FILTER_MOVING_MEDIAN, delay_filter_enu),
diff --git a/dm.h b/dm.h
index 47bd847..80d1ce5 100644
--- a/dm.h
+++ b/dm.h
@@ -34,6 +34,9 @@ enum delay_mechanism {
 	/** Peer delay mechanism. */
 	DM_P2P,
 
+	/** Peer delay as measured by CMLDS. */
+	DM_COMMON_P2P,
+
 	/** No Delay Mechanism. */
 	DM_NO_MECHANISM = 0xFE,
 };
diff --git a/fd.h b/fd.h
index 16420d7..9a072ab 100644
--- a/fd.h
+++ b/fd.h
@@ -39,6 +39,7 @@ enum {
 	FD_SYNC_TX_TIMER,
 	FD_UNICAST_REQ_TIMER,
 	FD_UNICAST_SRV_TIMER,
+	FD_CMLDS,
 	FD_RTNL,
 	N_POLLFD,
 };
diff --git a/makefile b/makefile
index 7fc5f6f..e9c1ccc 100644
--- a/makefile
+++ b/makefile
@@ -30,8 +30,8 @@ TS2PHC	= ts2phc.o lstab.o nmea.o serial.o sock.o ts2phc_generic_pps_source.o \
  ts2phc_nmea_pps_source.o ts2phc_phc_pps_source.o ts2phc_pps_sink.o ts2phc_pps_source.o
 OBJ	= bmc.o clock.o clockadj.o clockcheck.o config.o designated_fsm.o \
  e2e_tc.o fault.o $(FILTERS) fsm.o hash.o interface.o monitor.o msg.o phc.o \
- port.o port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o $(SERVOS) \
- sk.o stats.o tc.o $(TRANSP) telecom.o tlv.o tsproc.o unicast_client.o \
+ pmc_common.o port.o port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o \
+ $(SERVOS) sk.o stats.o tc.o $(TRANSP) telecom.o tlv.o tsproc.o unicast_client.o \
  unicast_fsm.o unicast_service.o util.o version.o
 
 OBJECTS	= $(OBJ) hwstamp_ctl.o nsm.o phc2sys.o phc_ctl.o pmc.o pmc_agent.o \
diff --git a/port.c b/port.c
index 8afe1b2..75b4dd8 100644
--- a/port.c
+++ b/port.c
@@ -963,7 +963,8 @@ static int port_management_fill_response(struct port *target,
 		ptp_text_copy(cd->userDescription, &desc->userDescription);
 		buf += sizeof(struct PTPText) + cd->userDescription->length;
 
-		if (target->delayMechanism == DM_P2P) {
+		if (target->delayMechanism == DM_P2P ||
+		    target->delayMechanism == DM_COMMON_P2P) {
 			memcpy(buf, profile_id_p2p, PROFILE_ID_LEN);
 		} else {
 			struct config *cfg = clock_config(target->clock);
@@ -1868,6 +1869,33 @@ static void port_clear_fda(struct port *p, int count)
 		p->fda.fd[i] = -1;
 }
 
+static int port_cmlds_initialize(struct port *p)
+{
+	struct config *cfg = clock_config(p->clock);
+	struct subscribe_events_np sen = {0};
+	const int zero_datalen = 1;
+	const UInteger8 hops = 0;
+
+	p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");
+	p->cmlds_pmc = pmc_create(cfg, TRANS_UDS,
+				  config_get_string(cfg, p->name, "cmlds_client_address"),
+				  config_get_string(cfg, p->name, "cmlds_server_address"),
+				  hops,
+				  config_get_int(cfg, NULL, "domainNumber"),
+				  config_get_int(cfg, p->name, "transportSpecific") << 4,
+				  zero_datalen);
+	if (!p->cmlds_pmc) {
+		return -1;
+	}
+
+	event_bitmask_set(sen.bitmask, NOTIFY_CMLDS, TRUE);
+	sen.duration = 3600;
+	p->fda.fd[FD_CMLDS] = pmc_get_transport_fd(p->cmlds_pmc);
+	pmc_send_set_action(p->cmlds_pmc, MID_SUBSCRIBE_EVENTS_NP, &sen, sizeof(sen));
+
+	return 0;
+}
+
 void port_disable(struct port *p)
 {
 	int i;
@@ -1885,6 +1913,12 @@ void port_disable(struct port *p)
 		close(p->fda.fd[FD_FIRST_TIMER + i]);
 	}
 
+	if (p->cmlds_pmc) {
+		pmc_destroy(p->cmlds_pmc);
+		p->fda.fd[FD_CMLDS] = -1;
+		p->cmlds_pmc = NULL;
+	}
+
 	/* Keep rtnl socket to get link status info. */
 	port_clear_fda(p, FD_RTNL);
 	clock_fda_changed(p->clock);
@@ -1932,7 +1966,8 @@ int port_initialize(struct port *p)
 		pr_err("inhibit_delay_req can only be set when asCapable == 'true'.");
 		return -1;
 	}
-	if (port_delay_mechanism(p) == DM_NO_MECHANISM) {
+	if (port_delay_mechanism(p) == DM_COMMON_P2P ||
+	    port_delay_mechanism(p) == DM_NO_MECHANISM) {
 		p->inhibit_delay_req = 1;
 	}
 
@@ -1960,6 +1995,10 @@ int port_initialize(struct port *p)
 		goto no_tmo;
 	}
 
+	if (port_delay_mechanism(p) == DM_COMMON_P2P && port_cmlds_initialize(p)) {
+		goto no_tmo;
+	}
+
 	/* No need to open rtnl socket on UDS port. */
 	if (!port_is_uds(p)) {
 		/*
@@ -2101,6 +2140,65 @@ int process_announce(struct port *p, struct ptp_message *m)
 	return result;
 }
 
+static int process_cmlds(struct port *p)
+{
+	struct cmlds_info_np *cmlds;
+	struct management_tlv *mgt;
+	struct ptp_message *msg;
+	struct TLV *tlv;
+	int err = 0;
+
+	msg = pmc_recv(p->cmlds_pmc);
+	if (!msg) {
+		pr_err("%s: pmc_recv failed", p->log_name);
+		return -1;
+	}
+	if (msg_type(msg) != MANAGEMENT) {
+		pr_err("%s: pmc_recv bad message", p->log_name);
+		err = -1;
+		goto out;
+	}
+	tlv = (struct TLV *) msg->management.suffix;
+	if (tlv->type != TLV_MANAGEMENT) {
+		pr_err("%s: pmc_recv bad message", p->log_name);
+		err = -1;
+		goto out;
+	}
+	mgt = (struct management_tlv *) msg->management.suffix;
+	if (mgt->length == 2 && mgt->id != MID_NULL_MANAGEMENT) {
+		pr_err("%s: pmc_recv bad length", p->log_name);
+		goto out;
+	}
+
+	switch (mgt->id) {
+	case MID_CMLDS_INFO_NP:
+		if (msg->header.sourcePortIdentity.portNumber != p->cmlds_port) {
+			break;
+		}
+		cmlds = (struct cmlds_info_np *) mgt->data;
+		p->peer_delay = nanoseconds_to_tmv(cmlds->meanLinkDelay >> 16);
+		p->peerMeanPathDelay = tmv_to_TimeInterval(p->peer_delay);
+
+		if (p->state == PS_UNCALIBRATED || p->state == PS_SLAVE) {
+			const struct timestamp dummy = {0};
+			const tmv_t tx = timestamp_to_tmv(dummy);
+			clock_peer_delay(p->clock, p->peer_delay, tx, tx,
+					 p->nrate.ratio);
+		}
+		break;
+	case MID_SUBSCRIBE_EVENTS_NP:
+		break;
+	default:
+		pr_err("%s: pmc_recv bad mgt id 0x%x", p->log_name, mgt->id);
+		err = -1;
+		break;
+	}
+
+out:
+	msg_put(msg);
+	return err;
+}
+
 static int process_delay_req(struct port *p, struct ptp_message *m)
 {
 	struct ptp_message *msg;
@@ -2112,7 +2210,7 @@ static int process_delay_req(struct port *p, struct ptp_message *m)
 		return 0;
 	}
 
-	if (p->delayMechanism == DM_P2P) {
+	if (p->delayMechanism == DM_P2P || p->delayMechanism == DM_COMMON_P2P) {
 		pr_warning("%s: delay request on P2P port", p->log_name);
 		return 0;
 	}
@@ -2277,6 +2375,9 @@ int process_pdelay_req(struct port *p, struct ptp_message *m)
 		return -1;
 	}
 
+	if (p->delayMechanism == DM_COMMON_P2P) {
+		return 0;
+	}
 	if (p->delayMechanism == DM_E2E) {
 		pr_warning("%s: pdelay_req on E2E port", p->log_name);
 		return 0;
@@ -2472,6 +2573,9 @@ calc:
 
 int process_pdelay_resp(struct port *p, struct ptp_message *m)
 {
+	if (p->delayMechanism == DM_COMMON_P2P) {
+		return 0;
+	}
 	if (p->peer_delay_resp) {
                 if (!p->multiple_pdr_detected) {
                         pr_err("%s: multiple peer responses", p->log_name);
@@ -2642,6 +2746,48 @@ struct foreign_clock *port_compute_best(struct port *p)
 	return p->best;
 }
 
+static void port_cmlds_transition(struct port *p, enum port_state next)
+{
+	port_clr_tmo(p->fda.fd[FD_ANNOUNCE_TIMER]);
+	port_clr_tmo(p->fda.fd[FD_SYNC_RX_TIMER]);
+	port_clr_tmo(p->fda.fd[FD_DELAY_TIMER]);
+	port_clr_tmo(p->fda.fd[FD_QUALIFICATION_TIMER]);
+	port_clr_tmo(p->fda.fd[FD_MANNO_TIMER]);
+	port_clr_tmo(p->fda.fd[FD_SYNC_TX_TIMER]);
+	/* Leave FD_UNICAST_REQ_TIMER running. */
+
+	switch (next) {
+	case PS_INITIALIZING:
+		break;
+	case PS_FAULTY:
+	case PS_DISABLED:
+		port_disable(p);
+		break;
+	case PS_LISTENING:
+		port_set_announce_tmo(p);
+		break;
+	case PS_PRE_MASTER:
+		port_set_qualification_tmo(p);
+		break;
+	case PS_MASTER:
+	case PS_GRAND_MASTER:
+		if (!p->inhibit_announce) {
+			set_tmo_log(p->fda.fd[FD_MANNO_TIMER], 1, -10); /*~1ms*/
+		}
+		port_set_sync_tx_tmo(p);
+		break;
+	case PS_PASSIVE:
+		port_set_announce_tmo(p);
+		break;
+	case PS_UNCALIBRATED:
+		flush_last_sync(p);
+		/* fall through */
+	case PS_SLAVE:
+		port_set_announce_tmo(p);
+		break;
+	};
+}
+
 static void port_e2e_transition(struct port *p, enum port_state next)
 {
 	port_clr_tmo(p->fda.fd[FD_ANNOUNCE_TIMER]);
@@ -2747,10 +2893,16 @@ static void bc_dispatch(struct port *p, enum fsm_event event, int mdiff)
 		return;
 	}
 
-	if (p->delayMechanism == DM_P2P) {
+	switch (p->delayMechanism) {
+	case DM_P2P:
 		port_p2p_transition(p, p->state);
-	} else {
+		break;
+	case DM_COMMON_P2P:
+		port_cmlds_transition(p, p->state);
+		break;
+	default:
 		port_e2e_transition(p, p->state);
+		break;
 	}
 
 	if (p->jbod && p->state == PS_UNCALIBRATED && p->phc_index >= 0 ) {
@@ -2944,6 +3096,10 @@ static enum fsm_event bc_event(struct port *p, int fd_index)
 		p->service_stats.unicast_request_timeout++;
 		return unicast_client_timer(p) ? EV_FAULT_DETECTED : EV_NONE;
 
+	case FD_CMLDS:
+		pr_debug("%s: CMLDS push notification", p->log_name);
+		return process_cmlds(p) ? EV_FAULT_DETECTED : EV_NONE;
+
 	case FD_RTNL:
 		pr_debug("%s: received link status notification", p->log_name);
 		rtnl_link_status(fd, p->name, port_link_status, p);
diff --git a/port_private.h b/port_private.h
index 1ef816a..7c8fcd9 100644
--- a/port_private.h
+++ b/port_private.h
@@ -26,6 +26,7 @@
 #include "fsm.h"
 #include "monitor.h"
 #include "msg.h"
+#include "pmc_common.h"
 #include "power_profile.h"
 #include "tmv.h"
 
@@ -165,6 +166,8 @@ struct port {
 	/* slave event monitoring */
 	struct monitor *slave_event_monitor;
 	bool unicast_state_dirty;
+	struct pmc *cmlds_pmc;
+	int cmlds_port;
 };
 
 #define portnum(p) (p->portIdentity.portNumber)
-- 
2.39.2

Re: [Linuxptp-devel] [PATCH v1 0/5] Common Mean Link Delay -- proof of concept

[Richard C. <ric...@gm...>]

On Wed, Nov 29, 2023 at 11:57:16PM -0800, Richard Cochran wrote:
> This is how CMLD should be done, IMHO.

Sample configs...

~~~~~~~~~~~~~~~~
CMLDS_server.cfg
~~~~~~~~~~~~~~~~

[global]
clientOnly      1
free_running    1
uds_address     /var/run/cmlds_server
use_syslog      0
verbose         1

[eth2]
delay_mechanism P2P

[eth1]
delay_mechanism P2P

~~~~~~~~~~~~~~~~
CMLDS_client.cfg
~~~~~~~~~~~~~~~~

[global]
use_syslog      0
verbose         1

[eth1]
delay_mechanism COMMON_P2P
cmlds_port      2

Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.

[Andrew Z. <and...@in...>]

Hi Richard,

I haven't tested this but here are some comments.

On Thu, 30 Nov 2023 at 08:58, Richard Cochran <ric...@gm...> wrote:
> From: Kishen Maloor <kis...@in...>
>
> Signed-off-by: Richard Cochran <ric...@gm...>
> ---
>  config.c       |   4 ++
>  dm.h           |   3 +
>  fd.h           |   1 +
>  makefile       |   4 +-
>  port.c         | 166 +++++++++++++++++++++++++++++++++++++++++++++++--
>  port_private.h |   3 +
>  6 files changed, 174 insertions(+), 7 deletions(-)
>
> diff --git a/config.c b/config.c
> index fe65b76..05ffd3c 100644
> --- a/config.c
> +++ b/config.c
> @@ -171,6 +171,7 @@ static struct config_enum delay_filter_enu[] = {
>
>  static struct config_enum delay_mech_enu[] = {
>         { "Auto", DM_AUTO },
> +       { "COMMON_P2P", DM_COMMON_P2P },
>         { "E2E",  DM_E2E },
>         { "P2P",  DM_P2P },
>         { "NONE", DM_NO_MECHANISM },
> @@ -249,6 +250,9 @@ struct config_item config_tab[] = {
>         GLOB_ITEM_INT("clock_class_threshold", CLOCK_CLASS_THRESHOLD_DEFAULT, 6, CLOCK_CLASS_THRESHOLD_DEFAULT),
>         GLOB_ITEM_ENU("clock_servo", CLOCK_SERVO_PI, clock_servo_enu),
>         GLOB_ITEM_ENU("clock_type", CLOCK_TYPE_ORDINARY, clock_type_enu),
> +       PORT_ITEM_STR("cmlds_client_address", "/var/run/cmlds_cleint"),

I assume the use of this is simply to set unique names for per-port
sockets.  Maybe something like tempnam() could be used instead.

> +       PORT_ITEM_INT("cmlds_port", 1, 1, UINT16_MAX),
> +       PORT_ITEM_STR("cmlds_server_address", "/var/run/cmlds_server"),
>         GLOB_ITEM_ENU("dataset_comparison", DS_CMP_IEEE1588, dataset_comp_enu),
>         PORT_ITEM_INT("delayAsymmetry", 0, INT_MIN, INT_MAX),
>         PORT_ITEM_ENU("delay_filter", FILTER_MOVING_MEDIAN, delay_filter_enu),
> diff --git a/dm.h b/dm.h
> index 47bd847..80d1ce5 100644
> --- a/dm.h
> +++ b/dm.h
> @@ -34,6 +34,9 @@ enum delay_mechanism {
>         /** Peer delay mechanism. */
>         DM_P2P,
>
> +       /** Peer delay as measured by CMLDS. */
> +       DM_COMMON_P2P,
> +
>         /** No Delay Mechanism. */
>         DM_NO_MECHANISM = 0xFE,
>  };
> diff --git a/fd.h b/fd.h
> index 16420d7..9a072ab 100644
> --- a/fd.h
> +++ b/fd.h
> @@ -39,6 +39,7 @@ enum {
>         FD_SYNC_TX_TIMER,
>         FD_UNICAST_REQ_TIMER,
>         FD_UNICAST_SRV_TIMER,
> +       FD_CMLDS,
>         FD_RTNL,
>         N_POLLFD,
>  };
> diff --git a/makefile b/makefile
> index 7fc5f6f..e9c1ccc 100644
> --- a/makefile
> +++ b/makefile
> @@ -30,8 +30,8 @@ TS2PHC        = ts2phc.o lstab.o nmea.o serial.o sock.o ts2phc_generic_pps_source.o \
>   ts2phc_nmea_pps_source.o ts2phc_phc_pps_source.o ts2phc_pps_sink.o ts2phc_pps_source.o
>  OBJ    = bmc.o clock.o clockadj.o clockcheck.o config.o designated_fsm.o \
>   e2e_tc.o fault.o $(FILTERS) fsm.o hash.o interface.o monitor.o msg.o phc.o \
> - port.o port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o $(SERVOS) \
> - sk.o stats.o tc.o $(TRANSP) telecom.o tlv.o tsproc.o unicast_client.o \
> + pmc_common.o port.o port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o \
> + $(SERVOS) sk.o stats.o tc.o $(TRANSP) telecom.o tlv.o tsproc.o unicast_client.o \
>   unicast_fsm.o unicast_service.o util.o version.o
>
>  OBJECTS        = $(OBJ) hwstamp_ctl.o nsm.o phc2sys.o phc_ctl.o pmc.o pmc_agent.o \
> diff --git a/port.c b/port.c
> index 8afe1b2..75b4dd8 100644
> --- a/port.c
> +++ b/port.c
> @@ -963,7 +963,8 @@ static int port_management_fill_response(struct port *target,
>                 ptp_text_copy(cd->userDescription, &desc->userDescription);
>                 buf += sizeof(struct PTPText) + cd->userDescription->length;
>
> -               if (target->delayMechanism == DM_P2P) {
> +               if (target->delayMechanism == DM_P2P ||
> +                   target->delayMechanism == DM_COMMON_P2P) {
>                         memcpy(buf, profile_id_p2p, PROFILE_ID_LEN);
>                 } else {
>                         struct config *cfg = clock_config(target->clock);
> @@ -1868,6 +1869,33 @@ static void port_clear_fda(struct port *p, int count)
>                 p->fda.fd[i] = -1;
>  }
>
> +static int port_cmlds_initialize(struct port *p)
> +{
> +       struct config *cfg = clock_config(p->clock);
> +       struct subscribe_events_np sen = {0};
> +       const int zero_datalen = 1;
> +       const UInteger8 hops = 0;
> +
> +       p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");

Should this fall back to port_number(p)?

> +       p->cmlds_pmc = pmc_create(cfg, TRANS_UDS,
> +                                 config_get_string(cfg, p->name, "cmlds_client_address"),
> +                                 config_get_string(cfg, p->name, "cmlds_server_address"),
> +                                 hops,
> +                                 config_get_int(cfg, NULL, "domainNumber"),
> +                                 config_get_int(cfg, p->name, "transportSpecific") << 4,

The sdoId is (by either IEEE 1588 or 802.1AS) different for the CMLDS
than for a P2P_COMMON port, I think a check in port_ignore will
discard the messages as a result.  Maybe hardcode TS_CMLDS here? (with
TS_CMLDS defined in msg.h next to TS_IEEE_8021AS)

Using pmc here surely works but I wonder if it's not actually more
work than reusing c->uds_port.

> +                                 zero_datalen);
> +       if (!p->cmlds_pmc) {
> +               return -1;
> +       }
> +
> +       event_bitmask_set(sen.bitmask, NOTIFY_CMLDS, TRUE);
> +       sen.duration = 3600;
> +       p->fda.fd[FD_CMLDS] = pmc_get_transport_fd(p->cmlds_pmc);
> +       pmc_send_set_action(p->cmlds_pmc, MID_SUBSCRIBE_EVENTS_NP, &sen, sizeof(sen));
> +
> +       return 0;
> +}
> +
>  void port_disable(struct port *p)
>  {
>         int i;
> @@ -1885,6 +1913,12 @@ void port_disable(struct port *p)
>                 close(p->fda.fd[FD_FIRST_TIMER + i]);
>         }
>
> +       if (p->cmlds_pmc) {
> +               pmc_destroy(p->cmlds_pmc);
> +               p->fda.fd[FD_CMLDS] = -1;
> +               p->cmlds_pmc = NULL;
> +       }
> +
>         /* Keep rtnl socket to get link status info. */
>         port_clear_fda(p, FD_RTNL);
>         clock_fda_changed(p->clock);
> @@ -1932,7 +1966,8 @@ int port_initialize(struct port *p)
>                 pr_err("inhibit_delay_req can only be set when asCapable == 'true'.");
>                 return -1;
>         }
> -       if (port_delay_mechanism(p) == DM_NO_MECHANISM) {
> +       if (port_delay_mechanism(p) == DM_COMMON_P2P ||
> +           port_delay_mechanism(p) == DM_NO_MECHANISM) {
>                 p->inhibit_delay_req = 1;
>         }
>
> @@ -1960,6 +1995,10 @@ int port_initialize(struct port *p)
>                 goto no_tmo;
>         }
>
> +       if (port_delay_mechanism(p) == DM_COMMON_P2P && port_cmlds_initialize(p)) {
> +               goto no_tmo;
> +       }
> +
>         /* No need to open rtnl socket on UDS port. */
>         if (!port_is_uds(p)) {
>                 /*
> @@ -2101,6 +2140,65 @@ int process_announce(struct port *p, struct ptp_message *m)
>         return result;
>  }
>
> +static int process_cmlds(struct port *p)
> +{
> +       struct cmlds_info_np *cmlds;
> +       struct management_tlv *mgt;
> +       struct ptp_message *msg;
> +       struct TLV *tlv;
> +       int err = 0;
> +
> +       msg = pmc_recv(p->cmlds_pmc);
> +       if (!msg) {
> +               pr_err("%s: pmc_recv failed", p->log_name);
> +               return -1;
> +       }
> +       if (msg_type(msg) != MANAGEMENT) {
> +               pr_err("%s: pmc_recv bad message", p->log_name);
> +               err = -1;
> +               goto out;
> +       }
> +       tlv = (struct TLV *) msg->management.suffix;
> +       if (tlv->type != TLV_MANAGEMENT) {
> +               pr_err("%s: pmc_recv bad message", p->log_name);
> +               err = -1;
> +               goto out;
> +       }
> +       mgt = (struct management_tlv *) msg->management.suffix;
> +       if (mgt->length == 2 && mgt->id != MID_NULL_MANAGEMENT) {
> +               pr_err("%s: pmc_recv bad length", p->log_name);
> +               goto out;
> +       }
> +
> +       switch (mgt->id) {
> +       case MID_CMLDS_INFO_NP:
> +               if (msg->header.sourcePortIdentity.portNumber != p->cmlds_port) {
> +                       break;
> +               }
> +               cmlds = (struct cmlds_info_np *) mgt->data;
> +               p->peer_delay = nanoseconds_to_tmv(cmlds->meanLinkDelay >> 16);
> +               p->peerMeanPathDelay = tmv_to_TimeInterval(p->peer_delay);

cmlds->meanLinkDelay is already a TimeInterval in case you want to use that.

> +
> +               if (p->state == PS_UNCALIBRATED || p->state == PS_SLAVE) {
> +                       const struct timestamp dummy = {0};
> +                       const tmv_t tx = timestamp_to_tmv(dummy);

tmv_zero() could also be used.

> +                       clock_peer_delay(p->clock, p->peer_delay, tx, tx,
> +                                        p->nrate.ratio);

p->nrate.ratio hasn't been set yet, I think a line similar to this is missing:

p->nrate.ratio = 1.0 + (double) cmlds->scaledNeighborRateRatio / POW2_41;

The CMLDS port will also need to ensure that the NRR is calculated
because now it's done conditionally.

In Kishen's patch series struct cmlds_info_np also had a
.serviceMeasurementValid which enabled the port_capable() logic to
work and IIRC that is important at least for 802.1AS compliance.  If
that was to be re-added, the event would need to be emitted in an
extra place in the CMLDS, perhaps where pdr_missing is incremented.

Best regards

Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.

[Richard C. <ric...@gm...>]

On Thu, Nov 30, 2023 at 04:32:20PM +0100, Andrew Zaborowski wrote:

> > @@ -249,6 +250,9 @@ struct config_item config_tab[] = {
> >         GLOB_ITEM_INT("clock_class_threshold", CLOCK_CLASS_THRESHOLD_DEFAULT, 6, CLOCK_CLASS_THRESHOLD_DEFAULT),
> >         GLOB_ITEM_ENU("clock_servo", CLOCK_SERVO_PI, clock_servo_enu),
> >         GLOB_ITEM_ENU("clock_type", CLOCK_TYPE_ORDINARY, clock_type_enu),
> > +       PORT_ITEM_STR("cmlds_client_address", "/var/run/cmlds_cleint"),
> 
> I assume the use of this is simply to set unique names for per-port
> sockets.  Maybe something like tempnam() could be used instead.

No, it has to be under user control, because the path might be under a
security policy.
 
> > @@ -1868,6 +1869,33 @@ static void port_clear_fda(struct port *p, int count)
> >                 p->fda.fd[i] = -1;
> >  }
> >
> > +static int port_cmlds_initialize(struct port *p)
> > +{
> > +       struct config *cfg = clock_config(p->clock);
> > +       struct subscribe_events_np sen = {0};
> > +       const int zero_datalen = 1;
> > +       const UInteger8 hops = 0;
> > +
> > +       p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");
> 
> Should this fall back to port_number(p)?

I don't see how, since there is always a default for each
configuration option.  Also, I understood from you guys that the port
number _has_ to be different to pass compliance tests. For that, I
thought adding a "starting port number" option would do the trick.
 
> > +       p->cmlds_pmc = pmc_create(cfg, TRANS_UDS,
> > +                                 config_get_string(cfg, p->name, "cmlds_client_address"),
> > +                                 config_get_string(cfg, p->name, "cmlds_server_address"),
> > +                                 hops,
> > +                                 config_get_int(cfg, NULL, "domainNumber"),
> > +                                 config_get_int(cfg, p->name, "transportSpecific") << 4,
> 
> The sdoId is (by either IEEE 1588 or 802.1AS) different for the CMLDS
> than for a P2P_COMMON port, I think a check in port_ignore will
> discard the messages as a result.  Maybe hardcode TS_CMLDS here? (with
> TS_CMLDS defined in msg.h next to TS_IEEE_8021AS)
> 
> Using pmc here surely works but I wonder if it's not actually more
> work than reusing c->uds_port.

I considered that, but I invite you to try implementing that to see
which one is simpler.

> > +       switch (mgt->id) {
> > +       case MID_CMLDS_INFO_NP:
> > +               if (msg->header.sourcePortIdentity.portNumber != p->cmlds_port) {
> > +                       break;
> > +               }
> > +               cmlds = (struct cmlds_info_np *) mgt->data;
> > +               p->peer_delay = nanoseconds_to_tmv(cmlds->meanLinkDelay >> 16);
> > +               p->peerMeanPathDelay = tmv_to_TimeInterval(p->peer_delay);
> 
> cmlds->meanLinkDelay is already a TimeInterval in case you want to use that.

good point

> > +
> > +               if (p->state == PS_UNCALIBRATED || p->state == PS_SLAVE) {
> > +                       const struct timestamp dummy = {0};
> > +                       const tmv_t tx = timestamp_to_tmv(dummy);
> 
> tmv_zero() could also be used.

yeah

> > +                       clock_peer_delay(p->clock, p->peer_delay, tx, tx,
> > +                                        p->nrate.ratio);
> 
> p->nrate.ratio hasn't been set yet, I think a line similar to this is missing:
> 
> p->nrate.ratio = 1.0 + (double) cmlds->scaledNeighborRateRatio / POW2_41;

Right
 
> The CMLDS port will also need to ensure that the NRR is calculated
> because now it's done conditionally.

Huh, I didn't change that part?
 
> In Kishen's patch series struct cmlds_info_np also had a
> .serviceMeasurementValid which enabled the port_capable() logic to
> work and IIRC that is important at least for 802.1AS compliance.  If
> that was to be re-added, the event would need to be emitted in an
> extra place in the CMLDS, perhaps where pdr_missing is incremented.

serviceMeasurementValid is implicit, because if you get a PUSH
notification, it is valid.

Thanks,
Richard

Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.

[Andrew Z. <and...@in...>]

On Thu, 30 Nov 2023 at 17:44, Richard Cochran <ric...@gm...> wrote:
> On Thu, Nov 30, 2023 at 04:32:20PM +0100, Andrew Zaborowski wrote:
> > > +       PORT_ITEM_STR("cmlds_client_address", "/var/run/cmlds_cleint"),
> >
> > I assume the use of this is simply to set unique names for per-port
> > sockets.  Maybe something like tempnam() could be used instead.
>
> No, it has to be under user control, because the path might be under a
> security policy.

Do you mean that the user might want it to be under a security policy?
 Otherwise it could be in /tmp.

>
> > > @@ -1868,6 +1869,33 @@ static void port_clear_fda(struct port *p, int count)
> > >                 p->fda.fd[i] = -1;
> > >  }
> > >
> > > +static int port_cmlds_initialize(struct port *p)
> > > +{
> > > +       struct config *cfg = clock_config(p->clock);
> > > +       struct subscribe_events_np sen = {0};
> > > +       const int zero_datalen = 1;
> > > +       const UInteger8 hops = 0;
> > > +
> > > +       p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");
> >
> > Should this fall back to port_number(p)?
>
> I don't see how, since there is always a default for each
> configuration option.

Right, it'd need to default to, say, -1 which would be checked here.

> Also, I understood from you guys that the port
> number _has_ to be different to pass compliance tests. For that, I
> thought adding a "starting port number" option would do the trick

The port identity (clock identity + port number) has to be unique.  As
long as the clock identity is unique, which it has to be, I don't
think the port number *has* to be different.

>
> > In Kishen's patch series struct cmlds_info_np also had a
> > .serviceMeasurementValid which enabled the port_capable() logic to
> > work and IIRC that is important at least for 802.1AS compliance.  If
> > that was to be re-added, the event would need to be emitted in an
> > extra place in the CMLDS, perhaps where pdr_missing is incremented.
>
> serviceMeasurementValid is implicit, because if you get a PUSH
> notification, it is valid.

Right, serviceMeasurementValid == true is implicit but
serviceMeasurementValid == false is useful for the .port_id_valid /
.pdr_missing logic to work.  The timeout logic is disabled with
P2P_COMMON.

Best regards

Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.

[Richard C. <ric...@gm...>]

On Thu, Nov 30, 2023 at 07:12:54PM +0100, Andrew Zaborowski wrote:
> On Thu, 30 Nov 2023 at 17:44, Richard Cochran <ric...@gm...> wrote:
> > On Thu, Nov 30, 2023 at 04:32:20PM +0100, Andrew Zaborowski wrote:
> > > > +       PORT_ITEM_STR("cmlds_client_address", "/var/run/cmlds_cleint"),
> > >
> > > I assume the use of this is simply to set unique names for per-port
> > > sockets.  Maybe something like tempnam() could be used instead.
> >
> > No, it has to be under user control, because the path might be under a
> > security policy.
> 
> Do you mean that the user might want it to be under a security policy?

Yes.

>  Otherwise it could be in /tmp.

The top down path has to be the user's choice,
 /tmp/... or
 /var/run/... or
 /whatever/...

and so there must be a configuration option in any case.  It seems
silly to restrict the option to the leading path and not include the
socket file name.

> > > > @@ -1868,6 +1869,33 @@ static void port_clear_fda(struct port *p, int count)
> > > >                 p->fda.fd[i] = -1;
> > > >  }
> > > >
> > > > +static int port_cmlds_initialize(struct port *p)
> > > > +{
> > > > +       struct config *cfg = clock_config(p->clock);
> > > > +       struct subscribe_events_np sen = {0};
> > > > +       const int zero_datalen = 1;
> > > > +       const UInteger8 hops = 0;
> > > > +
> > > > +       p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");
> > >
> > > Should this fall back to port_number(p)?
> >
> > I don't see how, since there is always a default for each
> > configuration option.
> 
> Right, it'd need to default to, say, -1 which would be checked here.

Okay, sure.  That will reduce configuration burden for normal setups.

> > Also, I understood from you guys that the port
> > number _has_ to be different to pass compliance tests. For that, I
> > thought adding a "starting port number" option would do the trick
> 
> The port identity (clock identity + port number) has to be unique.  As
> long as the clock identity is unique, which it has to be, I don't
> think the port number *has* to be different.

Got it.

> > serviceMeasurementValid is implicit, because if you get a PUSH
> > notification, it is valid.
> 
> Right, serviceMeasurementValid == true is implicit but
> serviceMeasurementValid == false is useful for the .port_id_valid /
> .pdr_missing logic to work.  The timeout logic is disabled with
> P2P_COMMON.

Right, so the next TODO is to enable FD_DELAY_TIMER for P2P_COMMON
mode and let it 1) renew the push subscription and 2) catch the case
when the push notification does not arrive on time.

Thoughts?

Thanks,
Richard

Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.

[Kishen M. <kis...@in...>]

Hi Richard,

Firstly, thanks so much for your reviews and efforts to get
this done.

I'll just rehash a couple of points also raised by Andrew.

On this change below:

 > +static int port_cmlds_initialize(struct port *p)
 > +{
 > +	struct config *cfg = clock_config(p->clock);
 > +	struct subscribe_events_np sen = {0};
 > +	const int zero_datalen = 1;
 > +	const UInteger8 hops = 0;
 > +
 > +	p->cmlds_port = config_get_int(cfg, p->name, "cmlds_port");
 > +	p->cmlds_pmc = pmc_create(cfg, TRANS_UDS,
 > +				  config_get_string(cfg, p->name, "cmlds_client_address"),
 > +				  config_get_string(cfg, p->name, "cmlds_server_address"),
 > +				  hops,
 > +				  config_get_int(cfg, NULL, "domainNumber"),
 > +				  config_get_int(cfg, p->name, "transportSpecific") << 4,

I haven't studied the flow here, but using the P2P_COMMON instance's
transportSpecific and domainNumber above might cause the CMLDS server's 
port_ignore() to drop this message as the
CMLDS ptp4l process would be configured with
transportSpecific/domain=0x2/0.

Perhaps just passing 0x2/0 above will fix things? This is assuming that
there are no transportSpecific/domainNumber checks on the receive path
for CMLDS events (which seemed to be the case looking at the code).

On 11/30/23 11:28 AM, Richard Cochran wrote:
...
>>> serviceMeasurementValid is implicit, because if you get a PUSH
>>> notification, it is valid.
>>
>> Right, serviceMeasurementValid == true is implicit but
>> serviceMeasurementValid == false is useful for the .port_id_valid /
>> .pdr_missing logic to work.  The timeout logic is disabled with
>> P2P_COMMON.
> 
> Right, so the next TODO is to enable FD_DELAY_TIMER for P2P_COMMON
> mode and let it 1) renew the push subscription and 2) catch the case
> when the push notification does not arrive on time.
> 

Yes, in our series CMLDS queries were performed synchronously at the 
Pdelay request cadence. If serviceMeasurementValid=false then 
port->pdr_missing was incremented and immediately followed by a call to 
port_capable(). This was necessary as I recollect there was a related 
Avnu test case.

If serviceMeasurementValid=true, then we set both peer_portid_valid and 
p->nrate.ratio_valid to true so that port_capable() could evaluate to true.

Re: [Linuxptp-devel] [PATCH v1 5/5] Implement the COMMON_P2P delay mechanism.

[Richard C. <ric...@gm...>]

On Thu, Nov 30, 2023 at 04:52:44PM -0800, Kishen Maloor wrote:

> > +	p->cmlds_pmc = pmc_create(cfg, TRANS_UDS,
> > +				  config_get_string(cfg, p->name, "cmlds_client_address"),
> > +				  config_get_string(cfg, p->name, "cmlds_server_address"),
> > +				  hops,
> > +				  config_get_int(cfg, NULL, "domainNumber"),
> > +				  config_get_int(cfg, p->name, "transportSpecific") << 4,
> 
> I haven't studied the flow here, but using the P2P_COMMON instance's
> transportSpecific and domainNumber above might cause the CMLDS server's
> port_ignore() to drop this message as the
> CMLDS ptp4l process would be configured with
> transportSpecific/domain=0x2/0.
> 
> Perhaps just passing 0x2/0 above will fix things? This is assuming that
> there are no transportSpecific/domainNumber checks on the receive path
> for CMLDS events (which seemed to be the case looking at the code).

Right, those two fields must match.  So I think:

- we'll need options for those two, and

- this is another reason to use a separate pmc instance for cmlds
  rather than reusing the uds port.

> > Right, so the next TODO is to enable FD_DELAY_TIMER for P2P_COMMON
> > mode and let it 1) renew the push subscription and 2) catch the case
> > when the push notification does not arrive on time.
> > 
> 
> Yes, in our series CMLDS queries were performed synchronously at the Pdelay
> request cadence. If serviceMeasurementValid=false then port->pdr_missing was
> incremented and immediately followed by a call to port_capable(). This was
> necessary as I recollect there was a related Avnu test case.
> 
> If serviceMeasurementValid=true, then we set both peer_portid_valid and
> p->nrate.ratio_valid to true so that port_capable() could evaluate to true.

I think that logic can be implemented without any service Measurement
Valid field in the TLV.

Thanks,
Richard