[PATCH ethtool] ethtool: add support show/set-hwtstamp

[PATCH ethtool] ethtool: add support show/set-hwtstamp

[Kevin(Yudong) Yang]

Before this patch, ethtool has -T/--show-time-stamping that only
shows the device's time stamping capabilities but not the time
stamping policy that is used by the device.

This patch adds support to set/get device time stamping policy at
the driver level by calling ioctl(SIOCSHWTSTAMP).

Tested: ran following cmds on a Mellanox NIC with mlx4_en driver:
./ethtool -T eth1
Hardware Transmit Timestamp Modes:
        off                   (HWTSTAMP_TX_OFF)
        on                    (HWTSTAMP_TX_ON)
Hardware Receive Filter Modes:
        none                  (HWTSTAMP_FILTER_NONE)
        all                   (HWTSTAMP_FILTER_ALL)

./ethtool --show-hwtstamp eth1;
Rx filter 0, none                  (HWTSTAMP_FILTER_NONE)
Tx type 0, off                   (HWTSTAMP_TX_OFF)

./ethtool --set-hwtstamp eth1 rx 1; ./ethtool --show-hwtstamp eth1;
Rx filter 1, all                   (HWTSTAMP_FILTER_ALL)
Tx type 0, off                   (HWTSTAMP_TX_OFF)

./ethtool --set-hwtstamp eth1 rx 1 tx 1; ./ethtool --show-hwtstamp eth1;
rx unmodified, ignoring
Rx filter 1, all                   (HWTSTAMP_FILTER_ALL)
Tx type 1, on                    (HWTSTAMP_TX_ON)

./ethtool --set-hwtstamp eth1 rx 0; ./ethtool --show-hwtstamp eth1;
Rx filter 0, none                  (HWTSTAMP_FILTER_NONE)
Tx type 1, on                    (HWTSTAMP_TX_ON)

./ethtool --set-hwtstamp eth1 tx 0; ./ethtool --show-hwtstamp eth1
Rx filter 0, none                  (HWTSTAMP_FILTER_NONE)
Tx type 0, off                   (HWTSTAMP_TX_OFF)

./ethtool --set-hwtstamp eth1 rx 123 tx 456
rx should be in [0..15], tx should be in [0..2]

Signed-off-by: Kevin Yang <yyd@google.com>
Reviewed-by: Neal Cardwell <ncardwell@google.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
 ethtool.8.in | 14 +++++++++
 ethtool.c    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+)

diff --git a/ethtool.8.in b/ethtool.8.in
index a50a476..c2d1b42 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -315,6 +315,14 @@ ethtool \- query or control network driver and hardware settings
 .B ethtool \-T|\-\-show\-time\-stamping
 .I devname
 .HP
+.B ethtool \-\-show\-hwtstamp
+.I devname
+.HP
+.B ethtool \-\-set\-hwtstamp
+.I devname
+.BN rx
+.BN tx
+.HP
 .B ethtool \-x|\-\-show\-rxfh\-indir|\-\-show\-rxfh
 .I devname
 .HP
@@ -1026,6 +1034,12 @@ Sets the dump flag for the device.
 Show the device's time stamping capabilities and associated PTP
 hardware clock.
 .TP
+.B \-\-show\-hwtstamp
+Show the device's time stamping policy at the driver level.
+.TP
+.B \-\-set\-hwtstamp
+Set the device's time stamping policy at the driver level.
+.TP
 .B \-x \-\-show\-rxfh\-indir \-\-show\-rxfh
 Retrieves the receive flow hash indirection table and/or RSS hash key.
 .TP
diff --git a/ethtool.c b/ethtool.c
index 606af3e..466b3a3 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4719,6 +4719,79 @@ static int do_tsinfo(struct cmd_context *ctx)
 	return 0;
 }
 
+static int do_ghwtstamp(struct cmd_context *ctx)
+{
+	struct hwtstamp_config cfg = { 0 };
+
+	ctx->ifr.ifr_data = (void *)&cfg;
+	if (ioctl(ctx->fd, SIOCGHWTSTAMP, &ctx->ifr)) {
+		perror("Cannot get device time stamping settings");
+		return -1;
+	}
+
+	printf("Rx filter %d", cfg.rx_filter);
+	if (cfg.rx_filter < N_RX_FILTERS)
+		printf(", %s\n", rx_filter_labels[cfg.rx_filter]);
+	else
+		printf("\n");
+
+	printf("Tx type %d", cfg.tx_type);
+	if (cfg.tx_type < N_TX_TYPES)
+		printf(", %s\n", tx_type_labels[cfg.tx_type]);
+	else
+		printf("\n");
+	return 0;
+}
+
+static int do_shwtstamp(struct cmd_context *ctx)
+{
+	struct hwtstamp_config cfg = { .rx_filter = -1, .tx_type = -1 },
+			       pre_cfg;
+	int changed = 0;
+	struct cmdline_info cmdline_config[] = {
+		{
+			.name		= "rx",
+			.type		= CMDL_S32,
+			.wanted_val	= &cfg.rx_filter,
+			.ioctl_val	= &pre_cfg.rx_filter,
+		},
+		{
+			.name		= "tx",
+			.type		= CMDL_S32,
+			.wanted_val	= &cfg.tx_type,
+			.ioctl_val	= &pre_cfg.tx_type,
+		},
+	};
+
+	parse_generic_cmdline(ctx, &changed,
+			      cmdline_config, ARRAY_SIZE(cmdline_config));
+
+	ctx->ifr.ifr_data = (void *)&pre_cfg;
+	if (ioctl(ctx->fd, SIOCGHWTSTAMP, &ctx->ifr)) {
+		perror("Cannot get device time stamping settings");
+		return -1;
+	}
+
+	changed = 0;
+	do_generic_set(cmdline_config, ARRAY_SIZE(cmdline_config), &changed);
+	if (!changed) {
+		fprintf(stderr, "no time-stamping policy changed.\n");
+		return 0;
+	}
+
+	if (cfg.tx_type >= N_TX_TYPES || cfg.rx_filter >= N_RX_FILTERS) {
+		printf("rx should be in [0..%d], tx should be in [0..%d]\n",
+		       N_RX_FILTERS - 1, N_TX_TYPES - 1);
+		return 1;
+	}
+
+	if (ioctl(ctx->fd, SIOCSHWTSTAMP, &ctx->ifr)) {
+		perror("Cannot set device time stamping settings");
+		return -1;
+	}
+	return 0;
+}
+
 static int do_getmodule(struct cmd_context *ctx)
 {
 	struct ethtool_modinfo modinfo;
@@ -5735,6 +5808,18 @@ static const struct option args[] = {
 		.nlfunc	= nl_tsinfo,
 		.help	= "Show time stamping capabilities"
 	},
+	{
+		.opts	= "--show-hwtstamp",
+		.func	= do_ghwtstamp,
+		.help	= "Show time stamping policy at the driver level"
+	},
+	{
+		.opts	= "--set-hwtstamp",
+		.func	= do_shwtstamp,
+		.help	= "Set time stamping policy at the driver level",
+		.xhelp	= "		[ rx N ]\n"
+			  "		[ tx N ]\n"
+	},
 	{
 		.opts	= "-x|--show-rxfh-indir|--show-rxfh",
 		.func	= do_grxfh,
-- 
2.28.0.402.g5ffc5be6b7-goog

Re: [PATCH ethtool] ethtool: add support show/set-hwtstamp

[Andrew Lunn]

On Tue, Sep 01, 2020 at 05:20:09PM -0400, Kevin(Yudong) Yang wrote:
> Before this patch, ethtool has -T/--show-time-stamping that only
> shows the device's time stamping capabilities but not the time
> stamping policy that is used by the device.

Hi Kavin

How does this differ from hwstamp_ctl(1)?

    Andrew

Re: [PATCH ethtool] ethtool: add support show/set-hwtstamp

[Eric Dumazet]

On Wed, Sep 2, 2020 at 2:36 AM Kevin Yang <yyd@google.com> wrote:
>
> Hi Andrew,
>
> They are pretty much the same, both use ioctl(SIOCSHWTSTAMP).
>
> Adding this to ethtool is because:
> - This time stamping policy is a hardware setting aligned with ethtool's purpose "query and control network device driver and hardware settings"
> - ethtool is widely used, system administrators don't need to install another binary to control this feature.
>


Absolutely. ethtool has -T support already, and is the facto tool for
things not covered by iproute2 suite.

Having to remember a myriad of binaries that basically use a single
ioctl() is time consuming.

> Kevin
>
> On Tue, Sep 1, 2020 at 6:02 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Tue, Sep 01, 2020 at 05:20:09PM -0400, Kevin(Yudong) Yang wrote:
>> > Before this patch, ethtool has -T/--show-time-stamping that only
>> > shows the device's time stamping capabilities but not the time
>> > stamping policy that is used by the device.
>>
>> Hi Kavin
>>
>> How does this differ from hwstamp_ctl(1)?
>>
>>     Andrew

Re: [PATCH ethtool] ethtool: add support show/set-hwtstamp

[Michal Kubecek]

On Tue, Sep 01, 2020 at 08:36:08PM -0400, Kevin Yang wrote:
> On Tue, Sep 1, 2020 at 6:02 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > On Tue, Sep 01, 2020 at 05:20:09PM -0400, Kevin(Yudong) Yang wrote:
> > > Before this patch, ethtool has -T/--show-time-stamping that only
> > > shows the device's time stamping capabilities but not the time
> > > stamping policy that is used by the device.
> >
> > How does this differ from hwstamp_ctl(1)?
> 
> They are pretty much the same, both use ioctl(SIOCSHWTSTAMP).
> 
> Adding this to ethtool is because:
> - This time stamping policy is a hardware setting aligned with ethtool's
> purpose "query and control network device driver and hardware settings"
> - ethtool is widely used, system administrators don't need to install
> another binary to control this feature.

Adding this feature to ethtool IMHO makes good sense, I'm just not sure
if it's necessary to add new subcommands, perhaps we could add the
"show" part to -T / --show-time-stamping and add --set-time-stamping.

However, I don't like the idea of adding a new ioctl based interface to
ethtool while we are working on replacing and deprecating existing one.
I would much rather like adding this to the netlink interface (which
would, of course, require also kernel side implementation).

Michal

Re: [PATCH ethtool] ethtool: add support show/set-hwtstamp

[Kevin Yang]

On Wed, Sep 2, 2020 at 3:04 AM Michal Kubecek <mkubecek@suse.cz> wrote:
>
> On Tue, Sep 01, 2020 at 08:36:08PM -0400, Kevin Yang wrote:
> > On Tue, Sep 1, 2020 at 6:02 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > On Tue, Sep 01, 2020 at 05:20:09PM -0400, Kevin(Yudong) Yang wrote:
> > > > Before this patch, ethtool has -T/--show-time-stamping that only
> > > > shows the device's time stamping capabilities but not the time
> > > > stamping policy that is used by the device.
> > >
> > > How does this differ from hwstamp_ctl(1)?
> >
> > They are pretty much the same, both use ioctl(SIOCSHWTSTAMP).
> >
> > Adding this to ethtool is because:
> > - This time stamping policy is a hardware setting aligned with ethtool's
> > purpose "query and control network device driver and hardware settings"
> > - ethtool is widely used, system administrators don't need to install
> > another binary to control this feature.
>
> Adding this feature to ethtool IMHO makes good sense, I'm just not sure
> if it's necessary to add new subcommands, perhaps we could add the
> "show" part to -T / --show-time-stamping and add --set-time-stamping.

Thanks for the nice suggestion. This sounds good to me, I will work on v2
to have:
"-T / --show-time-stamping" to show capabilities AND current policy;
and "--set-time-stamping" to change the policy.

> However, I don't like the idea of adding a new ioctl based interface to
> ethtool while we are working on replacing and deprecating existing one.
> I would much rather like adding this to the netlink interface (which
> would, of course, require also kernel side implementation).
>
> Michal