| Message ID | 20200901212009.1314401-1-yyd@google.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | [ethtool] ethtool: add support show/set-hwtstamp | expand |
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
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
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
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
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,
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(+)