On Mon, Jan 30, 2023 at 03:39:45PM +0000, Wojtek Wasko via Linuxptp-devel wrote:
> Current code only prints a message in case the kernel returns an error
> for a clock adjustment. Failing to adjust clock leads to an inconsistent
> state in which a servo continues to report "locked" state while the
> underlying PHC's state is essentially undetermined.
>
> This change resets the servo in case of failure to adjust the clock.
I like the error handling, but I'd like to have this one patch split
into a series of four patches:
1. Let methods in clockadj.[ch] return errors to caller.
2. Implement error handling in ptp4l.
3. Implement error handling in phc2sys.
4. Implement error handling in ts2phc.
That way, in the unlikely event of a regression, we can revert 2, 3, or 4.
Also, I have a few minor comments on coding style...
> diff --git a/clock.c b/clock.c
> index 134c7c3..ff1abbf 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1778,18 +1778,20 @@ static void clock_step_window(struct clock *c)
> c->step_window_counter = c->step_window;
> }
>
> -static void clock_synchronize_locked(struct clock *c, double adj)
> +static int clock_synchronize_locked(struct clock *c, double adj)
> {
> if (c->sanity_check) {
> clockcheck_freq(c->sanity_check, clockadj_get_freq(c->clkid));
> }
> - clockadj_set_freq(c->clkid, -adj);
> + if (clockadj_set_freq(c->clkid, -adj) < 0)
> + return -1;
Please don't omit the braces, and drop the '< 0'
clockadj_set_freq returns either 0 or -1, so the test should be
simplified to 'if (clockadj_set_freq()) { ... }'
> if (c->clkid == CLOCK_REALTIME) {
> sysclk_set_sync();
> }
> if (c->sanity_check) {
> clockcheck_set_freq(c->sanity_check, -adj);
> }
> + return 0;
> }
>
> enum servo_state clock_synchronize(struct clock *c, tmv_t ingress, tmv_t
> origin)
> @@ -1841,8 +1843,10 @@ enum servo_state clock_synchronize(struct clock *c,
> tmv_t ingress, tmv_t origin)
> case SERVO_UNLOCKED:
> break;
> case SERVO_JUMP:
> - clockadj_set_freq(c->clkid, -adj);
> - clockadj_step(c->clkid, -tmv_to_nanoseconds(c->master_offset));
> + if (clockadj_set_freq(c->clkid, -adj) < 0)
> + goto servo_unlock;
Same here
> + if (clockadj_step(c->clkid,
> -tmv_to_nanoseconds(c->master_offset)) < 0)
> + goto servo_unlock;
and here
> c->ingress_ts = tmv_zero();
> if (c->sanity_check) {
> clockcheck_set_freq(c->sanity_check, -adj);
> @@ -1853,14 +1857,17 @@ enum servo_state clock_synchronize(struct clock *c,
> tmv_t ingress, tmv_t origin)
> clock_step_window(c);
> break;
> case SERVO_LOCKED:
> - clock_synchronize_locked(c, adj);
> + if (clock_synchronize_locked(c, adj) < 0)
> + goto servo_unlock;
ditto
> break;
> case SERVO_LOCKED_STABLE:
> if (c->write_phase_mode) {
> - clockadj_set_phase(c->clkid, -offset);
> + if (clockadj_set_phase(c->clkid, -offset) < 0)
> + goto servo_unlock;
ditto
> adj = 0;
> } else {
> - clock_synchronize_locked(c, adj);
> + if (clock_synchronize_locked(c, adj) < 0)
> + goto servo_unlock;
ditto
> }
> break;
> }
> @@ -1877,6 +1884,11 @@ enum servo_state clock_synchronize(struct clock *c,
> tmv_t ingress, tmv_t origin)
> clock_notify_event(c, NOTIFY_TIME_SYNC);
>
> return state;
> +
> +servo_unlock:
> + servo_reset(c->servo);
> + c->servo_state = SERVO_UNLOCKED;
> + return SERVO_UNLOCKED;
> }
>
> void clock_sync_interval(struct clock *c, int n)
> diff --git a/clockadj.c b/clockadj.c
> index 4c920b9..1437ec0 100644
> --- a/clockadj.c
> +++ b/clockadj.c
> @@ -48,7 +48,7 @@ void clockadj_init(clockid_t clkid)
> #endif
> }
>
> -void clockadj_set_freq(clockid_t clkid, double freq)
> +int clockadj_set_freq(clockid_t clkid, double freq)
> {
> struct timex tx;
> memset(&tx, 0, sizeof(tx));
> @@ -62,8 +62,11 @@ void clockadj_set_freq(clockid_t clkid, double freq)
>
> tx.modes |= ADJ_FREQUENCY;
> tx.freq = (long) (freq * 65.536);
> - if (clock_adjtime(clkid, &tx) < 0)
> + if (clock_adjtime(clkid, &tx) < 0) {
FYI here the '< 0" makes sense, because clock_adjtime is the cousin of
adjtimex which can return both -1 and positive time code.
> pr_err("failed to adjust the clock: %m");
> + return -1;
> + }
> + return 0;
> }
>
> double clockadj_get_freq(clockid_t clkid)
Thanks,
Richard
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
