Re: [Linuxptp-devel] [PATCH 1/3] phc2sys: Add clocks after processing configuration.

[Jacob K. <jac...@in...>]

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 <mli...@re...>

Looks good to me.

Reviewed-by: Jacob Keller <jac...@in...>

> ---
>  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());
>