[PATCH ethtool 1/6] ethtool: fix uninitialized return value

[PATCH ethtool 1/6] ethtool: fix uninitialized return value

[Ivan Vecera]

Fixes: b0fe96d ("Ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE and PHY downshift")
Signed-off-by: Ivan Vecera <cera@cera.cz>
---
 ethtool.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 2e87384..e7495fe 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4723,8 +4723,8 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
 {
 	int argc = ctx->argc;
 	char **argp = ctx->argp;
-	int err, i;
 	u8 downshift_changed = 0;
+	int i;
 
 	if (argc < 1)
 		exit_bad_args();
@@ -4750,8 +4750,7 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
 		cont.ds.id = ETHTOOL_PHY_DOWNSHIFT;
 		cont.ds.type_id = ETHTOOL_TUNABLE_U8;
 		cont.ds.len = 1;
-		err = send_ioctl(ctx, &cont.ds);
-		if (err < 0) {
+		if (send_ioctl(ctx, &cont.ds) < 0) {
 			perror("Cannot Get PHY downshift count");
 			return 87;
 		}
@@ -4762,7 +4761,7 @@ static int do_get_phy_tunable(struct cmd_context *ctx)
 			fprintf(stdout, "Downshift disabled\n");
 	}
 
-	return err;
+	return 0;
 }
 
 static __u32 parse_reset(char *val, __u32 bitset, char *arg, __u32 *data)
-- 
2.16.4

[PATCH ethtool 2/6] ethtool: fix RING_VF assignment

[Ivan Vecera]

Fixes: 36ee712 ("ethtool: support queue and VF fields for rxclass filters")
Cc: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Ivan Vecera <cera@cera.cz>
---
 rxclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rxclass.c b/rxclass.c
index ce4b382..42d122d 100644
--- a/rxclass.c
+++ b/rxclass.c
@@ -1066,7 +1066,7 @@ static int rxclass_get_val(char *str, unsigned char *p, u32 *flags,
 		val++;
 
 		*(u64 *)&p[opt->offset] &= ~ETHTOOL_RX_FLOW_SPEC_RING_VF;
-		*(u64 *)&p[opt->offset] = (u64)val << ETHTOOL_RX_FLOW_SPEC_RING_VF_OFF;
+		*(u64 *)&p[opt->offset] |= (u64)val << ETHTOOL_RX_FLOW_SPEC_RING_VF_OFF;
 		break;
 	}
 	case OPT_RING_QUEUE: {
-- 
2.16.4

RE: [PATCH ethtool 2/6] ethtool: fix RING_VF assignment

[Keller, Jacob E]

> -----Original Message-----
> From: Ivan Vecera [mailto:cera@cera.cz]
> Sent: Friday, June 08, 2018 2:20 AM
> To: linville@tuxdriver.com
> Cc: netdev@vger.kernel.org; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [PATCH ethtool 2/6] ethtool: fix RING_VF assignment
> 
> Fixes: 36ee712 ("ethtool: support queue and VF fields for rxclass filters")
> Cc: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Ivan Vecera <cera@cera.cz>
> ---
>  rxclass.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/rxclass.c b/rxclass.c
> index ce4b382..42d122d 100644
> --- a/rxclass.c
> +++ b/rxclass.c
> @@ -1066,7 +1066,7 @@ static int rxclass_get_val(char *str, unsigned char *p,
> u32 *flags,
>  		val++;
> 
>  		*(u64 *)&p[opt->offset] &=
> ~ETHTOOL_RX_FLOW_SPEC_RING_VF;
> -		*(u64 *)&p[opt->offset] = (u64)val <<
> ETHTOOL_RX_FLOW_SPEC_RING_VF_OFF;
> +		*(u64 *)&p[opt->offset] |= (u64)val <<
> ETHTOOL_RX_FLOW_SPEC_RING_VF_OFF;
>  		break;
>  	}

Hah. Good catch.

Thanks,
Jake

>  	case OPT_RING_QUEUE: {
> --
> 2.16.4

[PATCH ethtool 3/6] ethtool: remove unused global variable

[Ivan Vecera]

Fixes: 2c2ee7a ("ethtool: Add support for sfc register dumps")
Signed-off-by: Ivan Vecera <cera@cera.cz>
---
 sfc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/sfc.c b/sfc.c
index 9478b38..b4c590f 100644
--- a/sfc.c
+++ b/sfc.c
@@ -3083,9 +3083,6 @@ static const struct efx_nic_reg_field efx_nic_reg_fields_TX_PACE[] = {
 	REGISTER_FIELD_BZ(TX_PACE_SB_AF),
 	REGISTER_FIELD_BZ(TX_PACE_SB_NOT_AF),
 };
-static const struct efx_nic_reg_field efx_nic_reg_fields_TX_PACE_DROP_QID[] = {
-	REGISTER_FIELD_BZ(TX_PACE_QID_DRP_CNT),
-};
 static const struct efx_nic_reg_field efx_nic_reg_fields_TX_VLAN[] = {
 	REGISTER_FIELD_BB(TX_VLAN0),
 	REGISTER_FIELD_BB(TX_VLAN0_PORT0_EN),
-- 
2.16.4

[PATCH ethtool 4/6] ethtool: several fixes in do_gregs()

[Ivan Vecera]

- correctly close gregs_dump_file in case of fstat() failure
- check for error from realloc

Fixes: be4c2d0 ("ethtool.c: fix dump_regs heap corruption")
Cc: David Decotigny <decot@googlers.com>
Signed-off-by: Ivan Vecera <cera@cera.cz>
---
 ethtool.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/ethtool.c b/ethtool.c
index e7495fe..2b90984 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -3179,17 +3179,26 @@ static int do_gregs(struct cmd_context *ctx)
 	if (!gregs_dump_raw && gregs_dump_file != NULL) {
 		/* overwrite reg values from file dump */
 		FILE *f = fopen(gregs_dump_file, "r");
+		struct ethtool_regs *nregs;
 		struct stat st;
 		size_t nread;
 
 		if (!f || fstat(fileno(f), &st) < 0) {
 			fprintf(stderr, "Can't open '%s': %s\n",
 				gregs_dump_file, strerror(errno));
+			if (f)
+				fclose(f);
 			free(regs);
 			return 75;
 		}
 
-		regs = realloc(regs, sizeof(*regs) + st.st_size);
+		nregs = realloc(regs, sizeof(*regs) + st.st_size);
+		if (!nregs) {
+			perror("Cannot allocate memory for register dump");
+			free(regs); /* was not freed by realloc */
+			return 73;
+		}
+		regs = nregs;
 		regs->len = st.st_size;
 		nread = fread(regs->data, regs->len, 1, f);
 		fclose(f);
-- 
2.16.4

[PATCH ethtool 5/6] ethtool: correctly free hkey when get_stringset() fails

[Ivan Vecera]

Memory allocated for 'hkey' is not freed when
get_stringset(..., ETH_SS_RSS_HASH_FUNCS...) fails.

Fixes: b888f35 ("ethtool: Support for configurable RSS hash function")
Cc: Gal Pressman <galp@mellanox.com>
Signed-off-by: Ivan Vecera <cera@cera.cz>
---
 ethtool.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 2b90984..fb93ae8 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -3910,7 +3910,7 @@ static int do_srxfhindir(struct cmd_context *ctx, int rxfhindir_default,
 static int do_srxfh(struct cmd_context *ctx)
 {
 	struct ethtool_rxfh rss_head = {0};
-	struct ethtool_rxfh *rss;
+	struct ethtool_rxfh *rss = NULL;
 	struct ethtool_rxnfc ring_count;
 	int rxfhindir_equal = 0, rxfhindir_default = 0;
 	struct ethtool_gstrings *hfuncs = NULL;
@@ -4064,7 +4064,8 @@ static int do_srxfh(struct cmd_context *ctx)
 		hfuncs = get_stringset(ctx, ETH_SS_RSS_HASH_FUNCS, 0, 1);
 		if (!hfuncs) {
 			perror("Cannot get hash functions names");
-			return 1;
+			err = 1;
+			goto free;
 		}
 
 		for (i = 0; i < hfuncs->len && !req_hfunc ; i++) {
@@ -4078,8 +4079,8 @@ static int do_srxfh(struct cmd_context *ctx)
 		if (!req_hfunc) {
 			fprintf(stderr,
 				"Unknown hash function: %s\n", req_hfunc_name);
-			free(hfuncs);
-			return 1;
+			err = 1;
+			goto free;
 		}
 	}
 
@@ -4120,9 +4121,7 @@ static int do_srxfh(struct cmd_context *ctx)
 	}
 
 free:
-	if (hkey)
-		free(hkey);
-
+	free(hkey);
 	free(rss);
 	free(hfuncs);
 	return err;
-- 
2.16.4

Re: [PATCH ethtool 5/6] ethtool: correctly free hkey when get_stringset() fails

[Gal Pressman]

On 08-Jun-18 12:20, Ivan Vecera wrote:
> Memory allocated for 'hkey' is not freed when
> get_stringset(..., ETH_SS_RSS_HASH_FUNCS...) fails.
> 
> Fixes: b888f35 ("ethtool: Support for configurable RSS hash function")

Thanks for fixing this!
Please use the first 12 characters of the sha1 in the Fixes line.

> Cc: Gal Pressman <galp@mellanox.com>
> Signed-off-by: Ivan Vecera <cera@cera.cz>
> ---
>  ethtool.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 2b90984..fb93ae8 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -3910,7 +3910,7 @@ static int do_srxfhindir(struct cmd_context *ctx, int rxfhindir_default,
>  static int do_srxfh(struct cmd_context *ctx)
>  {
>  	struct ethtool_rxfh rss_head = {0};
> -	struct ethtool_rxfh *rss;
> +	struct ethtool_rxfh *rss = NULL;
>  	struct ethtool_rxnfc ring_count;
>  	int rxfhindir_equal = 0, rxfhindir_default = 0;
>  	struct ethtool_gstrings *hfuncs = NULL;
> @@ -4064,7 +4064,8 @@ static int do_srxfh(struct cmd_context *ctx)
>  		hfuncs = get_stringset(ctx, ETH_SS_RSS_HASH_FUNCS, 0, 1);
>  		if (!hfuncs) {
>  			perror("Cannot get hash functions names");
> -			return 1;
> +			err = 1;
> +			goto free;
>  		}
>  
>  		for (i = 0; i < hfuncs->len && !req_hfunc ; i++) {
> @@ -4078,8 +4079,8 @@ static int do_srxfh(struct cmd_context *ctx)
>  		if (!req_hfunc) {
>  			fprintf(stderr,
>  				"Unknown hash function: %s\n", req_hfunc_name);
> -			free(hfuncs);
> -			return 1;
> +			err = 1;
> +			goto free;
>  		}
>  	}
>  
> @@ -4120,9 +4121,7 @@ static int do_srxfh(struct cmd_context *ctx)
>  	}
>  
>  free:
> -	if (hkey)
> -		free(hkey);
> -
> +	free(hkey);
>  	free(rss);
>  	free(hfuncs);
>  	return err;
> 

[PATCH ethtool 6/6] ethtool: remove unreachable code

[Ivan Vecera]

The default switch case is unreachable as the MAX_CHANNEL_NUM == 4.

Fixes: a5e73bb ("ethtool:QSFP Plus/QSFP28 Diagnostics Information Support")
Cc: Vidya Sagar Ravipati <vidya@cumulusnetworks.com>
Signed-off-by: Ivan Vecera <cera@cera.cz>
---
 qsfp.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/qsfp.c b/qsfp.c
index aecd5bb..32e195d 100644
--- a/qsfp.c
+++ b/qsfp.c
@@ -661,9 +661,6 @@ static void sff8636_dom_parse(const __u8 *id, struct sff_diags *sd)
 			tx_power_offset = SFF8636_TX_PWR_4_OFFSET;
 			tx_bias_offset = SFF8636_TX_BIAS_4_OFFSET;
 			break;
-		default:
-			printf(" Invalid channel: %d\n", i);
-			break;
 		}
 		sd->scd[i].bias_cur = OFFSET_TO_U16(tx_bias_offset);
 		sd->scd[i].rx_power = OFFSET_TO_U16(rx_power_offset);
-- 
2.16.4

Re: [PATCH ethtool 1/6] ethtool: fix uninitialized return value

[John W. Linville]

On Fri, Jun 08, 2018 at 11:20:05AM +0200, Ivan Vecera wrote:
> Fixes: b0fe96d ("Ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE and PHY downshift")
> Signed-off-by: Ivan Vecera <cera@cera.cz>

LGTM -- I have queued the series for the next release, including
extending the commit IDs...

Thanks!

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.