Hi Miroslav Lichvar, Please find our comments inline.
Thanks, Amar B S > -----Original Message----- > From: Miroslav Lichvar [mailto:mlich...@redhat.com] > Sent: 20 May 2021 18:43 > To: Amar Subramanyam <asubraman...@altiostar.com> > Cc: linuxptp-devel@lists.sourceforge.net > Subject: Re: [Linuxptp-devel] [PATCH] Sync issues observed when ptp4l is ran > with jbod and client only mode (clientOnly=1 and boundary_clock_jbod=1) > > CAUTION: This email originated from outside of Altiostar. Do not click on > links > or open attachments unless you recognize the sender and you are sure the > content is safe. You will never be asked to reset your Altiostar password via > email. > > > On Mon, May 17, 2021 at 10:02:59AM +0300, Amar Subramanyam via Linuxptp- > devel wrote: > > This patch addresses the following issues when ptp4l is run on > > multiple ports with jbod and client only mode (i.e. clientOnly=1 and > > boundary_clock_jbod=1):- > > > > 1.The LISTENING port prints continuously > > "selected best master clock 000000.0000.000003 > > updating UTC offset to 37" > > > > We limited the log such that now it prints only when there is a > > change in the best-master clock. > > > > 2.The port other than SLAVE (LISTENING port) prints an error > > "port 1: master state recommended in slave only mode > > ptp4l[1205469.356]: port 1: defaultDS.priority1 probably > > misconfigured" > > for every ANNOUNCE RECEIPT Timeout. > > I think it would be better to have separate patches for the two issues. We will send out separate patches for these issues. > > > -static void clock_update_slave(struct clock *c) > > +static void clock_update_slave(struct clock *c, int mdiff) > > { > > struct parentDS *pds = &c->dad.pds; > > struct ptp_message *msg; > > > > - if (!c->best) > > + if (!c->best || !mdiff) > > return; > > Instead of adding and checking the mdiff parameter here, not doing anything, > why not just not call the function? This is another way of handling but intent is not to do checks explicitly every time function is called. We can change it the other way round. > > > diff --git a/port.c b/port.c > > index 10bb9e1..650ca00 100644 > > --- a/port.c > > +++ b/port.c > > @@ -2531,7 +2531,7 @@ void port_dispatch(struct port *p, enum > > fsm_event event, int mdiff) static void bc_dispatch(struct port *p, > > enum fsm_event event, int mdiff) { > > if (clock_slave_only(p->clock)) { > > - if (event == EV_RS_MASTER || event == EV_RS_GRAND_MASTER) { > > + if (event == EV_RS_GRAND_MASTER) { > > port_slave_priority_warning(p); > > } > > } > > Makes sense to me. > > There is a comment in bmc_state_decision() referring to this code, which might > need to be updated with this change, or maybe that whole check specific to the > Automotive profile could be removed if the log message was the only reason for > it (I'm not sure). Yes, we think the log message was the only reason for it being added. But the comment and check in bmc_state_master() is still valid and no changes are required. Designated slave only mode throws EVENT_RS_GRAND_MASTER and not the event EV_RS_MASTER, when there is no foreign master. Our change doesn't affect the comment and check in bmc_state_decision() > > Thanks, > > -- > Miroslav Lichvar _______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
