On 10/5/2022 11:38 PM, Miroslav Lichvar wrote:
> Clocks specified by the -c option, or the default CLOCK_REALTIME, were
> added before the default values (e.g. sanity_freq_limit) were set in the
> private structure used by clock_add().
So if I understand, this allows clocks specified by -c to get all the
changes to config values, rather than being forced to get default values?
>
> Rework the code to save the names of sink clocks from command line and
> add them only after the configuration is complete.
>
> This also fixes the automatic mode to not add CLOCK_REALTIME twice.
> > Fixes: 33ac7d25cd92 ("phc2sys: Allow multiple sink clocks")
>
> Signed-off-by: Miroslav Lichvar <mlich...@redhat.com>
Looks good to me.
Reviewed-by: Jacob Keller <jacob.e.kel...@intel.com>
> ---
> phc2sys.8 | 2 +-
> phc2sys.c | 41 +++++++++++++++++++++++------------------
> 2 files changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/phc2sys.8 b/phc2sys.8
> index 963df83..3247fb1 100644
> --- a/phc2sys.8
> +++ b/phc2sys.8
> @@ -120,7 +120,7 @@ Specify the time sink by device (e.g. /dev/ptp1) or
> interface (e.g. eth1) or
> by name. The default is CLOCK_REALTIME (the system clock). Not compatible
> with the
> .B \-a
> -option. This option may be given multiple times.
> +option. This option may be given up to 128 times.
> .TP
> .BI \-E " servo"
> Specify which clock servo should be used. Valid values are pi for a PI
> diff --git a/phc2sys.c b/phc2sys.c
> index 2c8e905..8d2624f 100644
> --- a/phc2sys.c
> +++ b/phc2sys.c
> @@ -64,6 +64,8 @@
>
> #define PHC_PPS_OFFSET_LIMIT 10000000
>
> +#define MAX_DST_CLOCKS 128
> +
> struct clock {
> LIST_ENTRY(clock) list;
> LIST_ENTRY(clock) dst_list;
> @@ -1056,9 +1058,10 @@ static void usage(char *progname)
>
> int main(int argc, char *argv[])
> {
> - char *config = NULL, *last_dst_name = NULL, *progname, *src_name = NULL;
> + char *config = NULL, *progname, *src_name = NULL;
> + const char *dst_names[MAX_DST_CLOCKS];
> char uds_local[MAX_IFNAME_SIZE + 1];
> - int autocfg = 0, c, domain_number = 0, index, ntpshm_segment, offset;
> + int i, autocfg = 0, c, domain_number = 0, index, ntpshm_segment, offset;
> int pps_fd = -1, print_level = LOG_INFO, r = -1, rt = 0;
> int wait_sync = 0, dst_cnt = 0;
> struct config *cfg;
> @@ -1104,13 +1107,11 @@ int main(int argc, char *argv[])
> rt++;
> break;
> case 'c':
> - last_dst_name = optarg;
> - r = phc2sys_static_dst_configuration(&priv,
> - last_dst_name);
> - if (r) {
> - goto end;
> + if (dst_cnt == MAX_DST_CLOCKS) {
> + fprintf(stderr, "too many sink clocks\n");
> + goto bad_usage;
> }
> - dst_cnt++;
> + dst_names[dst_cnt++] = optarg;
> break;
> case 'd':
> pps_fd = open(optarg, O_RDONLY);
> @@ -1261,7 +1262,11 @@ int main(int argc, char *argv[])
> return c;
> }
>
> - if (autocfg && (src_name || last_dst_name || hardpps_configured(pps_fd)
> ||
> + if (!autocfg && dst_cnt == 0) {
> + dst_names[dst_cnt++] = "CLOCK_REALTIME";
> + }
> +
> + if (autocfg && (src_name || dst_cnt > 0 || hardpps_configured(pps_fd) ||
> wait_sync || priv.forced_sync_offset)) {
> fprintf(stderr,
> "autoconfiguration cannot be mixed with manual config
> options.\n");
> @@ -1279,15 +1284,8 @@ int main(int argc, char *argv[])
> goto bad_usage;
> }
>
> - if (!last_dst_name) {
> - last_dst_name = "CLOCK_REALTIME";
> - r = phc2sys_static_dst_configuration(&priv, last_dst_name);
> - if (r) {
> - goto end;
> - }
> - }
Ok, so the fix for muiltiple clock realtime is because autocfg now had
last_dst_name still unset, so we still followed through with this block
being removed. And the above block checks !autocfg, which makes it avoid
double-adding the CLOCK_REALTIME. ok.
> - if (hardpps_configured(pps_fd) && (dst_cnt > 1 ||
> - strcmp(last_dst_name, "CLOCK_REALTIME"))) {
> + if (hardpps_configured(pps_fd) && (dst_cnt != 1 ||
> + strcmp(dst_names[0], "CLOCK_REALTIME"))) {
What was the reason for changing this from dst_cnt > 1 to dst_cnt != 1?
I guess they're mostly equivalent but this would now nicely reject
zero-destination clocks?
> fprintf(stderr,
> "cannot use a pps device unless destination is
> CLOCK_REALTIME\n");
> goto bad_usage;
> @@ -1308,6 +1306,13 @@ int main(int argc, char *argv[])
> priv.kernel_leap = config_get_int(cfg, NULL, "kernel_leap");
> priv.sanity_freq_limit = config_get_int(cfg, NULL, "sanity_freq_limit");
>
> + for (i = 0; i < dst_cnt; i++) {
> + r = phc2sys_static_dst_configuration(&priv, dst_names[i]);
> + if (r) {
> + goto end;
> + }
> + }
> +
> snprintf(uds_local, sizeof(uds_local), "/var/run/phc2sys.%d",
> getpid());
>
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
