Re: [Linuxptp-devel] [PATCH v2 0/1] Last (?) patch before version 4 release

2023-04-17 Thread Vincent Cheng 
On Mon, Apr 17, 2023 at 10:50:35AM +0200, Miroslav Lichvar wrote:
> On Sat, Apr 15, 2023 at 09:57:52PM -0400, Vincent Cheng wrote:
> > Compile issue after applying patch against master commit:
> > 
> >   commit 60d829b102be9b8b13357a0f7a93f3e02d44b4ce
> >   Author: Maciek Machnikowski 
> >   Date:   Tue Apr 11 15:16:47 2023 +0200
> > 
> > ---
> > port.c: In function ‘port_tx_sync’:
> > port.c:1778:35: error: ‘CTL_FOLLOW_UP’ undeclared (first use in this 
> > function)
> >   fup->header.control= CTL_FOLLOW_UP;
> 
> That's odd. These lines should be removed by the patch. I see only one
> line (a comment) containing "header.control":

You are correct.  User error.
I foolishly cut and paste from Outlook email client to quickly make the patch.
Closer inspection of Outlook rendering of email showed some issues with line 
endings
which resulted in faulty patch file.

Vincent


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH v2 0/1] Last (?) patch before version 4 release

2023-04-15 Thread Vincent Cheng 
On Sat, Apr 15, 2023 at 05:29:32PM -0700, Richard Cochran wrote:
> Based on Erez's review, I expanded Christopher's patch to remove the
> control field codes completely.  I've tested this with the following
> hardware, and nothing broke:
> 
> Intel I210
> Intel X550T
> LabX Titanium 411 AVB switch
> 
> Please give this a try with your favorite hardware time stamping
> device.  I'd like to know how many devices stop working when the
> controlField is clear.

Works fine with Xilinx Timer on a ZCU102 system.

Thanks,
Vincent

Compile issue after applying patch against master commit:

  commit 60d829b102be9b8b13357a0f7a93f3e02d44b4ce
  Author: Maciek Machnikowski 
  Date:   Tue Apr 11 15:16:47 2023 +0200

---
port.c: In function ‘port_tx_sync’:
port.c:1778:35: error: ‘CTL_FOLLOW_UP’ undeclared (first use in this function)
  fup->header.control= CTL_FOLLOW_UP;
   ^
port.c:1778:35: note: each undeclared identifier is reported only once for each 
function it appears in
port.c: In function ‘process_delay_req’:
port.c:2129:35: error: ‘CTL_DELAY_RESP’ undeclared (first use in this function)
  msg->header.control= CTL_DELAY_RESP;
   ^
port.c: In function ‘process_pdelay_req’:
port.c:2320:35: error: ‘CTL_OTHER’ undeclared (first use in this function)
  rsp->header.control= CTL_OTHER;
   ^
port.c: In function ‘port_management_construct’:
port.c:3209:35: error: ‘CTL_MANAGEMENT’ undeclared (first use in this function)
  msg->header.control= CTL_MANAGEMENT;



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 1/4] port: Expand delay_response_timeout to detect absence of delay response packets in UNLOCKED state

2023-03-13 Thread Vincent Cheng 
From: Vincent Cheng 

In case of broken network there may be an absolute absence of delay packets.
In this scenario ptp4l log messages do not indicate anything is wrong because
the delay response timer is only activate for LOCKED servo state.

This patch expands the delay_response_timeout feature to be active in the 
UNLOCKED servo state.

Signed-off-by: Vincent Cheng 
---
 port.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/port.c b/port.c
index 3453716..f2d7f57 100644
--- a/port.c
+++ b/port.c
@@ -2897,7 +2897,8 @@ static enum fsm_event bc_event(struct port *p, int 
fd_index)
if (port_delay_request(p)) {
return EV_FAULT_DETECTED;
}
-   if (p->delay_response_timeout && p->state == PS_SLAVE) {
+   if (p->delay_response_timeout && (p->state == PS_SLAVE ||
+ p->state == PS_UNCALIBRATED)) 
{
p->delay_response_counter++;
if (p->delay_response_counter >= 
p->delay_response_timeout) {
p->delay_response_counter = 0;
-- 
2.34.1



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 3/4] port: On transition to UNCALIBRATED/SLAVE only start delay request timer if delay_resp was granted

2023-03-13 Thread Vincent Cheng 
From: Vincent Cheng 

Sending delay requests prior to getting a delay response grant will incur 
unnecessary
delay_response_timeout errors if the remote master only responds to delay 
requests
when a delay reponse request was granted.

Signed-off-by: Vincent Cheng 
---
 port.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/port.c b/port.c
index f2d7f57..083710a 100644
--- a/port.c
+++ b/port.c
@@ -2676,7 +2676,9 @@ static void port_e2e_transition(struct port *p, enum 
port_state next)
/* fall through */
case PS_SLAVE:
port_set_announce_tmo(p);
-   port_set_delay_tmo(p);
+   if (unicast_client_delay_response_granted(p)) {
+   port_set_delay_tmo(p);
+   }
break;
};
 }
-- 
2.34.1



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 0/4] Add support to detect the absense of delay response in UNLOCKED state

2023-03-13 Thread Vincent Cheng 
From: Vincent Cheng 

The original intent of the delay_response_timeout submission was to kick servo 
out of LOCKED state if a valid delay response was not received within the 
configurable delay response timeout duration. In case of broken network there 
may be an absolute absence of delay packets.  In this scenario ptp4l log 
messages do not indicate anything is wrong because the delay response timer is 
only activated after LOCKED servo state.

This patch expands the delay_response_timeout feature to detect the lack of 
delay response in UNLOCKED servo state.

Before patch:
$ sudo ./ptp4l -mqSf foo.cfg --delay_response_timeout 3

  ptp4l[651.073]: port 1 (enp0s8): INITIALIZING to LISTENING on INIT_COMPLETE
  ptp4l[655.085]: port 1 (enp0s8): new foreign master 080027.fffe.fd-1
  ptp4l[656.581]: port 1 (enp0s8): LISTENING to MASTER on 
ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
  ptp4l[656.581]: selected local clock 080027.fffe.799fc5 as best master
  ptp4l[656.581]: port 1 (enp0s8): assuming the grand master role
  ptp4l[659.082]: selected best master clock 080027.fffe.fd
  ptp4l[659.083]: foreign master not using PTP timescale
  ptp4l[659.083]: port 1 (enp0s8): MASTER to UNCALIBRATED on RS_SLAVE

  // ptp4l is stuck, no delay response timeout message


After patch:
$ sudo ./ptp4l -mqSf foo.cfg --delay_response_timeout 3

  ptp4l[609.218]: port 1 (enp0s8): INITIALIZING to LISTENING on INIT_COMPLETE
  ptp4l[613.230]: port 1 (enp0s8): new foreign master 080027.fffe.fd-1
  ptp4l[615.116]: port 1 (enp0s8): LISTENING to MASTER on 
ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
  ptp4l[615.117]: selected local clock 080027.fffe.799fc5 as best master
  ptp4l[615.117]: port 1 (enp0s8): assuming the grand master role
  ptp4l[617.235]: selected best master clock 080027.fffe.fd
  ptp4l[617.235]: foreign master not using PTP timescale
  ptp4l[617.235]: port 1 (enp0s8): MASTER to UNCALIBRATED on RS_SLAVE
  ptp4l[621.414]: port 1 (enp0s8): delay response timeout
  ptp4l[621.625]: port 1 (enp0s8): delay response timeout
  ptp4l[621.836]: port 1 (enp0s8): delay response timeout
  ptp4l[621.984]: port 1 (enp0s8): delay response timeout


Vincent Cheng (4):
  port: Expand delay_response_timeout to detect absence of delay
response packets in UNLOCKED state
  unicast: Add function to query if delay response granted for best
master
  port: On transition to UNCALIBRATED/SLAVE only start delay request
timer if delay_resp was granted
  unicast: Start delay request timer on delay response grant

 port.c   |  7 +--
 unicast_client.c | 21 +
 unicast_client.h |  9 +
 3 files changed, 35 insertions(+), 2 deletions(-)

-- 
2.34.1



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 4/4] unicast: Start delay request timer on delay response grant

2023-03-13 Thread Vincent Cheng 
From: Vincent Cheng 

Signed-off-by: Vincent Cheng 
---
 unicast_client.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/unicast_client.c b/unicast_client.c
index 1d84183..e635db1 100644
--- a/unicast_client.c
+++ b/unicast_client.c
@@ -500,6 +500,7 @@ void unicast_client_grant(struct port *p, struct 
ptp_message *m,
}
unicast_client_set_renewal(p, ucma, g->durationField);
p->logMinDelayReqInterval = g->logInterMessagePeriod;
+   port_set_delay_tmo(p);
break;
case SYNC:
if ((ucma->granted & ucma->sydymsk) == ucma->sydymsk) {
-- 
2.34.1



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 2/4] unicast: Add function to query if delay response granted for best master

2023-03-13 Thread Vincent Cheng 
From: Vincent Cheng 

Signed-off-by: Vincent Cheng 
---
 unicast_client.c | 20 
 unicast_client.h |  8 
 2 files changed, 28 insertions(+)

diff --git a/unicast_client.c b/unicast_client.c
index 0843554..ef7fc5c 100644
--- a/unicast_client.c
+++ b/unicast_client.c
@@ -649,3 +649,23 @@ out:
msg_put(msg);
return err;
 }
+
+int unicast_client_delay_response_granted(struct port *p)
+{
+   struct unicast_master_address *ucma;
+
+   if (!unicast_client_enabled(p)) {
+   return 1;
+   }
+
+   if (p->best == NULL) {
+   return 0;
+   }
+
+   STAILQ_FOREACH(ucma, >unicast_master_table->addrs, list) {
+   if (cid_eq(>portIdentity.clockIdentity, 
>best->dataset.identity)) {
+   return ucma->granted & (1 << DELAY_RESP) ? 1 : 0;
+   }
+   }
+   return 0;
+}
diff --git a/unicast_client.h b/unicast_client.h
index 4a4d345..4a2a0eb 100644
--- a/unicast_client.h
+++ b/unicast_client.h
@@ -107,4 +107,12 @@ int unicast_client_msg_is_from_master_table_entry(struct 
port *p,
 int unicast_client_tx_cancel(struct port *p,
 struct unicast_master_address *dst,
 unsigned int bitmask);
+
+/**
+ * Tests whether a unicast delay response request was granted for a given port 
best master.
+ * @param p  The port in question.
+ * @return   One (1) if a unicast client and deslay response was granted 
for p->best
+ *   or zero otherwise.
+ */
+int unicast_client_delay_response_granted(struct port *p);
 #endif
-- 
2.34.1



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 0/1] Log message says foreign master is best master even if local clock has better clock class

2023-03-12 Thread Vincent Cheng 
From: Vincent Cheng 

Problem
===
When local data set has better clock class than remote server, log shows best 
master is foreign clock.
Expected log to show local clock is best master.

===
| Client   080027.fffe.799fc5 |
| |
|   clockClass  6 |
|   dataset_comparison  G.8275.x  |
|   clock_type  OC|
| |
| enp0s8  |
| port 1  |
===
  | 
  |
  Server A
  080027.fffe.fd, clockClass 7

ptp4l[8180.149]: port 1 (enp0s8): INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[8184.164]: port 1 (enp0s8): new foreign master 080027.fffe.fd-1
ptp4l[8185.489]: port 1 (enp0s8): LISTENING to MASTER on 
ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
ptp4l[8185.489]: selected local clock 080027.fffe.799fc5 as best master
ptp4l[8185.489]: port 1 (enp0s8): assuming the grand master role
ptp4l[8188.161]: selected best master clock 080027.fffe.fd
ptp4l[8188.162]: port 1 (enp0s8): assuming the grand master role


Solution

Include comparison of the local default data set and the best foreign clock 
data set for best_id assignment.


After Fix
=
// Local clock class 6 vs remote clock class 7
ptp4l[8115.316]: port 1 (enp0s8): INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[8119.327]: port 1 (enp0s8): new foreign master 080027.fffe.fd-1
ptp4l[8119.351]: port 1 (enp0s8): LISTENING to MASTER on 
ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
ptp4l[8119.351]: selected local clock 080027.fffe.799fc5 as best master
ptp4l[8119.351]: port 1 (enp0s8): assuming the grand master role
ptp4l[8123.326]: selected local clock 080027.fffe.799fc5 as best master
ptp4l[8123.326]: port 1 (enp0s8): assuming the grand master role

// Local clock class 135 vs remote clock class 7
ptp4l[8705.933]: port 1 (enp0s8): INITIALIZING to LISTENING on INIT_COMPLETE
ptp4l[8709.945]: port 1 (enp0s8): new foreign master 080027.fffe.fd-1
ptp4l[8711.514]: port 1 (enp0s8): LISTENING to MASTER on 
ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
ptp4l[8711.514]: selected local clock 080027.fffe.799fc5 as best master
ptp4l[8711.514]: port 1 (enp0s8): assuming the grand master role
ptp4l[8713.945]: selected best master clock 080027.fffe.fd
ptp4l[8713.945]: port 1 (enp0s8): MASTER to UNCALIBRATED on RS_SLAVE



Vincent Cheng (1):
  clock: Fix best master log message when D0 is better than Erbest

 clock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.34.1



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 1/1] clock: Fix best master log message when D0 is better than Erbest

2023-03-12 Thread Vincent Cheng 
From: Vincent Cheng 

When local clock data set is better than remote server, log message says remote
server is best master.  The port states are correct.

ex. local clock is clock class 6 and remote server on port 1 is clock class 7.

  ptp4l[5709.306]: selected best master clock 080027.fffe.fd
  ptp4l[5709.306]: port 1 (enp0s8): assuming the grand master role

After fix:
  ptp4l[6880.712]: selected local clock 080027.fffe.799fc5 as best master
  ptp4l[6880.712]: port 1 (enp0s8): assuming the grand master role

Signed-off-by: Vincent Cheng 
---
 clock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clock.c b/clock.c
index 75d7c40..f9ef306 100644
--- a/clock.c
+++ b/clock.c
@@ -2179,7 +2179,7 @@ static void handle_state_decision_event(struct clock *c)
best = fc;
}
 
-   if (best) {
+   if (best && c->dscmp(>dataset, clock_default_ds(c)) > 0) {
best_id = best->dataset.identity;
} else {
best_id = c->dds.clockIdentity;
-- 
2.34.1



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH] port: start sync rx timer on grant

2023-02-02 Thread Vincent Cheng 
On Fri, Jan 27, 2023 at 05:58:24AM -0800, Vadim Fedorenko via Linuxptp-devel 
wrote:
> In case of broken network there is a possibility of having management
> packets with proper data but absolute absence of sync packets. In such
> case the selected best master will stuck in HAVE_SYDY state without
> actually synchronising and will never fail because sync rx timeout timer
> is armed just after first receive of both SYNC and FOLLOW-UP packets.
> 
> The patch arms sync rx timeout timer once sync grant is received.

I agree the current sync rx timeout timer does not catch the case where
a SYNC packet is never received.  However the original intent of 
the syncReceiptTimeout was for 802.1AS-2011.

As per syncReceiptTimeout blurb in ptp4l.8:

.B syncReceiptTimeout
The number of sync/follow up messages that may go missing before
triggering a Best Master Clock election. This option is used for
running in gPTP mode according to the 802.1AS-2011 standard. Setting
this option to zero will disable the sync message timeout.
The default is 0 or disabled.

Patch https://sourceforge.net/p/linuxptp/mailman/message/31656005/
states that 'Sync rx timeout should be set only after receiving the first sync, 
see
section 10.2.7, figure 10-4 PortSyncSyncReceive state machine in 802.1AS'.

To maintain compatibility with 802.1AS we may need to introduce a separate
configuration option to configure the timeout for detecting receipt of SYNC
after HAVE_SYDY.  

Maybe something like 'syncReceiptTimeoutInitial' default 0, disabled?

The other argument for having a separate sync receipt timeout value is that 
the initial SYNC packet after a GNT may take longer to arrive (network 
congestion?)
compared to after the first SYNC has been established.

Thanks,
Vincent


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH 1/2] Improve efficiency of nullf servo synchronization

2023-01-09 Thread Vincent Cheng 
On Mon, Jan 09, 2023 at 04:09:29PM +, Vincent Cheng wrote:
> 
> 
> -Original Message-
> From: Rahul Rameshbabu via Linuxptp-devel 
>  
> Sent: Tuesday, December 20, 2022 4:37 PM
> To: linuxptp-devel@lists.sourceforge.net
> Cc: Gal Pressman ; Saeed Mahameed ; Tariq 
> Toukan 
> Subject: [Linuxptp-devel] [PATCH 1/2] Improve efficiency of nullf servo 
> synchronization
> 
> The nullf servo can now enter the SERVO_LOCKED_STABLE state by transitioning 
> first to the SERVO_LOCKED state when the offset is less than the set value 
> for offset_threshold. If offset_threshold is not set, the SERVO_LOCKED state 
> can be entered when the offset is less than or equal to the set value for 
> step_threshold.
> 
> Signed-off-by: Rahul Rameshbabu 

Acked-by: Vincent Cheng 

> ---
>  nullf.c   |  9 ++---
>  phc2sys.8 | 12 ++--
>  ptp4l.8   | 14 +++---
>  ts2phc.8  |  8 
>  4 files changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/nullf.c b/nullf.c
> index 9a40d07..8228636 100644
> --- a/nullf.c
> +++ b/nullf.c
> @@ -38,14 +38,17 @@ static double nullf_sample(struct servo *servo, int64_t 
> offset,
>  uint64_t local_ts, double weight,
>  enum servo_state *state)
>  {
> - if (!offset) {
> + long long int abs_offset = llabs(offset);
> +
> + if ((servo->offset_threshold && abs_offset < servo->offset_threshold) ||
> + (servo->step_threshold && servo->step_threshold >= abs_offset)) {
>   *state = SERVO_LOCKED;
>   return 0.0;
>   }
>  
>   if ((servo->first_update && servo->first_step_threshold &&
> -  servo->first_step_threshold < llabs(offset)) ||
> - (servo->step_threshold && servo->step_threshold < llabs(offset))) {
> +  servo->first_step_threshold < abs_offset) ||
> + (servo->step_threshold && servo->step_threshold < abs_offset)) {
>   *state = SERVO_JUMP;
>   } else {
>   *state = SERVO_UNLOCKED;

Thanks,
Vincent


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH 1/1] clock: Fix stale clock parent pid usage after best master change

2022-12-08 Thread Vincent Cheng 
On Thu, Dec 08, 2022 at 10:36:57AM EST, Richard Cochran wrote:
>On Fri, Dec 02, 2022 at 03:33:42PM -0500, vincent.cheng...@renesas.com wrote:
>
>> @@ -2015,6 +2027,8 @@ static void handle_state_decision_event(struct clock 
>> *c)
>>  c->best = best;
>>  c->best_id = best_id;
>>  
>> +clock_update_parent_identity(c);
>
>Calling this unconditionally, regardless of port state transitions,
>will update the parentPortIdentity incorrectly in some cases.
>
>According the 1588, the update should only occur when a port enters
>one of two specific states.
>
>>  LIST_FOREACH(piter, >ports, list) {
>>  enum port_state ps;
>>  enum fsm_event event;
>
>Let me suggest another way that avoids the incorrect
>parentPortIdentity update:
>
>In port_state_update() don't call unicast_client_state_changed().
>
>Instead, set a flag in the port that means "unicast state dirty".
>
>   3417 int port_state_update(struct port *p, enum fsm_event event, int mdiff)
>   3418 {
>...
>   3447 if (mdiff) {
>   3448 -   unicast_client_state_changed(p);
>+   p->unicast_state_dirty = true;
>   3449 }
>   3450 if (next != p->state) {
>   3451 port_show_transition(p, next, event);
>   3452 p->state = next;
>   3453 port_notify_event(p, NOTIFY_PORT_STATE);
>   3454 -   unicast_client_state_changed(p);
>+   p->unicast_state_dirty = true;
>   3455 return 1;
>   3456 }
>
>Then, in handle_state_decision_event(), after the big
>LIST_FOREACH(piter, >ports, list) loop, iterate once again over the
>ports:
>
>LIST_FOREACH(piter, >ports, list) {
>port_update_unicast_state(piter);
>}
>
>where port_update_unicast_state() calls unicast_client_state_changed()
>and clears the flag.
>
>Thanks,
>Richard

Thank-you for the feedback.  Implemented above suggestions in PATCH v2.

Thanks,
Vincent


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 2/3] unicast_client: fix checkpatch ERROR: trailing whitespace

2022-12-06 Thread vincent . cheng . xh 
From: Vincent Cheng 

Signed-off-by: Vincent Cheng 
---
 unicast_client.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/unicast_client.h b/unicast_client.h
index e643cb2..4a4d345 100644
--- a/unicast_client.h
+++ b/unicast_client.h
@@ -93,7 +93,7 @@ int unicast_client_timer(struct port *p);
  * @return   One (1) if the message is from an entry in the unicast
  *   master table, or zero otherwise.
  */
-int unicast_client_msg_is_from_master_table_entry(struct port *p, 
+int unicast_client_msg_is_from_master_table_entry(struct port *p,
  struct ptp_message *m);
 
 /**
-- 
2.34.1



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 1/3] unicast_client: stop sending abnormal contract cancel requests

2022-12-06 Thread vincent . cheng . xh 
From: Vincent Cheng 

On termination of a ptp4l unicast session, ptp4l sends
CANCEL_UNICAST_TRANSMISSION for announce, sync and delay
response messages for all entries of the unicast master table
regardless if the service is active or inactive.

This patch modifies unicast_client_tx_cancel() to only send
CANCEL_UNICAST_TRANSMISSION for the unicast services that have
been granted, ie. stop sending abnormal contract cancel requests.

Signed-off-by: Vincent Cheng 
---
 port.c   |  4 ++--
 unicast_client.c | 27 +++
 unicast_client.h | 12 +---
 3 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/port.c b/port.c
index 1866a20..fe1f9a8 100644
--- a/port.c
+++ b/port.c
@@ -177,7 +177,7 @@ static void port_cancel_unicast(struct port *p)
 
STAILQ_FOREACH(ucma, >unicast_master_table->addrs, list) {
if (ucma) {
-   unicast_client_tx_cancel(p, ucma);
+   unicast_client_tx_cancel(p, ucma, UNICAST_CANCEL_ALL);
}
}
 }
@@ -194,7 +194,7 @@ static int port_unicast_message_valid(struct port *p, 
struct ptp_message *m)
pr_warning("%s: new foreign master %s not in unicast master 
table",
   p->log_name, pid2str(>header.sourcePortIdentity));
 
-   if (unicast_client_tx_cancel(p, )) {
+   if (unicast_client_tx_cancel(p, , (1 << msg_type(m {
pr_warning("%s: cancel unicast transmission to %s 
failed",
   p->log_name, 
pid2str(>header.sourcePortIdentity));
}
diff --git a/unicast_client.c b/unicast_client.c
index 039967f..fcaeb65 100644
--- a/unicast_client.c
+++ b/unicast_client.c
@@ -592,28 +592,39 @@ int unicast_client_msg_is_from_master_table_entry(struct 
port *p, struct ptp_mes
 }
 
 int unicast_client_tx_cancel(struct port *p,
-struct unicast_master_address *dst)
+struct unicast_master_address *dst,
+unsigned int bitmask)
 {
struct ptp_message *msg;
int err;
 
+   if (!(dst->granted & bitmask)) {
+   return 0;
+   }
msg = port_signaling_uc_construct(p, >address, >portIdentity);
if (!msg) {
return -1;
}
-   err = attach_cancel(msg, ANNOUNCE);
-   if (err) {
-   goto out;
+   if (dst->granted & (bitmask & (1 << ANNOUNCE))) {
+   err = attach_cancel(msg, ANNOUNCE);
+   if (err) {
+   goto out;
+   }
+   dst->granted &= ~(1 << ANNOUNCE);
}
-   err = attach_cancel(msg, SYNC);
-   if (err) {
-   goto out;
+   if (dst->granted & (bitmask & (1 << SYNC))) {
+   err = attach_cancel(msg, SYNC);
+   if (err) {
+   goto out;
+   }
+   dst->granted &= ~(1 << SYNC);
}
-   if (p->delayMechanism != DM_P2P) {
+   if (dst->granted & (bitmask & (1 << DELAY_RESP))) {
err = attach_cancel(msg, DELAY_RESP);
if (err) {
goto out;
}
+   dst->granted &= ~(1 << DELAY_RESP);
}
 
err = port_prepare_and_send(p, msg, TRANS_GENERAL);
diff --git a/unicast_client.h b/unicast_client.h
index 18a12c8..e643cb2 100644
--- a/unicast_client.h
+++ b/unicast_client.h
@@ -20,6 +20,9 @@
 #ifndef HAVE_UNICAST_CLIENT_H
 #define HAVE_UNICAST_CLIENT_H
 
+#define UNICAST_CANCEL_ALL  (1 << ANNOUNCE | 1 << SYNC | 1 << DELAY_RESP)
+#define UNICAST_CANCEL_SYDY (1 << SYNC | 1 << DELAY_RESP)
+
 /**
  * Handles a CANCEL_UNICAST_TRANSMISSION TLV from the grantor.
  * @param p  The port on which the signaling message was received.
@@ -95,10 +98,13 @@ int unicast_client_msg_is_from_master_table_entry(struct 
port *p,
 
 /**
  * Transmit CANCEL_UNICAST_TRANSMISSION TLV to destination address.
- * @param p  The port in question.
- * @param dstThe destination address.
+ * @param pThe port in question.
+ * @param dst  The destination address.
+ * @param bitmask  Cancel message type bitmask
+ * @param
  * @return   Zero on success, non-zero otherwise.
  */
 int unicast_client_tx_cancel(struct port *p,
-struct unicast_master_address *dst);
+struct unicast_master_address *dst,
+unsigned int bitmask);
 #endif
-- 
2.34.1



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 3/3] unicast_client: cancel sync/delay_response on UC_EV_UNSELECTED event

2022-12-06 Thread vincent . cheng . xh 
From: Vincent Cheng 

When a unicast master is no longer the best master, an UC_EV_UNSELECTED
event occurs for that unicast master entry. However, the sync/delay_response
contracts are not explicitly cancelled.

This patch updates unicast_client_state_changed() to send 
CANCEL_UNICAST_TRANSMISSION to cancel sync/delay_response messages on 
UC_EV_UNSELECTED event.

Signed-off-by: Vincent Cheng 
---
 unicast_client.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/unicast_client.c b/unicast_client.c
index fcaeb65..011c1c7 100644
--- a/unicast_client.c
+++ b/unicast_client.c
@@ -527,6 +527,7 @@ void unicast_client_state_changed(struct port *p)
 {
struct unicast_master_address *ucma;
struct PortIdentity pid;
+   enum unicast_state prev_state;
 
if (!unicast_client_enabled(p)) {
return;
@@ -537,7 +538,13 @@ void unicast_client_state_changed(struct port *p)
if (pid_eq(>portIdentity, )) {
ucma->state = unicast_fsm(ucma->state, UC_EV_SELECTED);
} else {
+   prev_state = ucma->state;
+
ucma->state = unicast_fsm(ucma->state, 
UC_EV_UNSELECTED);
+
+   if ((prev_state != ucma->state) && (prev_state == 
UC_HAVE_SYDY)) {
+   unicast_client_tx_cancel(p, ucma, 
UNICAST_CANCEL_SYDY);
+   }
}
}
 }
-- 
2.34.1



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Linuxptp-devel] [PATCH 0/3] unicast_client: stop sending CANCEL_UNICAST_TRANSMISSION for inactive services

2022-12-06 Thread vincent . cheng . xh 
From: Vincent Cheng 

This patch set improves the sending of CANCEL_UNICAST_TRANSMISSION to cancel 
service.

Currently all entries in the unicast master table receive 
CANCEL_UNICAST_TRANSMISSION
for announce and sync/delay_response messages regardless if the service is 
active or inactive.

First patch stops sending cancel request for inactive services on program 
termination.
Second patch fixes "ERROR: trailing whitespace" from checkpatch.pl.
Third patch informs grantor sync/delay_resp service not needed after 
UC_EV_UNSELECTED event.

Vincent Cheng (3):
  unicast_client: stop sending abnormal contract cancel requests
  unicast_client: fix checkpatch ERROR: trailing whitespace
  unicast_client: cancel sync/delay_response on UC_EV_UNSELECTED event

 port.c   |  4 ++--
 unicast_client.c | 34 ++
 unicast_client.h | 14 ++
 3 files changed, 38 insertions(+), 14 deletions(-)

-- 
2.34.1



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH] port: Cancel unicast service when closing port.

2022-03-13 Thread Vincent Cheng 
On Sat, Mar 12, 2022 at 09:33:18PM EST, Richard Cochran wrote:
>On Sat, Mar 12, 2022 at 06:07:11PM -0500, vincent.cheng...@renesas.com wrote:
>
>> However, because there is no checking of received unicast master to 
>> configured
>> unicast table, ptp4l is quite content with packets from previous master 
>> 10.64.10.13.
>
>This patch doesn't actually fix the root cause.
>
>If the cancel request fails or is ignored by the remote master, then
>the master will continue to send Sync messages, and the issue
>persists.
>
>In general, there is no way for a program to enforce good behavior
>from another networked computer.
>
>The proper fix would be to check the source address of the received
>messages.

Added more comprehensive fix. PATCH v2 to follow.

Thanks,
Vincent


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH v4 1/1] clock: Introduce step_window to free run x Sync events after a clock step.

2021-02-21 Thread Vincent Cheng 
On Sun, Feb 21, 2021 at 02:08:02PM EST, Richard Cochran wrote:
>On Mon, Feb 15, 2021 at 09:58:53AM -0500, vincent.cheng...@renesas.com wrote:
>> @@ -1730,7 +1743,10 @@ enum servo_state clock_synchronize(struct clock *c, 
>> tmv_t ingress, tmv_t origin)
>>  
>>  c->cur.offsetFromMaster = tmv_to_TimeInterval(c->master_offset);
>>  
>> -if (c->free_running) {
>> +if (c->free_running || c->step_window_counter) {
>> +if (c->step_window_counter) {
>> +c->step_window_counter--;
>> +}
>>  return clock_no_adjust(c, ingress, origin);
>
>This isn't quite right.  clock_no_adjust() will return the wrong servo
>state.  Also, we want to prevent the Sync messages from entering the
>tsproc.
>
>I think the correct way is to drop the Sync messages altogether.
>
>I hacked your patch with my suggested changes, and I'll post it
>shortly.  Please review it and let me know what you think.

I have reviewed the hacked patch and it looks good.  Works for me.
Thank-you.

Vincent


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH v3 1/1] clock: Introduce step_window to free run x Sync events after after a clock step.

2021-02-15 Thread Vincent Cheng 
On Sun, Feb 14, 2021 at 11:22:13PM EST, Richard Cochran wrote:
>Vincent,
>
>This is shaping up nicely.  I only have a few nits to pick...

Thank-you for taking the time to point them out.

>On Sun, Feb 14, 2021 at 05:22:09PM -0500, vincent.cheng...@renesas.com wrote:
>
>> @@ -1193,6 +1195,9 @@ struct clock *clock_create(enum clock_type type, 
>> struct config *config,
>>  return NULL;
>>  }
>>  
>> +c->step_window = config_get_int(config, NULL, "step_window");
>> +c->step_window_counter = 0;
>
>No need to initialize to zero.

Got it, thanks. Will fix in v4.

>> @@ -1695,6 +1700,14 @@ int clock_switch_phc(struct clock *c, int phc_index)
>>  return 0;
>>  }
>>  
>> +static void clock_step_window(struct clock *c)
>> +{
>> +if (!c->step_window)
>> +return;
>
>Please always include curly brackets:  if (test) { ... }

D'oh. Sorry about that. Fixed in v4.

>
>> +
>> +c->step_window_counter = c->step_window;
>> +}
>> +
>>  static void clock_synchronize_locked(struct clock *c, double adj)
>>  {
>>  clockadj_set_freq(c->clkid, -adj);
>> @@ -1730,7 +1743,9 @@ enum servo_state clock_synchronize(struct clock *c, 
>> tmv_t ingress, tmv_t origin)
>>  
>>  c->cur.offsetFromMaster = tmv_to_TimeInterval(c->master_offset);
>>  
>> -if (c->free_running) {
>> +if (c->free_running || (c->step_window_counter)) {
>
>No need for parenthesis here --^
>
>> +if (c->step_window_counter)
>> +c->step_window_counter--;
>
>Curly brackets, please.

Fixed in v4.

Thanks,
Vincent


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH v2 0/1] clock: Introduce step_window to free run n Sync events after a clock step.

2021-02-14 Thread Vincent Cheng 
On Sun, Feb 14, 2021 at 04:30:29AM EST, Luigi 'Comio' Mantellini wrote:
>Hi,
>
>This patch cover my scenario where the timestamper can be aligned once at
>second.
>Can I suggest to a avoid to touch free_running variable and introduce
>another one?

Hi Luigi,

Sure. Fixed in v3 patch.

Vincent



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH 1/1] clock: Introduce step_window to free run x seconds after a clock step.

2021-02-13 Thread Vincent Cheng 
On Sat, Feb 13, 2021 at 10:28:32AM EST, Richard Cochran wrote:
>On Mon, Feb 01, 2021 at 05:55:41PM -0500, vincent.cheng...@renesas.com wrote:
>
>> +/* Set up timer expire notification mechanism */
>> +se.sigev_notify = SIGEV_THREAD;
>> +se.sigev_value.sival_ptr = (void *)c;
>> +se.sigev_notify_function = clock_clear_free_running;
>> +se.sigev_notify_attributes = NULL;
>> +
>> +if (timer_create(CLOCK_MONOTONIC, , >step_timer_id)) {
>> +pr_err("failed to create step timer_id");
>> +return NULL;
>> +}
>
>I agree with the premise of this patch, but I dislike the
>implementation as it uses a timer with asynchronous signal.
>
>The code already has multiple timers, and these are managed using the
>timerfd API.  This is the modern way of handling asynchronous timers.
>
>I can see two better ways of implementing this new feature.
>
>1. If you really want a to use a timer and keep the option value as a
>   time value, then increase N_CLOCK_PFD and add the timer to
>   clock.pollfd.
>
>   If you choose this option, please increase the resolution of the
>   step_window to milliseconds.  After all, the window of expected
>   stale time stamps may be much shorter than one second!
>
>2. You could define the step_window in terms of the number of Sync
>   events to skip.  That reduces the implementation to a simple
>   counter.  The option would still relate to time, because a Sync
>   rate of X means 2^X seconds.
>
>I prefer #2 because it is much simpler.
>
>Thoughts?

I will choose door #2 and get back to you.
Thank-you for the good feedback.

Vincent


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [RFC Patch 2/2] port: Fix link down/up to continue using phc_index set from command line -p option.

2021-01-07 Thread Vincent Cheng 
Hi Jacob,

On Thu, Jan 07, 2021 at 02:33:56PM EST, Jacob Keller wrote:
>
>
>On 1/6/2021 11:39 AM, vincent.cheng...@renesas.com wrote:
>> 
>> diff --git a/port.c b/port.c
>> index db3e9ac..bdc73e4 100644
>> --- a/port.c
>> +++ b/port.c
>> @@ -2568,7 +2568,8 @@ void port_link_status(void *ctx, int linkup, int 
>> ts_index)
>> "timestamping mode, set link status down 
>> by force.",
>> interface_label(p->iface));
>>  p->link_status = LINK_DOWN | LINK_STATE_CHANGED;
>> -} else if (p->phc_index != 
>> interface_phc_index(p->iface)) {
>> +} else if ((p->phc_index != 
>> interface_phc_index(p->iface)) &&
>> +   (!p->phc_from_cmdline)) {
>>  p->phc_index = interface_phc_index(p->iface);
>>  
>
>Makes sense. But maybe we would want to do a warning about when the PHC
>index on the networking device no longer matches the command line? (like
>we do during the initial setup?)

Good idea.  Will add in same message from initial setup in next submission.


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [RFC Patch 1/2] port: Add phc_from_cmdline to indicate the phc index was from the command line.

2021-01-07 Thread Vincent Cheng 
Hi Jacob,

On Thu, Jan 07, 2021 at 02:32:47PM EST, Jacob Keller wrote:
>
>
>On 1/6/2021 11:39 AM, vincent.cheng...@renesas.com wrote:
>> diff --git a/port_private.h b/port_private.h
>> index fcabaa6..6e40e15 100644
>> --- a/port_private.h
>> +++ b/port_private.h
>> @@ -69,6 +69,7 @@ struct port {
>>  struct fdarray fda;
>>  int fault_fd;
>>  int phc_index;
>> +int phc_from_cmdline;
>>  
>
>This doesn't need to be a separate patch. You can just squash these
>together, since there is no user added in this patch.

Will fix squash together in next submission. Thanks.


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel