Re: [Linuxptp-devel] [PATCH v2 4/7] Add BMCA config option.
On Tue, 2018-10-02 at 16:00 -0700, Richard Cochran wrote: > On Tue, Oct 02, 2018 at 04:25:32PM +, Patel, Vedang wrote: > > > > We still need to clear out the timer when the device transitions > > out of > > slave state for other events like EV_FAULT_DETECTED. So, I think we > > still need to keep it there. > So, IOW, the reset is a special case for the noop-BCMA, right? > Yes, it is a special case. So, I guess it makes sense to do it only when we are running noop-BMCA. Thanks, Vedang > Thanks, > Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/7] Add BMCA config option.
On Tue, Oct 02, 2018 at 04:25:32PM +, Patel, Vedang wrote: > We still need to clear out the timer when the device transitions out of > slave state for other events like EV_FAULT_DETECTED. So, I think we > still need to keep it there. So, IOW, the reset is a special case for the noop-BCMA, right? Thanks, Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/7] Add BMCA config option.
On Mon, 2018-10-01 at 18:26 -0700, Richard Cochran wrote: > On Mon, Oct 01, 2018 at 06:11:16PM +, Patel, Vedang wrote: > > > > On Mon, 2018-10-01 at 10:59 -0700, Richard Cochran wrote: > > > > > > On Mon, Oct 01, 2018 at 05:24:48PM +, Patel, Vedang wrote: > > > > > > > > > > > > I will add the comment for clearing the SYNC_RX_TIMER. It is > > > > basically > > > > to clear out the event returned by poll(). > > > But why? Does the original code have a bug? Please explain. > > > > > In the original code, whenever we receive a FD_SYNC_RX_TIMER > > timeout, > > it is usually accompanied by a state transition. So, > > port_*_transition() will take care of clearing the event for us (by > > calling port_clr_tmo() at the beginning). But, when BMCA == 'noop', > > there is nothing to do, so we are not clearing out the event and > > poll() > > will report the event in every loop. I am just taking care of > > clearing > > this event here. > Then you should have followed through and removed the redundant clear > in the state transition code. Or what am I missing? > We still need to clear out the timer when the device transitions out of slave state for other events like EV_FAULT_DETECTED. So, I think we still need to keep it there. Thanks, Vedang > Thanks, > Richard ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/7] Add BMCA config option.
On Mon, 2018-10-01 at 10:59 -0700, Richard Cochran wrote:
> On Mon, Oct 01, 2018 at 05:24:48PM +, Patel, Vedang wrote:
> >
> > I will add the comment for clearing the SYNC_RX_TIMER. It is
> > basically
> > to clear out the event returned by poll().
> But why? Does the original code have a bug? Please explain.
>
In the original code, whenever we receive a FD_SYNC_RX_TIMER timeout,
it is usually accompanied by a state transition. So,
port_*_transition() will take care of clearing the event for us (by
calling port_clr_tmo() at the beginning). But, when BMCA == 'noop',
there is nothing to do, so we are not clearing out the event and poll()
will report the event in every loop. I am just taking care of clearing
this event here.
> >
> > But, I don't understand how moving the inhibit_announce before this
> > will help.
> You avoid changing the behavior of the non-noop-BCMA code.
>
> >
> > I am not removing the hunk anywhere.
> You are right. I misread the next patch.
>
> BUT you avoid changing the original behavior by doing this:
>
> if (p->inhibit_announce) {
> port_clr_tmo(p->fda.fd[FD_ANNOUNCE_TIMER]);
> port_clr_tmo(p->fda.fd[FD_SYNC_RX_TIMER]);
> } else {
> port_set_announce_tmo(p);
> }
>
The SYNC_RX_TIMER needs to be cleared out whenever BMCA == 'noop' (or
any other future state machine which does not do any state transition
in this case). I will add the condition to clear out the timer only
when BMCA == 'noop'.
Thanks,
Vedang
> See?
>
> Thanks,
> Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/7] Add BMCA config option.
On Mon, Oct 01, 2018 at 05:24:48PM +, Patel, Vedang wrote:
> I will add the comment for clearing the SYNC_RX_TIMER. It is basically
> to clear out the event returned by poll().
But why? Does the original code have a bug? Please explain.
> But, I don't understand how moving the inhibit_announce before this
> will help.
You avoid changing the behavior of the non-noop-BCMA code.
> I am not removing the hunk anywhere.
You are right. I misread the next patch.
BUT you avoid changing the original behavior by doing this:
if (p->inhibit_announce) {
port_clr_tmo(p->fda.fd[FD_ANNOUNCE_TIMER]);
port_clr_tmo(p->fda.fd[FD_SYNC_RX_TIMER]);
} else {
port_set_announce_tmo(p);
}
See?
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/7] Add BMCA config option.
Hi Richard,
On Sun, 2018-09-30 at 20:02 -0700, Richard Cochran wrote:
> On Sun, Sep 30, 2018 at 08:00:51PM -0700, Richard Cochran wrote:
> >
> > On Wed, Sep 26, 2018 at 02:57:36PM -0700, Vedang Patel wrote:
> > >
> > > @@ -2472,6 +2480,7 @@ static enum fsm_event bc_event(struct port
> > > *p, int fd_index)
> > > if (p->best)
> > > fc_clear(p->best);
> > > port_set_announce_tmo(p);
> > > + port_clr_tmo(p->fda.fd[FD_SYNC_RX_TIMER]);
> > > delay_req_prune(p);
> > > if (clock_slave_only(p->clock) && p-
> > > >delayMechanism != DM_P2P &&
> > > port_renew_transport(p)) {
> > > @@ -2862,10 +2871,24 @@ struct port *port_open(int phc_index,
> > This hunk needs some kind of justification, especially since you
> > undo
> > it later in the series.
> Can you avoid this by putting the inhibit_announce patch first?
>
I will add the comment for clearing the SYNC_RX_TIMER. It is basically
to clear out the event returned by poll().
But, I don't understand how moving the inhibit_announce before this
will help. I am not removing the hunk anywhere.
Thanks,
Vedang
> Thanks,
> Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/7] Add BMCA config option.
On Sun, Sep 30, 2018 at 08:00:51PM -0700, Richard Cochran wrote:
> On Wed, Sep 26, 2018 at 02:57:36PM -0700, Vedang Patel wrote:
> > @@ -2472,6 +2480,7 @@ static enum fsm_event bc_event(struct port *p, int
> > fd_index)
> > if (p->best)
> > fc_clear(p->best);
> > port_set_announce_tmo(p);
> > + port_clr_tmo(p->fda.fd[FD_SYNC_RX_TIMER]);
> > delay_req_prune(p);
> > if (clock_slave_only(p->clock) && p->delayMechanism != DM_P2P &&
> > port_renew_transport(p)) {
> > @@ -2862,10 +2871,24 @@ struct port *port_open(int phc_index,
>
> This hunk needs some kind of justification, especially since you undo
> it later in the series.
Can you avoid this by putting the inhibit_announce patch first?
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
