On Tue, Jun 12, 2018 at 12:40:06PM +0000, Geva, Erez wrote:
> Signed-off-by: Erez Geva <erez.geva....@siemens.com>
Please write a proper commit message with a succinct subject line.
> +static struct config_enum hwts_filter_enu[] = {
> + { "default", HWTS_FILTER_DEF },
"DEF" looks silly. Please spell out "DEFAULT".
> + { "ignore", HWTS_FILTER_IGNORE },
> + { "upgrade", HWTS_FILTER_UPGRADE },
> + { "check", HWTS_FILTER_CHECK },
Why four different modes?
What we discussed was:
> My preference would be to have an option to not adjust it. Then you could
> use hwstamp_ctl or another program before running ptp4l to set it to the
> value you want. In this mode, ptp4l would simply check to see whether the
> filter was set to a sufficient value for PTP operations and fail with an
> error if it was not.
That means just two modes:
1. normal - current code
2. check - read only ioctl; quit if setting not rich enough for the HW
time stamping mode
> + { NULL, 0 },
> +};
> +
> struct config_item config_tab[] = {
> PORT_ITEM_INT("announceReceiptTimeout", 3, 2, UINT8_MAX),
> GLOB_ITEM_INT("assume_two_step", 0, 0, 1),
> @@ -268,6 +276,7 @@ struct config_item config_tab[] = {
> GLOB_ITEM_STR("userDescription", ""),
> GLOB_ITEM_INT("utc_offset", CURRENT_UTC_OFFSET, 0, INT_MAX),
> GLOB_ITEM_INT("verbose", 0, 0, 1),
> + PORT_ITEM_ENU("hwts_filter", HWTS_FILTER_DEF, hwts_filter_enu),
Keep list in alphabetical order please.
> };
>
> static enum parser_result
> diff --git a/ptp4l.8 b/ptp4l.8
> index ae6e491..4e2e87e 100644
> --- a/ptp4l.8
> +++ b/ptp4l.8
> @@ -624,6 +624,14 @@ The time source is a single byte code that gives an idea
> of the kind
> of local clock in use. The value is purely informational, having no
> effect on the outcome of the Best Master Clock algorithm, and is
> advertised when the clock becomes grand master.
> +.TP
> +.B hwts_filter
> +Select the hardware time stamp filter setting mode;
> +Possible values are ignore, upgrade, check.
That is only three values, but you added four.
> +Some AVB/TSN applications may need the receiving hardware time stamp.
This has nothing to do with AVB/TSN. Instead, we want ptp4l to play
nicely with *any* other program that needs time stamping.
> +Upgrade mode prevent downgrading the RX filter.
> +Check mode only check but do not set.
> +The default is ignore.
You have the default as HWTS_FILTER_DEF which is distinct from
HWTS_FILTER_IGNORE.
> .SH TIME SCALE USAGE
>
> diff --git a/raw.c b/raw.c
> index 8dc50bc..ff60fea 100644
> --- a/raw.c
> +++ b/raw.c
> @@ -200,7 +200,8 @@ static void addr_to_mac(void *mac, struct address *addr)
> }
>
> static int raw_open(struct transport *t, struct interface *iface,
> - struct fdarray *fda, enum timestamp_type ts_type)
> + struct fdarray *fda, enum timestamp_type ts_type,
> + enum hwts_filter_mode ftm)
Let's not tack on other parameter to the transports. This is not a
per-port setting. Instead, we want a global variable in sk.c along
with sk_tx_timeout and sk_check_fupsync.
> @@ -55,12 +56,27 @@ static int hwts_init(int fd, const char *device, int
> rx_filter, int tx_type)
> strncpy(ifreq.ifr_name, device, sizeof(ifreq.ifr_name) - 1);
>
> ifreq.ifr_data = (void *) &cfg;
> - cfg.tx_type = tx_type;
> - cfg.rx_filter = rx_filter;
> - req = cfg;
> - err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
> - if (err < 0)
> - return err;
> +
> + if (ftm != HWTS_FILTER_IGNORE) {
> + if (ioctl(fd, SIOCGHWTSTAMP, &ifreq) < 0) {
> + pr_err("ioctl SIOCGHWTSTAMP failed: %m");
> + return -1;
> + }
> + }
> +
> + if (ftm == HWTS_FILTER_CHECK) {
> + req.tx_type = tx_type;
> + req.rx_filter = rx_filter;
> + } else {
> + cfg.tx_type = tx_type;
> + if (ftm == HWTS_FILTER_IGNORE ||
> + cfg.rx_filter != HWTSTAMP_FILTER_ALL)
> + cfg.rx_filter = rx_filter;
> + req = cfg;
> + err = ioctl(fd, SIOCSHWTSTAMP, &ifreq);
> + if (err < 0)
> + return err;
> + }
This is too convoluted and super confusing.
...
> +enum hwts_filter_mode {
> + HWTS_FILTER_DEF, /* default value for real value detection */
Sorry, I really can't follow these comments.
> + HWTS_FILTER_IGNORE, /* ignore previous mode and set new mode */
> + HWTS_FILTER_UPGRADE, /* upgrade rx filter */
> + HWTS_FILTER_CHECK, /* check filters but do not change them */
> +};
What we really want is a Boolean that makes the ioctl read only.
Thanks,
Richard
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
