[PATCH ethtool v2 0/3] Add copybreak support

[PATCH ethtool v2 0/3] Add copybreak support

[Govindarajulu Varadarajan]

This series add support for setting/getting driver's copybreak value.
copybreak is set/get using ethtool tunable interface.

This was added to net-next in
commit: f0db9b073415848709dd59a6394969882f517da9

	ethtool: Add generic options for tunables

v2:
Add support for tx_copybreak
Add 'ethtool -B eth0 rx 100 tx 256' instead of 'ethtool -B eth0 100'

Govindarajulu Varadarajan (3):
  ethtool-copy.h: Sync with net-next 3.17.0-rc7
  ethtool: Add copybreak support
  ethtool.8.in: Add man page for copybreak

 ethtool-copy.h |  29 ++++++++++
 ethtool.8.in   |  20 +++++++
 ethtool.c      | 177 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 226 insertions(+)

-- 
2.1.0

[PATCH ethtool v2 1/3] ethtool-copy.h: Sync with net-next 3.17.0-rc7

[Govindarajulu Varadarajan]

This covers kernel changes up to:

commit:	1255a5055449781a92076fc5429952f2b33cf309
Author:	Eric Dumazet <edumazet@google.com>
Date:	Sun Oct 5 12:35:21 2014 +0300

	ethtool: Ethtool parameter to dynamically change tx_copybreak

Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
 ethtool-copy.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 61b78fc..3c89379 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -209,6 +209,33 @@ struct ethtool_value {
 	__u32	data;
 };
 
+enum tunable_id {
+	ETHTOOL_ID_UNSPEC,
+	ETHTOOL_RX_COPYBREAK,
+	ETHTOOL_TX_COPYBREAK,
+};
+
+enum tunable_type_id {
+	ETHTOOL_TUNABLE_UNSPEC,
+	ETHTOOL_TUNABLE_U8,
+	ETHTOOL_TUNABLE_U16,
+	ETHTOOL_TUNABLE_U32,
+	ETHTOOL_TUNABLE_U64,
+	ETHTOOL_TUNABLE_STRING,
+	ETHTOOL_TUNABLE_S8,
+	ETHTOOL_TUNABLE_S16,
+	ETHTOOL_TUNABLE_S32,
+	ETHTOOL_TUNABLE_S64,
+};
+
+struct ethtool_tunable {
+	__u32	cmd;
+	__u32	id;
+	__u32	type_id;
+	__u32	len;
+	void	*data[0];
+};
+
 /**
  * struct ethtool_regs - hardware register dump
  * @cmd: Command number = %ETHTOOL_GREGS
@@ -1152,6 +1179,8 @@ enum ethtool_sfeatures_retval_bits {
 
 #define ETHTOOL_GRSSH		0x00000046 /* Get RX flow hash configuration */
 #define ETHTOOL_SRSSH		0x00000047 /* Set RX flow hash configuration */
+#define ETHTOOL_GTUNABLE	0x00000048 /* Get tunable configuration */
+#define ETHTOOL_STUNABLE	0x00000049 /* Set tunable configuration */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
-- 
2.1.0

[PATCH ethtool v2 2/3] ethtool: Add copybreak support

[Govindarajulu Varadarajan]

This patch adds support for setting/getting driver's rx_copybreak value.
copybreak is set/get using new ethtool tunable interface.

This was added to net-next in
commit: f0db9b073415848709dd59a6394969882f517da9

	ethtool: Add generic options for tunables

Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
 ethtool.c | 177 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 177 insertions(+)

diff --git a/ethtool.c b/ethtool.c
index bf583f3..4045356 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -179,6 +179,12 @@ static const struct flag_info flags_msglvl[] = {
 	{ "wol",	NETIF_MSG_WOL },
 };
 
+static const char *tunable_name[] = {
+	[ETHTOOL_ID_UNSPEC]	= "Unspec",
+	[ETHTOOL_RX_COPYBREAK]	= "rx",
+	[ETHTOOL_TX_COPYBREAK]	= "tx",
+};
+
 struct off_flag_def {
 	const char *short_name;
 	const char *long_name;
@@ -1805,6 +1811,173 @@ static int do_gring(struct cmd_context *ctx)
 	return 0;
 }
 
+static int get_u32tunable(struct cmd_context *ctx, enum tunable_id id,
+			  __u32 *value)
+{
+	struct ethtool_tunable *etuna;
+	int ret;
+
+	etuna = calloc(sizeof(*etuna) + sizeof(__u32), 1);
+	if (!etuna)
+		return 1;
+	etuna->cmd = ETHTOOL_GTUNABLE;
+	etuna->id = id;
+	etuna->type_id = ETHTOOL_TUNABLE_U32;
+	etuna->len = sizeof(__u32);
+	ret = send_ioctl(ctx, etuna);
+	*value = *(__u32 *)((void *)etuna + sizeof(*etuna));
+	free(etuna);
+
+	return ret;
+}
+
+static int print_u32tunable(int err, enum tunable_id id, const __u32 value)
+{
+	if (err) {
+		switch (errno) {
+		/* Driver does not support this particular tunable
+		 * Usually displays 0
+		 */
+		case EINVAL:
+			goto print;
+		/* Driver does not support get tunables ops or no such device
+		 * No point in proceeding further
+		 */
+		case EOPNOTSUPP:
+		case ENODEV:
+			perror("Cannot get device settings");
+			exit(err);
+		default:
+			perror(tunable_name[id]);
+			return err;
+		}
+	}
+print:
+	fprintf(stdout, "%s: %u\n", tunable_name[id], value);
+
+	return 0;
+}
+
+static int do_gcopybreak(struct cmd_context *ctx)
+{
+	int err, anyerror = 0;
+	__u32 u32value;
+
+	if (ctx->argc != 0)
+		exit_bad_args();
+
+	fprintf(stdout, "Copybreak settings for device %s\n", ctx->devname);
+
+	err = get_u32tunable(ctx, ETHTOOL_RX_COPYBREAK, &u32value);
+	err = print_u32tunable(err, ETHTOOL_RX_COPYBREAK, u32value);
+	if (err)
+		anyerror = err;
+
+	err = get_u32tunable(ctx, ETHTOOL_TX_COPYBREAK, &u32value);
+	err = print_u32tunable(err, ETHTOOL_TX_COPYBREAK, u32value);
+	if (err)
+		anyerror = err;
+
+	if (anyerror)
+		fprintf(stderr, "Failed to get all settings. displayed partial settings\n");
+
+	return anyerror;
+}
+
+static int set_u32tunable(struct cmd_context *ctx, enum tunable_id id,
+			  const __u32 value)
+{
+	struct ethtool_tunable *etuna;
+	int ret;
+	__u32 *data;
+
+	etuna = malloc(sizeof(*etuna) + sizeof(__u32));
+	if (!etuna) {
+		perror(tunable_name[id]);
+		return 1;
+	}
+	data = (void *)etuna + sizeof(*etuna);
+	*data = value;
+	etuna->cmd = ETHTOOL_STUNABLE;
+	etuna->id = id;
+	etuna->type_id = ETHTOOL_TUNABLE_U32;
+	etuna->len = sizeof(__u32);
+	ret = send_ioctl(ctx, etuna);
+	free(etuna);
+
+	return ret;
+}
+
+static int check_set_u32tunable(int err, enum tunable_id id)
+{
+	if (err) {
+		switch (errno) {
+		/* Driver does not support get tunables ops or no such device
+		 * No point in proceeding further
+		 */
+		case EOPNOTSUPP:
+		case ENODEV:
+			perror("Cannot set device settings");
+			exit(err);
+		default:
+			perror(tunable_name[id]);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int do_scopybreak(struct cmd_context *ctx)
+{
+	int err, anyerr = 0;
+	int copybreak_changed = 0;
+	__u32 rx, tx;
+	s32 rx_seen = 0;
+	s32 tx_seen = 0;
+
+	struct cmdline_info cmdline_channels[] = {
+		{ .name = "rx",
+		  .type = CMDL_U32,
+		  .wanted_val = &rx,
+		  .seen_val = &rx_seen, },
+
+		{ .name = "tx",
+		  .type = CMDL_U32,
+		  .wanted_val = &tx,
+		  .seen_val = &tx_seen, },
+	};
+
+	parse_generic_cmdline(ctx, &copybreak_changed, cmdline_channels,
+			      ARRAY_SIZE(cmdline_channels));
+
+	if (!copybreak_changed) {
+		fprintf(stderr, "no copybreak parameters changed\n");
+		return 0;
+	}
+
+	if (rx_seen) {
+		err = set_u32tunable(ctx, ETHTOOL_RX_COPYBREAK, rx);
+		err = check_set_u32tunable(err, ETHTOOL_RX_COPYBREAK);
+		if (err)
+			anyerr = err;
+	}
+
+	if (tx_seen) {
+		err = set_u32tunable(ctx, ETHTOOL_TX_COPYBREAK, tx);
+		err = check_set_u32tunable(err, ETHTOOL_TX_COPYBREAK);
+		if (err)
+			anyerr = err;
+	}
+
+	if (anyerr) {
+		fprintf(stderr, "Failed to set all requested parameters\n");
+		return anyerr;
+	}
+
+	return 0;
+}
+
 static int do_schannels(struct cmd_context *ctx)
 {
 	struct ethtool_channels echannels;
@@ -4055,6 +4228,10 @@ static const struct option {
 	  "		[ rx-mini N ]\n"
 	  "		[ rx-jumbo N ]\n"
 	  "		[ tx N ]\n" },
+	{ "-b|--show-copybreak", 1, do_gcopybreak, "Show copybreak values" },
+	{ "-B|--set-copybreak", 1, do_scopybreak, "Set copybreak values",
+	  "		[ rx N]\n"
+	  "		[ tx N]\n" },
 	{ "-k|--show-features|--show-offload", 1, do_gfeatures,
 	  "Get state of protocol offload and other features" },
 	{ "-K|--features|--offload", 1, do_sfeatures,
-- 
2.1.0

[PATCH ethtool v2 3/3] ethtool.8.in: Add man page for copybreak

[Govindarajulu Varadarajan]

Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
 ethtool.8.in | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/ethtool.8.in b/ethtool.8.in
index ae56293..9f0955f 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -176,6 +176,14 @@ ethtool \- query or control network driver and hardware settings
 .BN rx\-jumbo
 .BN tx
 .HP
+.B ethtool \-b|\-\-show\-copybreak
+.I devname
+.HP
+.B ethtool \-B|\-\-set\-copybreak
+.I devname
+.BN rx
+.BN tx
+.HP
 .B ethtool \-i|\-\-driver
 .I devname
 .HP
@@ -399,6 +407,18 @@ Changes the number of ring entries for the Rx Jumbo ring.
 .BI tx \ N
 Changes the number of ring entries for the Tx ring.
 .TP
+.B \-b|\-\-show\-copybreak
+Get device copybreak values
+.TP
+.B \-B|\-\-set\-copybreak
+Set device copybreak values
+.TP
+.BI rx \ N
+Change rx_copybreak
+.TP
+.BI tx \ N
+Change tx_copybreak
+.TP
 .B \-i \-\-driver
 Queries the specified network device for associated driver information.
 .TP
-- 
2.1.0

Re: [PATCH ethtool v2 2/3] ethtool: Add copybreak support

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1987 bytes --]

On Tue, 2014-10-07 at 04:42 +0530, Govindarajulu Varadarajan wrote:
> This patch adds support for setting/getting driver's rx_copybreak value.
> copybreak is set/get using new ethtool tunable interface.
> 
> This was added to net-next in
> commit: f0db9b073415848709dd59a6394969882f517da9
> 
> 	ethtool: Add generic options for tunables
> 
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
> ---
>  ethtool.c | 177 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 177 insertions(+)
> 
> diff --git a/ethtool.c b/ethtool.c
> index bf583f3..4045356 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -179,6 +179,12 @@ static const struct flag_info flags_msglvl[] = {
>  	{ "wol",	NETIF_MSG_WOL },
>  };
>  
> +static const char *tunable_name[] = {
> +	[ETHTOOL_ID_UNSPEC]	= "Unspec",
> +	[ETHTOOL_RX_COPYBREAK]	= "rx",
> +	[ETHTOOL_TX_COPYBREAK]	= "tx",
> +};

Tunables should be named by a string set defined in the kernel.

[...]
> @@ -4055,6 +4228,10 @@ static const struct option {
>           "             [ rx-mini N ]\n"
>           "             [ rx-jumbo N ]\n"
>           "             [ tx N ]\n" },
> +       { "-b|--show-copybreak", 1, do_gcopybreak, "Show copybreak values" },
> +       { "-B|--set-copybreak", 1, do_scopybreak, "Set copybreak values",
> +         "             [ rx N]\n"
> +         "             [ tx N]\n" },
>         { "-k|--show-features|--show-offload", 1, do_gfeatures,
>           "Get state of protocol offload and other features" },
>         { "-K|--features|--offload", 1, do_sfeatures,
[...]

T don't think this is worth two options of its own.  You should be able
to add generic get/set-tunable optins.  You'll need to get the string
set to find out the names of tunables.  When setting a tunable, you'll
need to get it first to find out its type.

Ben.

-- 
Ben Hutchings
The two most common things in the universe are hydrogen and stupidity.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH ethtool v2 2/3] ethtool: Add copybreak support

[Hadar Hen Zion]

On Sun, Dec 14, 2014 at 8:46 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Tue, 2014-10-07 at 04:42 +0530, Govindarajulu Varadarajan wrote:
>> This patch adds support for setting/getting driver's rx_copybreak value.
>> copybreak is set/get using new ethtool tunable interface.
>>
>> This was added to net-next in
>> commit: f0db9b073415848709dd59a6394969882f517da9
>>
>>       ethtool: Add generic options for tunables
>>
>> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
>> ---
>>  ethtool.c | 177 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 177 insertions(+)
>>
>> diff --git a/ethtool.c b/ethtool.c
>> index bf583f3..4045356 100644
>> --- a/ethtool.c
>> +++ b/ethtool.c
>> @@ -179,6 +179,12 @@ static const struct flag_info flags_msglvl[] = {
>>       { "wol",        NETIF_MSG_WOL },
>>  };
>>
>> +static const char *tunable_name[] = {
>> +     [ETHTOOL_ID_UNSPEC]     = "Unspec",
>> +     [ETHTOOL_RX_COPYBREAK]  = "rx",
>> +     [ETHTOOL_TX_COPYBREAK]  = "tx",
>> +};
>
> Tunables should be named by a string set defined in the kernel.
>
> [...]
>> @@ -4055,6 +4228,10 @@ static const struct option {
>>           "             [ rx-mini N ]\n"
>>           "             [ rx-jumbo N ]\n"
>>           "             [ tx N ]\n" },
>> +       { "-b|--show-copybreak", 1, do_gcopybreak, "Show copybreak values" },
>> +       { "-B|--set-copybreak", 1, do_scopybreak, "Set copybreak values",
>> +         "             [ rx N]\n"
>> +         "             [ tx N]\n" },
>>         { "-k|--show-features|--show-offload", 1, do_gfeatures,
>>           "Get state of protocol offload and other features" },
>>         { "-K|--features|--offload", 1, do_sfeatures,
> [...]
>
> T don't think this is worth two options of its own.  You should be able
> to add generic get/set-tunable optins.  You'll need to get the string
> set to find out the names of tunables.  When setting a tunable, you'll
> need to get it first to find out its type.
>
> Ben.
>
> --
> Ben Hutchings
> The two most common things in the universe are hydrogen and stupidity.

Hi Govindarajulu,

Do you have a patch that fixes Ben's comments?

I would like to add the copybreak feature so let me know if you want
me to fix the patch according to Ben's comments and resend it.

Thanks,
Hadar

Re: [PATCH ethtool v2 2/3] ethtool: Add copybreak support

[Govindarajulu Varadarajan]

On Wed, 13 May 2015, Hadar Hen Zion wrote:
> Hi Govindarajulu,
>
> Do you have a patch that fixes Ben's comments?
>
> I would like to add the copybreak feature so let me know if you want
> me to fix the patch according to Ben's comments and resend it.
>

Hi Hadar

I haven't looked at this for a while. Please go ahead.

Thanks