| Message ID | 20200608175255.3353-1-gvaradar@cisco.com |
|---|---|
| State | Deferred |
| Delegated to: | John Linville |
| Headers | show |
| Series | [ethtool,1/2] ethtool: add support for get/set ethtool_tunable | expand |
I'm sorry it took me so long to get to this patch; I wanted to review it two weeks ago but we had problems with e-mail migration and it took me some time to recover missing mails. On Mon, Jun 08, 2020 at 10:52:54AM -0700, Govindarajulu Varadarajan wrote: > Add support for ETHTOOL_GTUNABLE and ETHTOOL_STUNABLE options. > > Tested rx-copybreak on enic driver. Tested ETHTOOL_TUNNABLE_STRING TUNABLE > options with test/debug changes in kernel. This makes me wonder how are string tunables supposed to work. Unfortunately there is neither documentation nor code one could look at. I tried to understand it from this patch but it didn't help much either: do_stunable() will pass a string of arbitrary size to kernel but do_gtunable() allocates a buffer of fixed size (for a given tunable). Is this supposed to be the maximum value length? Or is kernel going to return an error if the buffer is insufficient and userspace repeats the request? > Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com> > --- > ethtool.c | 227 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 227 insertions(+) > > diff --git a/ethtool.c b/ethtool.c > index 900880a..6cff5dd 100644 > --- a/ethtool.c > +++ b/ethtool.c > @@ -4731,6 +4731,217 @@ static int do_seee(struct cmd_context *ctx) > return 0; > } > > +/* copy of net/ethtool/common.c */ > +char > +tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN] = { > + [ETHTOOL_ID_UNSPEC] = "Unspec", > + [ETHTOOL_RX_COPYBREAK] = "rx-copybreak", > + [ETHTOOL_TX_COPYBREAK] = "tx-copybreak", > + [ETHTOOL_PFC_PREVENTION_TOUT] = "pfc-prevention-tout", > +}; Please align the right sides of the initializations. > + > +struct ethtool_tunable_info { > + enum tunable_id t_id; > + enum tunable_type_id t_type_id; > + size_t size; > + cmdline_type_t type; > + union val { > + u8 u8; > + u16 u16; > + u32 u32; > + u64 u64; > + int8_t s8; > + int16_t s16; > + int32_t s32; > + int64_t s64; > + void *str; > + } seen; > + union val wanted; > +}; As the union type is used twice, it would make more sense to define it outside of the struct declaration. However, seen is always used as int so it's not clear why you declare it as the union. Also, it's inconsistent to use kernel integer types for some tunable types and standard C90 types for the others. > +struct ethtool_tunable_info tinfo[] = { Given the length of ethtool.c and amount of code for different subcommands in it, please use more specific name than "tinfo", rather something like "tunables_info". Also, it should be static. > + { .t_id = ETHTOOL_RX_COPYBREAK, > + .t_type_id = ETHTOOL_TUNABLE_U32, > + .size = sizeof(u32), > + .type = CMDL_U32, > + }, > + { .t_id = ETHTOOL_TX_COPYBREAK, > + .t_type_id = ETHTOOL_TUNABLE_U32, > + .size = sizeof(u32), > + .type = CMDL_U32, > + }, > + { .t_id = ETHTOOL_PFC_PREVENTION_TOUT, > + .t_type_id = ETHTOOL_TUNABLE_U16, > + .size = sizeof(u16), > + .type = CMDL_U16, > + }, > +}; > +#define TINFO_SIZE ARRAY_SIZE(tinfo) > + > +static int do_stunable(struct cmd_context *ctx) > +{ > + struct cmdline_info cmdline_tunable[TINFO_SIZE]; > + int changed = 0; > + int i; > + > + for (i = 0; i < TINFO_SIZE; i++) { > + cmdline_tunable[i].name = tunable_strings[tinfo[i].t_id]; > + cmdline_tunable[i].type = tinfo[i].type; > + cmdline_tunable[i].wanted_val = &tinfo[i].wanted; > + cmdline_tunable[i].seen_val = &tinfo[i].seen; > + } > + > + parse_generic_cmdline(ctx, &changed, cmdline_tunable, TINFO_SIZE); > + if (!changed) > + exit_bad_args(); > + > + for (i = 0; i < TINFO_SIZE; i++) { > + char **val = (char **)&tinfo[i].wanted; > + struct ethtool_tunable *tuna; > + size_t size; > + int *seen; > + int ret; > + > + seen = (int *)(&tinfo[i].seen); > + if (!*seen) > + continue; > + > + size = sizeof(*tuna); > + if (tinfo[i].type == CMDL_STR) { > + size += strlen(*val) + 1; > + } else { > + size += tinfo[i].size; > + } The curly braces are not needed here. > + tuna = calloc(1, size); > + if (!tuna) { > + perror(tunable_strings[tinfo[i].t_id]); > + return 1; > + } > + tuna->cmd = ETHTOOL_STUNABLE; > + tuna->id = tinfo[i].t_id; > + tuna->type_id = tinfo[i].t_type_id; > + if (tinfo[i].type == CMDL_STR) { > + tuna->len = strlen(*val) + 1; > + memcpy(tuna->data, *val, strlen(*val) + 1); > + } else { > + tuna->len = tinfo[i].size; > + memcpy(tuna->data, &tinfo[i].wanted, tuna->len); > + } > + ret = send_ioctl(ctx, tuna); > + if (ret) { > + perror(tunable_strings[tuna->id]); > + return ret; > + } > + free(tuna); > + tuna = NULL; This seems pointless, variable tuna is not (guaranteed to be) preserved between iterations and even if it were, there would be no need to zero it here as if the value did matter, it would be uninitialized on first iteration. > + /* reset seen and wanted to 0 */ > + memset(&tinfo[i].seen, 0, sizeof(tinfo[i].seen)); > + memset(&tinfo[i].wanted, 0, sizeof(tinfo[i].seen)); Comments should not say what you are doing but why. In this case, I do not see why do you need to clear these as you are not going to use either of them again. > + } > + return 0; > +} > + > +static void print_tunable(struct ethtool_tunable *tuna) > +{ > + char *name = tunable_strings[tuna->id]; > + u8 *val_u8; > + u16 *val_u16; > + u32 *val_u32; > + u64 *val_u64; > + int8_t *val_s8; > + int16_t *val_s16; > + int32_t *val_s32; > + long long int *val_s64; > + char *val_str; Why don't you reuse the union from struct ethtool_tunable_info? You could then replace all the val_* assignments with const union tunable_val *val = (const union tunable_val *)tuna->data; and use union members in (f)printf() calls below. > + > + switch (tuna->type_id) { > + case ETHTOOL_TUNABLE_U8: > + val_u8 = (u8 *)tuna->data; > + fprintf(stdout, "%s: %u\n", name, *val_u8); > + break; > + case ETHTOOL_TUNABLE_U16: > + val_u16 = (u16 *)tuna->data; > + fprintf(stdout, "%s: %u\n", name, *val_u16); > + break; > + case ETHTOOL_TUNABLE_U32: > + val_u32 = (u32 *)tuna->data; > + fprintf(stdout, "%s: %u\n", name, *val_u32); > + break; > + case ETHTOOL_TUNABLE_U64: > + val_u64 = (u64 *)tuna->data; > + fprintf(stdout, "%s: %llu\n", name, *val_u64); > + break; > + case ETHTOOL_TUNABLE_S8: > + val_s8 = (int8_t *)tuna->data; > + fprintf(stdout, "%s: %d\n", name, *val_s8); > + break; > + case ETHTOOL_TUNABLE_S16: > + val_s16 = (int16_t *)tuna->data; > + fprintf(stdout, "%s: %d\n", name, *val_s16); > + break; > + case ETHTOOL_TUNABLE_S32: > + val_s32 = (int32_t *)tuna->data; > + fprintf(stdout, "%s: %d\n", name, *val_s32); > + break; > + case ETHTOOL_TUNABLE_S64: > + val_s64 = (long long int *)tuna->data; > + fprintf(stdout, "%s: %lld\n", name, *val_s64); > + break; > + case ETHTOOL_TUNABLE_STRING: > + val_str = (char *)tuna->data; > + fprintf(stdout, "%s: %s\n", name, val_str); > + break; > + default: > + fprintf(stdout, "%s: Unknown format\n", name); > + } > +} > + > +static int do_gtunable(struct cmd_context *ctx) > +{ > + char **argp = ctx->argp; > + int argc = ctx->argc; > + int i; > + int j; > + > + if (argc < 1) > + exit_bad_args(); > + > + for (i = 0; i < argc; i++) { > + int valid = 0; > + > + for (j = 0; j < TINFO_SIZE; j++) { > + char *ts = tunable_strings[tinfo[j].t_id]; > + struct ethtool_tunable *tuna; > + int ret; > + > + if (strcmp(argp[i], ts)) > + continue; > + valid = 1; > + > + tuna = calloc(1, sizeof(*tuna) + tinfo[j].size); > + if (!tuna) { > + perror(ts); > + return 1; > + } > + tuna->cmd = ETHTOOL_GTUNABLE; > + tuna->id = tinfo[j].t_id; > + tuna->type_id = tinfo[j].t_type_id; > + tuna->len = tinfo[j].size; > + ret = send_ioctl(ctx, tuna); > + if (ret) { > + fprintf(stderr, "%s: Cannot get tunable\n", ts); > + return ret; > + } > + print_tunable(tuna); > + free(tuna); > + tuna = NULL; As in do_stunable(), no need to reset tuna here. > + } > + if (!valid) > + exit_bad_args(); > + } > + return 0; > +} > + > static int do_get_phy_tunable(struct cmd_context *ctx) > { > int argc = ctx->argc; > @@ -5468,6 +5679,22 @@ static const struct option args[] = { > " [ fast-link-down ]\n" > " [ energy-detect-power-down ]\n" > }, > + { > + .opts = "-b|--get-tunable", > + .func = do_gtunable, > + .help = "Get tunable", > + .xhelp = " [ rx-copybreak ]\n" > + " [ tx-copybreak ]\n" > + " [ pfc-precention-tout ]\n" > + }, > + { > + .opts = "-B|--set-tunable", > + .func = do_stunable, > + .help = "Set tunable", > + .xhelp = " [ rx-copybreak N]\n" > + " [ tx-copybreak N]\n" > + " [ pfc-precention-tout N]\n" > + }, > { > .opts = "--reset", > .func = do_reset, I don't think tunables are so basic that they would need one-letter command line options. Actually, I'm not sure if we even need to add more tunables when we can add netlink attributes to any request type. Michal > -- > 2.27.0 >
On Sun, 2020-07-05 at 00:50 +0200, Michal Kubecek wrote: > On Mon, Jun 08, 2020 at 10:52:54AM -0700, Govindarajulu Varadarajan wrote: > > Add support for ETHTOOL_GTUNABLE and ETHTOOL_STUNABLE options. > > > > Tested rx-copybreak on enic driver. Tested ETHTOOL_TUNNABLE_STRING > TUNABLE > > options with test/debug changes in kernel. > > This makes me wonder how are string tunables supposed to work. > Unfortunately there is neither documentation nor code one could look at. > I tried to understand it from this patch but it didn't help much either: > do_stunable() will pass a string of arbitrary size to kernel but > do_gtunable() allocates a buffer of fixed size (for a given tunable). > Is this supposed to be the maximum value length? Or is kernel going to > return an error if the buffer is insufficient and userspace repeats the > request? I do not know. string tunable isn't implemented/supported in kernel. I assumed driver's get/set_tunable() returns error in case insufficient size. Addressing the rest of the comments and sending v2. -- Govind
diff --git a/ethtool.c b/ethtool.c index 900880a..6cff5dd 100644 --- a/ethtool.c +++ b/ethtool.c @@ -4731,6 +4731,217 @@ static int do_seee(struct cmd_context *ctx) return 0; } +/* copy of net/ethtool/common.c */ +char +tunable_strings[__ETHTOOL_TUNABLE_COUNT][ETH_GSTRING_LEN] = { + [ETHTOOL_ID_UNSPEC] = "Unspec", + [ETHTOOL_RX_COPYBREAK] = "rx-copybreak", + [ETHTOOL_TX_COPYBREAK] = "tx-copybreak", + [ETHTOOL_PFC_PREVENTION_TOUT] = "pfc-prevention-tout", +}; + +struct ethtool_tunable_info { + enum tunable_id t_id; + enum tunable_type_id t_type_id; + size_t size; + cmdline_type_t type; + union val { + u8 u8; + u16 u16; + u32 u32; + u64 u64; + int8_t s8; + int16_t s16; + int32_t s32; + int64_t s64; + void *str; + } seen; + union val wanted; +}; + +struct ethtool_tunable_info tinfo[] = { + { .t_id = ETHTOOL_RX_COPYBREAK, + .t_type_id = ETHTOOL_TUNABLE_U32, + .size = sizeof(u32), + .type = CMDL_U32, + }, + { .t_id = ETHTOOL_TX_COPYBREAK, + .t_type_id = ETHTOOL_TUNABLE_U32, + .size = sizeof(u32), + .type = CMDL_U32, + }, + { .t_id = ETHTOOL_PFC_PREVENTION_TOUT, + .t_type_id = ETHTOOL_TUNABLE_U16, + .size = sizeof(u16), + .type = CMDL_U16, + }, +}; +#define TINFO_SIZE ARRAY_SIZE(tinfo) + +static int do_stunable(struct cmd_context *ctx) +{ + struct cmdline_info cmdline_tunable[TINFO_SIZE]; + int changed = 0; + int i; + + for (i = 0; i < TINFO_SIZE; i++) { + cmdline_tunable[i].name = tunable_strings[tinfo[i].t_id]; + cmdline_tunable[i].type = tinfo[i].type; + cmdline_tunable[i].wanted_val = &tinfo[i].wanted; + cmdline_tunable[i].seen_val = &tinfo[i].seen; + } + + parse_generic_cmdline(ctx, &changed, cmdline_tunable, TINFO_SIZE); + if (!changed) + exit_bad_args(); + + for (i = 0; i < TINFO_SIZE; i++) { + char **val = (char **)&tinfo[i].wanted; + struct ethtool_tunable *tuna; + size_t size; + int *seen; + int ret; + + seen = (int *)(&tinfo[i].seen); + if (!*seen) + continue; + + size = sizeof(*tuna); + if (tinfo[i].type == CMDL_STR) { + size += strlen(*val) + 1; + } else { + size += tinfo[i].size; + } + tuna = calloc(1, size); + if (!tuna) { + perror(tunable_strings[tinfo[i].t_id]); + return 1; + } + tuna->cmd = ETHTOOL_STUNABLE; + tuna->id = tinfo[i].t_id; + tuna->type_id = tinfo[i].t_type_id; + if (tinfo[i].type == CMDL_STR) { + tuna->len = strlen(*val) + 1; + memcpy(tuna->data, *val, strlen(*val) + 1); + } else { + tuna->len = tinfo[i].size; + memcpy(tuna->data, &tinfo[i].wanted, tuna->len); + } + ret = send_ioctl(ctx, tuna); + if (ret) { + perror(tunable_strings[tuna->id]); + return ret; + } + free(tuna); + tuna = NULL; + /* reset seen and wanted to 0 */ + memset(&tinfo[i].seen, 0, sizeof(tinfo[i].seen)); + memset(&tinfo[i].wanted, 0, sizeof(tinfo[i].seen)); + } + return 0; +} + +static void print_tunable(struct ethtool_tunable *tuna) +{ + char *name = tunable_strings[tuna->id]; + u8 *val_u8; + u16 *val_u16; + u32 *val_u32; + u64 *val_u64; + int8_t *val_s8; + int16_t *val_s16; + int32_t *val_s32; + long long int *val_s64; + char *val_str; + + switch (tuna->type_id) { + case ETHTOOL_TUNABLE_U8: + val_u8 = (u8 *)tuna->data; + fprintf(stdout, "%s: %u\n", name, *val_u8); + break; + case ETHTOOL_TUNABLE_U16: + val_u16 = (u16 *)tuna->data; + fprintf(stdout, "%s: %u\n", name, *val_u16); + break; + case ETHTOOL_TUNABLE_U32: + val_u32 = (u32 *)tuna->data; + fprintf(stdout, "%s: %u\n", name, *val_u32); + break; + case ETHTOOL_TUNABLE_U64: + val_u64 = (u64 *)tuna->data; + fprintf(stdout, "%s: %llu\n", name, *val_u64); + break; + case ETHTOOL_TUNABLE_S8: + val_s8 = (int8_t *)tuna->data; + fprintf(stdout, "%s: %d\n", name, *val_s8); + break; + case ETHTOOL_TUNABLE_S16: + val_s16 = (int16_t *)tuna->data; + fprintf(stdout, "%s: %d\n", name, *val_s16); + break; + case ETHTOOL_TUNABLE_S32: + val_s32 = (int32_t *)tuna->data; + fprintf(stdout, "%s: %d\n", name, *val_s32); + break; + case ETHTOOL_TUNABLE_S64: + val_s64 = (long long int *)tuna->data; + fprintf(stdout, "%s: %lld\n", name, *val_s64); + break; + case ETHTOOL_TUNABLE_STRING: + val_str = (char *)tuna->data; + fprintf(stdout, "%s: %s\n", name, val_str); + break; + default: + fprintf(stdout, "%s: Unknown format\n", name); + } +} + +static int do_gtunable(struct cmd_context *ctx) +{ + char **argp = ctx->argp; + int argc = ctx->argc; + int i; + int j; + + if (argc < 1) + exit_bad_args(); + + for (i = 0; i < argc; i++) { + int valid = 0; + + for (j = 0; j < TINFO_SIZE; j++) { + char *ts = tunable_strings[tinfo[j].t_id]; + struct ethtool_tunable *tuna; + int ret; + + if (strcmp(argp[i], ts)) + continue; + valid = 1; + + tuna = calloc(1, sizeof(*tuna) + tinfo[j].size); + if (!tuna) { + perror(ts); + return 1; + } + tuna->cmd = ETHTOOL_GTUNABLE; + tuna->id = tinfo[j].t_id; + tuna->type_id = tinfo[j].t_type_id; + tuna->len = tinfo[j].size; + ret = send_ioctl(ctx, tuna); + if (ret) { + fprintf(stderr, "%s: Cannot get tunable\n", ts); + return ret; + } + print_tunable(tuna); + free(tuna); + tuna = NULL; + } + if (!valid) + exit_bad_args(); + } + return 0; +} + static int do_get_phy_tunable(struct cmd_context *ctx) { int argc = ctx->argc; @@ -5468,6 +5679,22 @@ static const struct option args[] = { " [ fast-link-down ]\n" " [ energy-detect-power-down ]\n" }, + { + .opts = "-b|--get-tunable", + .func = do_gtunable, + .help = "Get tunable", + .xhelp = " [ rx-copybreak ]\n" + " [ tx-copybreak ]\n" + " [ pfc-precention-tout ]\n" + }, + { + .opts = "-B|--set-tunable", + .func = do_stunable, + .help = "Set tunable", + .xhelp = " [ rx-copybreak N]\n" + " [ tx-copybreak N]\n" + " [ pfc-precention-tout N]\n" + }, { .opts = "--reset", .func = do_reset,
Add support for ETHTOOL_GTUNABLE and ETHTOOL_STUNABLE options. Tested rx-copybreak on enic driver. Tested ETHTOOL_TUNNABLE_STRING options with test/debug changes in kernel. Signed-off-by: Govindarajulu Varadarajan <gvaradar@cisco.com> --- ethtool.c | 227 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 227 insertions(+)