Re: [Linuxptp-devel] [PATCH v1] ptp4l: use ethtool operation to double check PHC

[Richard C. <ric...@gm...>]

Jacob,

Thanks for this patch. Could I bother you to make a few changes?
Most of my comments are just nit picking, a few are more substantial.

On Tue, May 08, 2012 at 01:52:47PM -0700, Jacob Keller wrote:
> diff --git a/clock.h b/clock.h
> index 825f438..dd1950d 100644
> --- a/clock.h
> +++ b/clock.h
> @@ -67,14 +67,14 @@ UInteger8 clock_class(struct clock *c);
>   * so subsequent calls will destroy the previous clock instance.
>   *
>   * @param phc         PTP hardware clock device to use.

Also need:
      @param phc_index   PTP hardware clock device to use.

> - *                    Pass NULL to select CLOCK_REALTIME.
> + *                    Pass -1 to select CLOCK_REALTIME.
>   * @param interface   An array of network interfaces.
>   * @param count       The number of elements in @a interfaces.
>   * @param ds          A pointer to a default data set for the clock.
>   * @param pod         A pointer to a default port data set for the clock.
>   * @return            A pointer to the single global clock instance.
>   */
> -struct clock *clock_create(char *phc, struct interface *iface, int count,
> +struct clock *clock_create(int phc_index, struct interface *iface, int count,
>  			   struct defaultDS *ds, struct port_defaults *pod);
>  
>  /**
> diff --git a/port.c b/port.c
> index ebcc7bb..1ef2fca 100644
> --- a/port.c
> +++ b/port.c
> @@ -33,6 +33,7 @@
>  #include "tmtab.h"
>  #include "tmv.h"
>  #include "util.h"
> +#include "sk.h"
>  
>  #define PORT_MAVE_LENGTH 10
>  
> @@ -1328,6 +1329,7 @@ enum fsm_event port_event(struct port *p, int fd_index)
>  }
>  
>  struct port *port_open(struct port_defaults *pod,
> +		       int phc_index,
>  		       char *name,
>  		       enum transport_type transport,
>  		       enum timestamp_type timestamping,
> @@ -1336,11 +1338,22 @@ struct port *port_open(struct port_defaults *pod,
>  		       struct clock *clock)
>  {
>  	struct port *p = malloc(sizeof(*p));
> +	int checked_phc_index = -1;
> +
>  	if (!p)
>  		return NULL;
>  
>  	memset(p, 0, sizeof(*p));
>  
> +	if (sk_interface_phc(name, &checked_phc_index))
> +		pr_warning("get_ts_info operation not supported");

I would like to have the port number in this message, like so:

		pr_warning("port %d: get_ts_info operation not supported", number);

> +	else if ((phc_index >= 0) && (phc_index != checked_phc_index)) {

Too many parentheses here.

> +		pr_err("requested and expected phc devices don't match");
> +		pr_err("/dev/ptp%d requested, but /dev/ptp%d expected",
> +		       phc_index, checked_phc_index);

To me, saying 'expected' is confusing. How about this instead?

	else if (phc_index >= 0 && phc_index != checked_phc_index) {
		pr_err("port %d: PHC device mismatch", number);
		pr_err("port %d: /dev/ptp%d requested, but /dev/ptp%d is attached",
		        number, phc_index, checked_phc_index);

> +		return NULL;
> +	}
> +
>  	p->pod = *pod;
>  	p->name = name;
>  	p->clock = clock;
> diff --git a/port.h b/port.h
> index 6f02b73..7ecd893 100644
> --- a/port.h
> +++ b/port.h
> @@ -89,6 +89,7 @@ enum fsm_event port_event(struct port *port, int fd_index);
>   * @return A pointer to an open port on success, or NULL otherwise.
>   */
>  struct port *port_open(struct port_defaults *pod,
> +		       int phc_index,

This new parameter should also be added to the doxygen comment.

>  		       char *name,
>  		       enum transport_type transport,
>  		       enum timestamp_type timestamping,
> diff --git a/ptp4l.c b/ptp4l.c
> index cbf0d7e..b645636 100644
> --- a/ptp4l.c
> +++ b/ptp4l.c
> @@ -77,23 +77,24 @@ static void usage(char *progname)
>  		"           (may be specified multiple times)\n"
>  		" -l [num]  set the logging level to 'num'\n"
>  		" -m        slave only mode (overrides configuration file)\n"
> -		" -p [dev]  PTP hardware clock device to use, default '%s'\n"
> +		" -p [dev]  PTP hardware clock device to use, default auto\n"
>  		"           (ignored for SOFTWARE/LEGACY HW time stamping)\n"
>  		" -q        quiet mode, do not use syslog(3)\n"
>  		" -v        verbose mode, print messages to stdout\n"
>  		"\n",
> -		progname, DEFAULT_PHC);
> +		progname);
>  }
>  
>  int main(int argc, char *argv[])
>  {
> -	char *config = NULL, *phc = DEFAULT_PHC, *progname;
> +	char *config = NULL, *req_phc = NULL, *progname;
>  	int c, i, nports = 0, slaveonly = 0;
>  	struct interface iface[MAX_PORTS];
>  	enum delay_mechanism dm = DM_E2E;
>  	enum transport_type transport = TRANS_UDP_IPV4;
>  	enum timestamp_type timestamping = TS_HARDWARE;
>  	struct clock *clock;
> +	int phc_index = -1;
>  
>  	/* Process the command line arguments. */
>  	progname = strrchr(argv[0], '/');
> @@ -139,7 +140,7 @@ int main(int argc, char *argv[])
>  			dm = DM_P2P;
>  			break;
>  		case 'p':
> -			phc = optarg;
> +			req_phc = optarg;
>  			break;
>  		case 'q':
>  			print_no_syslog();
> @@ -176,8 +177,21 @@ int main(int argc, char *argv[])
>  	for (i = 0; i < nports; i++) {
>  		iface[i].timestamping = timestamping;
>  	}
> +
> +	/* determine PHC Clock index */
>  	if (timestamping == TS_SOFTWARE || timestamping == TS_LEGACY_HW) {
> -		phc = NULL;
> +		phc_index = -1;
> +	} else if (req_phc) {
> +		sscanf(req_phc, "/dev/ptp%d", &phc_index);

Here I would like to see some error checking, like:

		if (1 != sscanf(req_phc, "/dev/ptp%d", &phc_index)) {
			fprintf(stderr, "bad device string\n");
			return -1;
		}

> +	} else if (sk_interface_phc(iface[0].name, &phc_index)) {
> +		fprintf(stderr, "get_ts_info operation not supported\n"
> +				"please specify PHC device\n");
> +		return -1;
> +	}
> +
> +	if (phc_index >= 0) {
> +		fprintf(stderr, "selected /dev/ptp%d as PHC clock\n",
> +				phc_index);

This isn't really an error message. Please remove this. Instead, how
about a similar message telling the PHC index and network device in
sk_interface_phc(), and using use pr_info so that the message goes
into the log.

>  	}
>  
>  	ds.slaveOnly = FALSE;
> @@ -213,7 +227,7 @@ int main(int argc, char *argv[])
>  		ds.clockQuality.clockClass = 255;
>  	}
>  
> -	clock = clock_create(phc, iface, nports, &ds, &pod);
> +	clock = clock_create(phc_index, iface, nports, &ds, &pod);
>  	if (!clock) {
>  		fprintf(stderr, "failed to create a clock\n");
>  		return -1;
> diff --git a/sk.c b/sk.c
> index 810d036..e7d2fbe 100644
> --- a/sk.c
> +++ b/sk.c
> @@ -20,6 +20,7 @@
>  #include <errno.h>
>  #include <linux/net_tstamp.h>
>  #include <linux/sockios.h>
> +#include <linux/ethtool.h>
>  #include <net/if.h>
>  #include <netinet/in.h>
>  #include <string.h>
> @@ -29,6 +30,10 @@
>  #include "print.h"
>  #include "sk.h"
>  
> +#ifndef SIOCETHTOOL
> +#define SIOCETHTOOL 0x8946
> +#endif
> +

I don't think this is needed. Why did you add it?

>  /* private methods */
>  
>  static int hwts_init(int fd, char *device)
> @@ -83,6 +88,37 @@ int sk_interface_index(int fd, char *name)
>  	return ifreq.ifr_ifindex;
>  }
>  
> +int sk_interface_phc(char *name, int *index)
> +{

This function will not compile using an older (eg 3.0 or 3.1) kernel.
This can be fixed by sandwitching the function body in:

#ifdef ETHTOOL_GET_TS_INFO
	...
#else
	return -1;
#endif

> +	struct ethtool_ts_info info;
> +	struct ifreq ifr;
> +	int fd, err;
> +
> +	memset(&ifr, 0, sizeof(ifr));
> +	memset(&info, 0, sizeof(info));
> +	info.cmd = ETHTOOL_GET_TS_INFO;
> +	strcpy(ifr.ifr_name, name);
> +	ifr.ifr_data = &info;

Need a cast to make gcc 4.4.6 happy.

	ifr.ifr_data = (char *) &info;

Thanks,
Richard