[dpdk-dev] [PATCH 0/8] Various fixes to compile with gcc6

[dpdk-dev] [PATCH 0/8] Various fixes to compile with gcc6

[Aaron Conole]

This series brings a number of code cleanups to allow building using gcc6,
with various legitimate warnings being fixed.

In particular, patch 3 ("drivers/net/e1000: Fix missing brackets") should be
checked for correctness (it does not alter any behavior from a functional
standpoint, but it may be required to do so for a correct fix).

Aaron Conole (8):
  lpm: Fix pointer aliasing issues
  app/test/test: Fix missing brackets
  drivers/net/e1000: Fix missing brackets
  drivers/net/e1000: Fix missing lsc interrupt check brackets
  drivers/net/ixgbe: Fix vlan filter missing brackets
  drivers/net/e1000/igb: Signed left shift operator
  drivers/net/ixgbe: Signed left shift operator
  drivers/net/ixgbe: Fix uninitialized warning

 app/test/test.c                    |  3 ++-
 drivers/net/e1000/base/e1000_phy.c | 12 ++++-----
 drivers/net/e1000/em_ethdev.c      |  3 ++-
 drivers/net/e1000/igb_pf.c         |  4 +--
 drivers/net/ixgbe/ixgbe_ethdev.c   |  3 ++-
 drivers/net/ixgbe/ixgbe_pf.c       |  4 +--
 drivers/net/ixgbe/ixgbe_rxtx.c     |  4 +--
 lib/librte_lpm/rte_lpm.h           | 53 +++++++++++++++++++++++++++++---------
 8 files changed, 59 insertions(+), 27 deletions(-)

-- 
2.5.0

[dpdk-dev] [PATCH 1/8] lpm: Fix pointer aliasing issues

[Aaron Conole]

The current implementation attempts to use a uint16_t to alias the lpm table
structures. Such aliasing can break optimizer performance. This patch uses
union type indirection and adds static inline functions for performing the
aliasing.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/librte_lpm/rte_lpm.h | 53 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
index c299ce2..eae6ff1 100644
--- a/lib/librte_lpm/rte_lpm.h
+++ b/lib/librte_lpm/rte_lpm.h
@@ -157,6 +157,33 @@ struct rte_lpm {
 };
 
 /**
+ * Convert from tbl_entry types to integer types
+ */
+static inline uint16_t
+rte_lpm_tbl24_entry_to_uint16(const struct rte_lpm_tbl24_entry *entry)
+{
+	union {
+		uint16_t                   i;
+		struct rte_lpm_tbl24_entry s;
+	} tbl_entry_u;
+
+	tbl_entry_u.s = *entry;
+	return tbl_entry_u.i;
+}
+
+static inline uint16_t
+rte_lpm_tbl8_entry_to_uint16(const struct rte_lpm_tbl8_entry *entry)
+{
+	union {
+		uint16_t                  i;
+		struct rte_lpm_tbl8_entry s;
+	} tbl_entry_u;
+
+	tbl_entry_u.s = *entry;
+	return tbl_entry_u.i;
+}
+
+/**
  * Create an LPM object.
  *
  * @param name
@@ -286,7 +313,7 @@ rte_lpm_lookup(struct rte_lpm *lpm, uint32_t ip, uint8_t *next_hop)
 	RTE_LPM_RETURN_IF_TRUE(((lpm == NULL) || (next_hop == NULL)), -EINVAL);
 
 	/* Copy tbl24 entry */
-	tbl_entry = *(const uint16_t *)&lpm->tbl24[tbl24_index];
+	tbl_entry = rte_lpm_tbl24_entry_to_uint16(&lpm->tbl24[tbl24_index]);
 
 	/* Copy tbl8 entry (only if needed) */
 	if (unlikely((tbl_entry & RTE_LPM_VALID_EXT_ENTRY_BITMASK) ==
@@ -295,7 +322,7 @@ rte_lpm_lookup(struct rte_lpm *lpm, uint32_t ip, uint8_t *next_hop)
 		unsigned tbl8_index = (uint8_t)ip +
 				((uint8_t)tbl_entry * RTE_LPM_TBL8_GROUP_NUM_ENTRIES);
 
-		tbl_entry = *(const uint16_t *)&lpm->tbl8[tbl8_index];
+		tbl_entry = rte_lpm_tbl8_entry_to_uint16(&lpm->tbl8[tbl8_index]);
 	}
 
 	*next_hop = (uint8_t)tbl_entry;
@@ -342,7 +369,8 @@ rte_lpm_lookup_bulk_func(const struct rte_lpm *lpm, const uint32_t * ips,
 
 	for (i = 0; i < n; i++) {
 		/* Simply copy tbl24 entry to output */
-		next_hops[i] = *(const uint16_t *)&lpm->tbl24[tbl24_indexes[i]];
+		next_hops[i] = rte_lpm_tbl24_entry_to_uint16(
+			&lpm->tbl24[tbl24_indexes[i]]);
 
 		/* Overwrite output with tbl8 entry if needed */
 		if (unlikely((next_hops[i] & RTE_LPM_VALID_EXT_ENTRY_BITMASK) ==
@@ -352,7 +380,8 @@ rte_lpm_lookup_bulk_func(const struct rte_lpm *lpm, const uint32_t * ips,
 					((uint8_t)next_hops[i] *
 					 RTE_LPM_TBL8_GROUP_NUM_ENTRIES);
 
-			next_hops[i] = *(const uint16_t *)&lpm->tbl8[tbl8_index];
+			next_hops[i] = rte_lpm_tbl8_entry_to_uint16(
+					&lpm->tbl8[tbl8_index]);
 		}
 	}
 	return 0;
@@ -419,13 +448,13 @@ rte_lpm_lookupx4(const struct rte_lpm *lpm, __m128i ip, uint16_t hop[4],
 	idx = _mm_cvtsi128_si64(i24);
 	i24 = _mm_srli_si128(i24, sizeof(uint64_t));
 
-	tbl[0] = *(const uint16_t *)&lpm->tbl24[(uint32_t)idx];
-	tbl[1] = *(const uint16_t *)&lpm->tbl24[idx >> 32];
+	tbl[0] = rte_lpm_tbl24_entry_to_uint16(&lpm->tbl24[(uint32_t)idx]);
+	tbl[1] = rte_lpm_tbl24_entry_to_uint16(&lpm->tbl24[idx >> 32]);
 
 	idx = _mm_cvtsi128_si64(i24);
 
-	tbl[2] = *(const uint16_t *)&lpm->tbl24[(uint32_t)idx];
-	tbl[3] = *(const uint16_t *)&lpm->tbl24[idx >> 32];
+	tbl[2] = rte_lpm_tbl24_entry_to_uint16(&lpm->tbl24[(uint32_t)idx]);
+	tbl[3] = rte_lpm_tbl24_entry_to_uint16(&lpm->tbl24[idx >> 32]);
 
 	/* get 4 indexes for tbl8[]. */
 	i8.x = _mm_and_si128(ip, mask8);
@@ -446,25 +475,25 @@ rte_lpm_lookupx4(const struct rte_lpm *lpm, __m128i ip, uint16_t hop[4],
 			RTE_LPM_VALID_EXT_ENTRY_BITMASK)) {
 		i8.u32[0] = i8.u32[0] +
 			(uint8_t)tbl[0] * RTE_LPM_TBL8_GROUP_NUM_ENTRIES;
-		tbl[0] = *(const uint16_t *)&lpm->tbl8[i8.u32[0]];
+		tbl[0] = rte_lpm_tbl8_entry_to_uint16(&lpm->tbl8[i8.u32[0]]);
 	}
 	if (unlikely((pt >> 16 & RTE_LPM_VALID_EXT_ENTRY_BITMASK) ==
 			RTE_LPM_VALID_EXT_ENTRY_BITMASK)) {
 		i8.u32[1] = i8.u32[1] +
 			(uint8_t)tbl[1] * RTE_LPM_TBL8_GROUP_NUM_ENTRIES;
-		tbl[1] = *(const uint16_t *)&lpm->tbl8[i8.u32[1]];
+		tbl[1] = rte_lpm_tbl8_entry_to_uint16(&lpm->tbl8[i8.u32[1]]);
 	}
 	if (unlikely((pt >> 32 & RTE_LPM_VALID_EXT_ENTRY_BITMASK) ==
 			RTE_LPM_VALID_EXT_ENTRY_BITMASK)) {
 		i8.u32[2] = i8.u32[2] +
 			(uint8_t)tbl[2] * RTE_LPM_TBL8_GROUP_NUM_ENTRIES;
-		tbl[2] = *(const uint16_t *)&lpm->tbl8[i8.u32[2]];
+		tbl[2] = rte_lpm_tbl8_entry_to_uint16(&lpm->tbl8[i8.u32[2]]);
 	}
 	if (unlikely((pt >> 48 & RTE_LPM_VALID_EXT_ENTRY_BITMASK) ==
 			RTE_LPM_VALID_EXT_ENTRY_BITMASK)) {
 		i8.u32[3] = i8.u32[3] +
 			(uint8_t)tbl[3] * RTE_LPM_TBL8_GROUP_NUM_ENTRIES;
-		tbl[3] = *(const uint16_t *)&lpm->tbl8[i8.u32[3]];
+		tbl[3] = rte_lpm_tbl8_entry_to_uint16(&lpm->tbl8[i8.u32[3]]);
 	}
 
 	hop[0] = (tbl[0] & RTE_LPM_LOOKUP_SUCCESS) ? (uint8_t)tbl[0] : defv;
-- 
2.5.0

Re: [dpdk-dev] [PATCH 1/8] lpm: Fix pointer aliasing issues

[Bruce Richardson]

On Thu, Feb 25, 2016 at 01:48:34PM -0500, Aaron Conole wrote:
> The current implementation attempts to use a uint16_t to alias the lpm table
> structures. Such aliasing can break optimizer performance. This patch uses
> union type indirection and adds static inline functions for performing the
> aliasing.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  lib/librte_lpm/rte_lpm.h | 53 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> index c299ce2..eae6ff1 100644
> --- a/lib/librte_lpm/rte_lpm.h
> +++ b/lib/librte_lpm/rte_lpm.h
> @@ -157,6 +157,33 @@ struct rte_lpm {
>  };
>  
>  /**
> + * Convert from tbl_entry types to integer types
> + */
> +static inline uint16_t
> +rte_lpm_tbl24_entry_to_uint16(const struct rte_lpm_tbl24_entry *entry)
> +{
> +	union {
> +		uint16_t                   i;
> +		struct rte_lpm_tbl24_entry s;
> +	} tbl_entry_u;
> +
> +	tbl_entry_u.s = *entry;
> +	return tbl_entry_u.i;
> +}
> +
> +static inline uint16_t
> +rte_lpm_tbl8_entry_to_uint16(const struct rte_lpm_tbl8_entry *entry)
> +{
> +	union {
> +		uint16_t                  i;
> +		struct rte_lpm_tbl8_entry s;
> +	} tbl_entry_u;
> +
> +	tbl_entry_u.s = *entry;
> +	return tbl_entry_u.i;
> +}
> +

These two new functions could be reduced to one with the help of patch:
http://dpdk.org/dev/patchwork/patch/9087/

Anyone care to go back and review or ack that patch for me and simplify all
the lpm code just that little bit?

/Bruce

Re: [dpdk-dev] [PATCH 1/8] lpm: Fix pointer aliasing issues

[Aaron Conole]

Hi Bruce,

Bruce Richardson <bruce.richardson@intel.com> writes:

> On Thu, Feb 25, 2016 at 01:48:34PM -0500, Aaron Conole wrote:
>> The current implementation attempts to use a uint16_t to alias the lpm table
>> structures. Such aliasing can break optimizer performance. This patch uses
>> union type indirection and adds static inline functions for performing the
>> aliasing.
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  lib/librte_lpm/rte_lpm.h | 53 +++++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 41 insertions(+), 12 deletions(-)
>> 
>> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
>> index c299ce2..eae6ff1 100644
>> --- a/lib/librte_lpm/rte_lpm.h
>> +++ b/lib/librte_lpm/rte_lpm.h
>> @@ -157,6 +157,33 @@ struct rte_lpm {
>>  };
>>  
>>  /**
>> + * Convert from tbl_entry types to integer types
>> + */
>> +static inline uint16_t
>> +rte_lpm_tbl24_entry_to_uint16(const struct rte_lpm_tbl24_entry *entry)
>> +{
>> +	union {
>> +		uint16_t                   i;
>> +		struct rte_lpm_tbl24_entry s;
>> +	} tbl_entry_u;
>> +
>> +	tbl_entry_u.s = *entry;
>> +	return tbl_entry_u.i;
>> +}
>> +
>> +static inline uint16_t
>> +rte_lpm_tbl8_entry_to_uint16(const struct rte_lpm_tbl8_entry *entry)
>> +{
>> +	union {
>> +		uint16_t                  i;
>> +		struct rte_lpm_tbl8_entry s;
>> +	} tbl_entry_u;
>> +
>> +	tbl_entry_u.s = *entry;
>> +	return tbl_entry_u.i;
>> +}
>> +
>
> These two new functions could be reduced to one with the help of patch:
> http://dpdk.org/dev/patchwork/patch/9087/
>
> Anyone care to go back and review or ack that patch for me and simplify all
> the lpm code just that little bit?

I definitely ack that patch; I also want to apply it in my local testing
setup and test a bit so you can add my Tested-by, but it looks sane on
the surface.

Thanks so much for the review!

> /Bruce

Re: [dpdk-dev] [PATCH 1/8] lpm: Fix pointer aliasing issues

[Thomas Monjalon]

2016-02-25 21:30, Bruce Richardson:
> On Thu, Feb 25, 2016 at 01:48:34PM -0500, Aaron Conole wrote:
> >  /**
> > + * Convert from tbl_entry types to integer types
> > + */
> > +static inline uint16_t
> > +rte_lpm_tbl24_entry_to_uint16(const struct rte_lpm_tbl24_entry *entry)
> > +{
> > +	union {
> > +		uint16_t                   i;
> > +		struct rte_lpm_tbl24_entry s;
> > +	} tbl_entry_u;
> > +
> > +	tbl_entry_u.s = *entry;
> > +	return tbl_entry_u.i;
> > +}
> > +
> > +static inline uint16_t
> > +rte_lpm_tbl8_entry_to_uint16(const struct rte_lpm_tbl8_entry *entry)
> > +{
> > +	union {
> > +		uint16_t                  i;
> > +		struct rte_lpm_tbl8_entry s;
> > +	} tbl_entry_u;
> > +
> > +	tbl_entry_u.s = *entry;
> > +	return tbl_entry_u.i;
> > +}
> > +
> 
> These two new functions could be reduced to one with the help of patch:
> http://dpdk.org/dev/patchwork/patch/9087/

Aaron, any news about a rework of this patch?

Re: [dpdk-dev] [PATCH 1/8] lpm: Fix pointer aliasing issues

[Aaron Conole]

Thomas Monjalon <thomas.monjalon@6wind.com> writes:

> 2016-02-25 21:30, Bruce Richardson:
>> On Thu, Feb 25, 2016 at 01:48:34PM -0500, Aaron Conole wrote:
>> >  /**
>> > + * Convert from tbl_entry types to integer types
>> > + */
>> > +static inline uint16_t
>> > +rte_lpm_tbl24_entry_to_uint16(const struct rte_lpm_tbl24_entry *entry)
>> > +{
>> > +	union {
>> > +		uint16_t                   i;
>> > +		struct rte_lpm_tbl24_entry s;
>> > +	} tbl_entry_u;
>> > +
>> > +	tbl_entry_u.s = *entry;
>> > +	return tbl_entry_u.i;
>> > +}
>> > +
>> > +static inline uint16_t
>> > +rte_lpm_tbl8_entry_to_uint16(const struct rte_lpm_tbl8_entry *entry)
>> > +{
>> > +	union {
>> > +		uint16_t                  i;
>> > +		struct rte_lpm_tbl8_entry s;
>> > +	} tbl_entry_u;
>> > +
>> > +	tbl_entry_u.s = *entry;
>> > +	return tbl_entry_u.i;
>> > +}
>> > +
>> 
>> These two new functions could be reduced to one with the help of patch:
>> http://dpdk.org/dev/patchwork/patch/9087/
>
> Aaron, any news about a rework of this patch?

The rework of this series is in my TODO list with deadline of
Thursday. I'll repost the series before then. Sorry for the
confusion/delay.

-Aaron

[dpdk-dev] [PATCH 2/8] app/test/test: Fix missing brackets

[Aaron Conole]

The test application calls printf(...) with the suite->suite_name argument.
The intent (based on whitespace) in the printf is to check suite->suite_name
first and then apply the printf. This doesn't happen due to missing brackets.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 app/test/test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/test/test.c b/app/test/test.c
index f35b304..ccad0e3 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -162,9 +162,10 @@ unit_test_suite_runner(struct unit_test_suite *suite)
 	int test_success;
 	unsigned total = 0, executed = 0, skipped = 0, succeeded = 0, failed = 0;
 
-	if (suite->suite_name)
+	if (suite->suite_name) {
 		printf(" + ------------------------------------------------------- +\n");
 		printf(" + Test Suite : %s\n", suite->suite_name);
+	}
 
 	if (suite->setup)
 		if (suite->setup() != 0)
-- 
2.5.0

Re: [dpdk-dev] [PATCH 2/8] app/test/test: Fix missing brackets

[Panu Matilainen]

On 02/25/2016 08:48 PM, Aaron Conole wrote:
> The test application calls printf(...) with the suite->suite_name argument.
> The intent (based on whitespace) in the printf is to check suite->suite_name
> first and then apply the printf. This doesn't happen due to missing brackets.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   app/test/test.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/app/test/test.c b/app/test/test.c
> index f35b304..ccad0e3 100644
> --- a/app/test/test.c
> +++ b/app/test/test.c
> @@ -162,9 +162,10 @@ unit_test_suite_runner(struct unit_test_suite *suite)
>   	int test_success;
>   	unsigned total = 0, executed = 0, skipped = 0, succeeded = 0, failed = 0;
>
> -	if (suite->suite_name)
> +	if (suite->suite_name) {
>   		printf(" + ------------------------------------------------------- +\n");
>   		printf(" + Test Suite : %s\n", suite->suite_name);
> +	}
>
>   	if (suite->setup)
>   		if (suite->setup() != 0)
>

Acked-by: Panu Matilainen <pmatilai@redhat.com>

This is just about as obvious as they get...

	- Panu -

Re: [dpdk-dev] [PATCH 2/8] app/test/test: Fix missing brackets

[Zhang, Helin]

> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: [PATCH 2/8] app/test/test: Fix missing brackets
> 
> The test application calls printf(...) with the suite->suite_name argument.
> The intent (based on whitespace) in the printf is to check suite->suite_name first
> and then apply the printf. This doesn't happen due to missing brackets.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>
> ---
>  app/test/test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test/test.c b/app/test/test.c index f35b304..ccad0e3 100644
> --- a/app/test/test.c
> +++ b/app/test/test.c
> @@ -162,9 +162,10 @@ unit_test_suite_runner(struct unit_test_suite *suite)
>  	int test_success;
>  	unsigned total = 0, executed = 0, skipped = 0, succeeded = 0, failed = 0;
> 
> -	if (suite->suite_name)
> +	if (suite->suite_name) {
>  		printf(" + ------------------------------------------------------- +\n");
>  		printf(" + Test Suite : %s\n", suite->suite_name);
> +	}
> 
>  	if (suite->setup)
>  		if (suite->setup() != 0)
> --
> 2.5.0

[dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets

[Aaron Conole]

The register read/write mphy functions have misleading whitespace around
the locked check. This cleanup merely preserves the existing functionality
while improving the ready check.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 drivers/net/e1000/base/e1000_phy.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/e1000/base/e1000_phy.c b/drivers/net/e1000/base/e1000_phy.c
index d43b7ce..8642d38 100644
--- a/drivers/net/e1000/base/e1000_phy.c
+++ b/drivers/net/e1000/base/e1000_phy.c
@@ -4153,13 +4153,13 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw *hw, u32 address, u32 *data)
 	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
 
 	/* Disable access to mPHY if it was originally disabled */
-	if (locked)
+	if (locked) {
 		ready = e1000_is_mphy_ready(hw);
 		if (!ready)
 			return -E1000_ERR_PHY;
-		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
-				E1000_MPHY_DIS_ACCESS);
+	}
 
+	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL, E1000_MPHY_DIS_ACCESS);
 	return E1000_SUCCESS;
 }
 
@@ -4218,13 +4218,13 @@ s32 e1000_write_phy_reg_mphy(struct e1000_hw *hw, u32 address, u32 data,
 	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
 
 	/* Disable access to mPHY if it was originally disabled */
-	if (locked)
+	if (locked) {
 		ready = e1000_is_mphy_ready(hw);
 		if (!ready)
 			return -E1000_ERR_PHY;
-		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
-				E1000_MPHY_DIS_ACCESS);
+	}
 
+	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL, E1000_MPHY_DIS_ACCESS);
 	return E1000_SUCCESS;
 }
 
-- 
2.5.0

Re: [dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets

[Lu, Wenzhuo]

Hi Aaron,


> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo; Zhang, Helin; Ananyev, Konstantin; Richardson, Bruce
> Subject: [PATCH 3/8] drivers/net/e1000: Fix missing brackets
> 
> The register read/write mphy functions have misleading whitespace around the
> locked check. This cleanup merely preserves the existing functionality while
> improving the ready check.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  drivers/net/e1000/base/e1000_phy.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/e1000/base/e1000_phy.c
> b/drivers/net/e1000/base/e1000_phy.c
> index d43b7ce..8642d38 100644
> --- a/drivers/net/e1000/base/e1000_phy.c
> +++ b/drivers/net/e1000/base/e1000_phy.c
> @@ -4153,13 +4153,13 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw
> *hw, u32 address, u32 *data)
>  	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
> 
>  	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>  		ready = e1000_is_mphy_ready(hw);
>  		if (!ready)
>  			return -E1000_ERR_PHY;
> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> -				E1000_MPHY_DIS_ACCESS);
> +	}
> 
> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> E1000_MPHY_DIS_ACCESS);
>  	return E1000_SUCCESS;
>  }
> 
> @@ -4218,13 +4218,13 @@ s32 e1000_write_phy_reg_mphy(struct e1000_hw
> *hw, u32 address, u32 data,
>  	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
> 
>  	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>  		ready = e1000_is_mphy_ready(hw);
>  		if (!ready)
>  			return -E1000_ERR_PHY;
> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> -				E1000_MPHY_DIS_ACCESS);
> +	}
> 
> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> E1000_MPHY_DIS_ACCESS);
>  	return E1000_SUCCESS;
>  }
> 
> --
> 2.5.0
Normally we will not maintain the base code. It's just taken from kernel driver.
Agree with you that the whitespace is misleading. But as it's no real impact. I'd like to say not a big deal, better not  change it. :)

Re: [dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets

[Aaron Conole]

Hi Wenzhou,

"Lu, Wenzhuo" <wenzhuo.lu@intel.com> writes:

> Hi Aaron,
>
>
>> -----Original Message-----
>> From: Aaron Conole [mailto:aconole@redhat.com]
>> Sent: Friday, February 26, 2016 2:49 AM
>> To: dev@dpdk.org
>> Cc: Lu, Wenzhuo; Zhang, Helin; Ananyev, Konstantin; Richardson, Bruce
>> Subject: [PATCH 3/8] drivers/net/e1000: Fix missing brackets
>> 
>> The register read/write mphy functions have misleading whitespace around the
>> locked check. This cleanup merely preserves the existing functionality while
>> improving the ready check.
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>>  drivers/net/e1000/base/e1000_phy.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/net/e1000/base/e1000_phy.c
>> b/drivers/net/e1000/base/e1000_phy.c
>> index d43b7ce..8642d38 100644
>> --- a/drivers/net/e1000/base/e1000_phy.c
>> +++ b/drivers/net/e1000/base/e1000_phy.c
>> @@ -4153,13 +4153,13 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw
>> *hw, u32 address, u32 *data)
>>  	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
>> 
>>  	/* Disable access to mPHY if it was originally disabled */
>> -	if (locked)
>> +	if (locked) {
>>  		ready = e1000_is_mphy_ready(hw);
>>  		if (!ready)
>>  			return -E1000_ERR_PHY;
>> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
>> -				E1000_MPHY_DIS_ACCESS);
>> +	}
>> 
>> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
>> E1000_MPHY_DIS_ACCESS);
>>  	return E1000_SUCCESS;
>>  }
>> 
>> @@ -4218,13 +4218,13 @@ s32 e1000_write_phy_reg_mphy(struct e1000_hw
>> *hw, u32 address, u32 data,
>>  	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
>> 
>>  	/* Disable access to mPHY if it was originally disabled */
>> -	if (locked)
>> +	if (locked) {
>>  		ready = e1000_is_mphy_ready(hw);
>>  		if (!ready)
>>  			return -E1000_ERR_PHY;
>> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
>> -				E1000_MPHY_DIS_ACCESS);
>> +	}
>> 
>> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
>> E1000_MPHY_DIS_ACCESS);
>>  	return E1000_SUCCESS;
>>  }
>> 
>> --
>> 2.5.0
> Normally we will not maintain the base code. It's just taken from kernel driver.
> Agree with you that the whitespace is misleading. But as it's no real
> impact. I'd like to say not a big deal, better not change it. :)

Thanks for this hint. It turns out my patch is wrong. It should actually
be this (and I've confirmed by looking at the drivers):

diff --git a/drivers/net/e1000/base/e1000_phy.c b/drivers/net/e1000/base/e1000_phy.c
index d43b7ce..ad3fd58 100644
--- a/drivers/net/e1000/base/e1000_phy.c
+++ b/drivers/net/e1000/base/e1000_phy.c
@@ -4153,12 +4153,12 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw *hw, u32 address, u32 *data)
 	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
 
 	/* Disable access to mPHY if it was originally disabled */
-	if (locked)
+	if (locked) {
 		ready = e1000_is_mphy_ready(hw);
 		if (!ready)
 			return -E1000_ERR_PHY;
-		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
-				E1000_MPHY_DIS_ACCESS);
+		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL, E1000_MPHY_DIS_ACCESS);
+	}
 
 	return E1000_SUCCESS;
 }
@@ -4218,12 +4218,12 @@ s32 e1000_write_phy_reg_mphy(struct e1000_hw *hw, u32 address, u32 data,
 	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
 
 	/* Disable access to mPHY if it was originally disabled */
-	if (locked)
+	if (locked) {
 		ready = e1000_is_mphy_ready(hw);
 		if (!ready)
 			return -E1000_ERR_PHY;
-		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
-				E1000_MPHY_DIS_ACCESS);
+		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL, E1000_MPHY_DIS_ACCESS);
+	}
 
 	return E1000_SUCCESS;
 }

I will cook up a v2 of this patch if it makes sense. It is a real bug,
so should be fixed.

-Aaron

Re: [dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets

[Panu Matilainen]

On 02/26/2016 03:13 PM, Aaron Conole wrote:
> Hi Wenzhou,
>
> "Lu, Wenzhuo" <wenzhuo.lu@intel.com> writes:
>
>> Hi Aaron,
>>
>>
>>> -----Original Message-----
>>> From: Aaron Conole [mailto:aconole@redhat.com]
>>> Sent: Friday, February 26, 2016 2:49 AM
>>> To: dev@dpdk.org
>>> Cc: Lu, Wenzhuo; Zhang, Helin; Ananyev, Konstantin; Richardson, Bruce
>>> Subject: [PATCH 3/8] drivers/net/e1000: Fix missing brackets
>>>
>>> The register read/write mphy functions have misleading whitespace around the
>>> locked check. This cleanup merely preserves the existing functionality while
>>> improving the ready check.
>>>
>>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>>> ---
>>>   drivers/net/e1000/base/e1000_phy.c | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/e1000/base/e1000_phy.c
>>> b/drivers/net/e1000/base/e1000_phy.c
>>> index d43b7ce..8642d38 100644
>>> --- a/drivers/net/e1000/base/e1000_phy.c
>>> +++ b/drivers/net/e1000/base/e1000_phy.c
>>> @@ -4153,13 +4153,13 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw
>>> *hw, u32 address, u32 *data)
>>>   	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
>>>
>>>   	/* Disable access to mPHY if it was originally disabled */
>>> -	if (locked)
>>> +	if (locked) {
>>>   		ready = e1000_is_mphy_ready(hw);
>>>   		if (!ready)
>>>   			return -E1000_ERR_PHY;
>>> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
>>> -				E1000_MPHY_DIS_ACCESS);
>>> +	}
>>>
>>> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
>>> E1000_MPHY_DIS_ACCESS);
>>>   	return E1000_SUCCESS;
>>>   }
>>>
>>> @@ -4218,13 +4218,13 @@ s32 e1000_write_phy_reg_mphy(struct e1000_hw
>>> *hw, u32 address, u32 data,
>>>   	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
>>>
>>>   	/* Disable access to mPHY if it was originally disabled */
>>> -	if (locked)
>>> +	if (locked) {
>>>   		ready = e1000_is_mphy_ready(hw);
>>>   		if (!ready)
>>>   			return -E1000_ERR_PHY;
>>> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
>>> -				E1000_MPHY_DIS_ACCESS);
>>> +	}
>>>
>>> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
>>> E1000_MPHY_DIS_ACCESS);
>>>   	return E1000_SUCCESS;
>>>   }
>>>
>>> --
>>> 2.5.0
>> Normally we will not maintain the base code. It's just taken from kernel driver.
>> Agree with you that the whitespace is misleading. But as it's no real
>> impact. I'd like to say not a big deal, better not change it. :)
>
> Thanks for this hint. It turns out my patch is wrong. It should actually
> be this (and I've confirmed by looking at the drivers):
>
> diff --git a/drivers/net/e1000/base/e1000_phy.c b/drivers/net/e1000/base/e1000_phy.c
> index d43b7ce..ad3fd58 100644
> --- a/drivers/net/e1000/base/e1000_phy.c
> +++ b/drivers/net/e1000/base/e1000_phy.c
> @@ -4153,12 +4153,12 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw *hw, u32 address, u32 *data)
>   	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
>
>   	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>   		ready = e1000_is_mphy_ready(hw);
>   		if (!ready)
>   			return -E1000_ERR_PHY;
> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> -				E1000_MPHY_DIS_ACCESS);
> +		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL, E1000_MPHY_DIS_ACCESS);
> +	}
>
>   	return E1000_SUCCESS;
>   }
> @@ -4218,12 +4218,12 @@ s32 e1000_write_phy_reg_mphy(struct e1000_hw *hw, u32 address, u32 data,
>   	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
>
>   	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>   		ready = e1000_is_mphy_ready(hw);
>   		if (!ready)
>   			return -E1000_ERR_PHY;
> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> -				E1000_MPHY_DIS_ACCESS);
> +		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL, E1000_MPHY_DIS_ACCESS);
> +	}
>
>   	return E1000_SUCCESS;
>   }
>
> I will cook up a v2 of this patch if it makes sense. It is a real bug,
> so should be fixed.

Yes, it quite clearly is a real bug and there needs to be a documented 
way of getting these things fixed. The README in the base/ directory is 
not particularly helpful, since it only says "dont touch it".

This is apparently fixed in FreeBSD codebase so fixing it would be 
"just" a matter of pulling in a newer version.

The other alternatives are either disabling the whole driver in gcc 6 
builds, or paper over the bug with warning disablers, or have everybody 
patch their packages locally to fix it, all of which just feel so stupid 
they're not alternatives, really.

OTOH the bug has been there for 2.5 years (since commit 
38db3f7f50bde45477f564783a06ac8fbd3348fa) and nobody has noticed...

Thomas, Bruce, thoughts/comments?

	- Panu -

Re: [dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets

[Thomas Monjalon]

2016-03-01 13:02, Panu Matilainen:
> On 02/26/2016 03:13 PM, Aaron Conole wrote:
> > "Lu, Wenzhuo" <wenzhuo.lu@intel.com> writes:
> >> Normally we will not maintain the base code. It's just taken from kernel driver.
> >> Agree with you that the whitespace is misleading. But as it's no real
> >> impact. I'd like to say not a big deal, better not change it. :)
> >
> > Thanks for this hint. It turns out my patch is wrong. It should actually
> > be this (and I've confirmed by looking at the drivers):
[...]
> > I will cook up a v2 of this patch if it makes sense. It is a real bug,
> > so should be fixed.

Yes, waiting for your v2.

> Yes, it quite clearly is a real bug and there needs to be a documented 
> way of getting these things fixed. The README in the base/ directory is 
> not particularly helpful, since it only says "dont touch it".

Yes don't touch it ;)

> This is apparently fixed in FreeBSD codebase so fixing it would be 
> "just" a matter of pulling in a newer version.

Probably.

> The other alternatives are either disabling the whole driver in gcc 6 
> builds, or paper over the bug with warning disablers, or have everybody 
> patch their packages locally to fix it, all of which just feel so stupid 
> they're not alternatives, really.

No they are not sane alternatives.

> OTOH the bug has been there for 2.5 years (since commit 
> 38db3f7f50bde45477f564783a06ac8fbd3348fa) and nobody has noticed...
> 
> Thomas, Bruce, thoughts/comments?

It is in the hands of Wenzhuo, the e1000 maintainer.
You just need his ack.

Re: [dpdk-dev] [PATCH 3/8] drivers/net/e1000: Fix missing brackets

[Zhang, Helin]

> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: [PATCH 3/8] drivers/net/e1000: Fix missing brackets
> 
> The register read/write mphy functions have misleading whitespace around the
> locked check. This cleanup merely preserves the existing functionality while
> improving the ready check.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  drivers/net/e1000/base/e1000_phy.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/e1000/base/e1000_phy.c
> b/drivers/net/e1000/base/e1000_phy.c
> index d43b7ce..8642d38 100644
> --- a/drivers/net/e1000/base/e1000_phy.c
> +++ b/drivers/net/e1000/base/e1000_phy.c
> @@ -4153,13 +4153,13 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw
> *hw, u32 address, u32 *data)
>  	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
> 
>  	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>  		ready = e1000_is_mphy_ready(hw);
>  		if (!ready)
>  			return -E1000_ERR_PHY;
> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> -				E1000_MPHY_DIS_ACCESS);
> +	}
> 
> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> E1000_MPHY_DIS_ACCESS);
I think the above line should be included in brackets above.
Though we should avoid modifying any code in base driver, I'd
suggest you to fix the issue here. Thank you!

>  	return E1000_SUCCESS;
>  }
> 
> @@ -4218,13 +4218,13 @@ s32 e1000_write_phy_reg_mphy(struct e1000_hw
> *hw, u32 address, u32 data,
>  	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
> 
>  	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>  		ready = e1000_is_mphy_ready(hw);
>  		if (!ready)
>  			return -E1000_ERR_PHY;
> -		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> -				E1000_MPHY_DIS_ACCESS);
> +	}
> 
> +	E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
> E1000_MPHY_DIS_ACCESS);
>  	return E1000_SUCCESS;
The same comments as above. Thanks!

>  }
> 
> --
> 2.5.0

[dpdk-dev] [PATCH 4/8] drivers/net/e1000: Fix missing lsc interrupt check brackets

[Aaron Conole]

The device lsc interupt check has a misleading whitespace around it which
can be improved by adding appropriate braces to the check. Since the ret
variable was checked after previous assignment, this introduces no functional
change.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 drivers/net/e1000/em_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 4a843fe..1d86091 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -637,13 +637,14 @@ eth_em_start(struct rte_eth_dev *dev)
 
 	if (rte_intr_allow_others(intr_handle)) {
 		/* check if lsc interrupt is enabled */
-		if (dev->data->dev_conf.intr_conf.lsc != 0)
+		if (dev->data->dev_conf.intr_conf.lsc != 0) {
 			ret = eth_em_interrupt_setup(dev);
 			if (ret) {
 				PMD_INIT_LOG(ERR, "Unable to setup interrupts");
 				em_dev_clear_queues(dev);
 				return ret;
 			}
+		}
 	} else {
 		rte_intr_callback_unregister(intr_handle,
 						eth_em_interrupt_handler,
-- 
2.5.0

Re: [dpdk-dev] [PATCH 4/8] drivers/net/e1000: Fix missing lsc interrupt check brackets

[Panu Matilainen]

On 02/25/2016 08:48 PM, Aaron Conole wrote:
> The device lsc interupt check has a misleading whitespace around it which
> can be improved by adding appropriate braces to the check. Since the ret
> variable was checked after previous assignment, this introduces no functional
> change.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   drivers/net/e1000/em_ethdev.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
> index 4a843fe..1d86091 100644
> --- a/drivers/net/e1000/em_ethdev.c
> +++ b/drivers/net/e1000/em_ethdev.c
> @@ -637,13 +637,14 @@ eth_em_start(struct rte_eth_dev *dev)
>
>   	if (rte_intr_allow_others(intr_handle)) {
>   		/* check if lsc interrupt is enabled */
> -		if (dev->data->dev_conf.intr_conf.lsc != 0)
> +		if (dev->data->dev_conf.intr_conf.lsc != 0) {
>   			ret = eth_em_interrupt_setup(dev);
>   			if (ret) {
>   				PMD_INIT_LOG(ERR, "Unable to setup interrupts");
>   				em_dev_clear_queues(dev);
>   				return ret;
>   			}
> +		}
>   	} else {
>   		rte_intr_callback_unregister(intr_handle,
>   						eth_em_interrupt_handler,
>

Acked-by: Panu Matilainen <pmatilai@redhat.com>

Seems really obvious but cc'd the e1000 maintainer too.

	- Panu -

Re: [dpdk-dev] [PATCH 4/8] drivers/net/e1000: Fix missing lsc interrupt check brackets

[Zhang, Helin]

> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: [PATCH 4/8] drivers/net/e1000: Fix missing lsc interrupt check brackets
> 
> The device lsc interupt check has a misleading whitespace around it which can be
> improved by adding appropriate braces to the check. Since the ret variable was
> checked after previous assignment, this introduces no functional change.

It seems a "Fixes: " line is required.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  drivers/net/e1000/em_ethdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
> index 4a843fe..1d86091 100644
> --- a/drivers/net/e1000/em_ethdev.c
> +++ b/drivers/net/e1000/em_ethdev.c
> @@ -637,13 +637,14 @@ eth_em_start(struct rte_eth_dev *dev)
> 
>  	if (rte_intr_allow_others(intr_handle)) {
>  		/* check if lsc interrupt is enabled */
> -		if (dev->data->dev_conf.intr_conf.lsc != 0)
> +		if (dev->data->dev_conf.intr_conf.lsc != 0) {
>  			ret = eth_em_interrupt_setup(dev);
>  			if (ret) {
>  				PMD_INIT_LOG(ERR, "Unable to setup interrupts");
>  				em_dev_clear_queues(dev);
>  				return ret;
>  			}
> +		}
>  	} else {
>  		rte_intr_callback_unregister(intr_handle,
>  						eth_em_interrupt_handler,
> --
> 2.5.0

[dpdk-dev] [PATCH 5/8] drivers/net/ixgbe: Fix vlan filter missing brackets

[Aaron Conole]

The ixgbe vlan filter code has an if check with an incorrect whitespace.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 3e6fe86..2e1c3ad 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4258,10 +4258,11 @@ ixgbe_set_pool_vlan_filter(struct rte_eth_dev *dev, uint16_t vlan,
 	if (ixgbe_vmdq_mode_check(hw) < 0)
 		return -ENOTSUP;
 	for (pool_idx = 0; pool_idx < ETH_64_POOLS; pool_idx++) {
-		if (pool_mask & ((uint64_t)(1ULL << pool_idx)))
+		if (pool_mask & ((uint64_t)(1ULL << pool_idx))) {
 			ret = hw->mac.ops.set_vfta(hw,vlan,pool_idx,vlan_on);
 			if (ret < 0)
 				return ret;
+		}
 	}
 
 	return ret;
-- 
2.5.0

Re: [dpdk-dev] [PATCH 5/8] drivers/net/ixgbe: Fix vlan filter missing brackets

[Panu Matilainen]

On 02/25/2016 08:48 PM, Aaron Conole wrote:
> The ixgbe vlan filter code has an if check with an incorrect whitespace.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   drivers/net/ixgbe/ixgbe_ethdev.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 3e6fe86..2e1c3ad 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -4258,10 +4258,11 @@ ixgbe_set_pool_vlan_filter(struct rte_eth_dev *dev, uint16_t vlan,
>   	if (ixgbe_vmdq_mode_check(hw) < 0)
>   		return -ENOTSUP;
>   	for (pool_idx = 0; pool_idx < ETH_64_POOLS; pool_idx++) {
> -		if (pool_mask & ((uint64_t)(1ULL << pool_idx)))
> +		if (pool_mask & ((uint64_t)(1ULL << pool_idx))) {
>   			ret = hw->mac.ops.set_vfta(hw,vlan,pool_idx,vlan_on);
>   			if (ret < 0)
>   				return ret;
> +		}
>   	}
>
>   	return ret;
>

Acked-by: Panu Matilainen <pmatilai@redhat.com>

Seems really obvious but cc'd the ixgbe maintainers too.

	- Panu -

Re: [dpdk-dev] [PATCH 5/8] drivers/net/ixgbe: Fix vlan filter missing brackets

[Zhang, Helin]

> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: [PATCH 5/8] drivers/net/ixgbe: Fix vlan filter missing brackets
> 
> The ixgbe vlan filter code has an if check with an incorrect whitespace.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>

> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 3e6fe86..2e1c3ad 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -4258,10 +4258,11 @@ ixgbe_set_pool_vlan_filter(struct rte_eth_dev *dev,
> uint16_t vlan,
>  	if (ixgbe_vmdq_mode_check(hw) < 0)
>  		return -ENOTSUP;
>  	for (pool_idx = 0; pool_idx < ETH_64_POOLS; pool_idx++) {
> -		if (pool_mask & ((uint64_t)(1ULL << pool_idx)))
> +		if (pool_mask & ((uint64_t)(1ULL << pool_idx))) {
>  			ret = hw->mac.ops.set_vfta(hw,vlan,pool_idx,vlan_on);
>  			if (ret < 0)
>  				return ret;
> +		}
>  	}
> 
>  	return ret;
> --
> 2.5.0

[dpdk-dev] [PATCH 6/8] drivers/net/e1000/igb: Signed left shift operator

[Aaron Conole]

Tell the compiler to use an unsigned constant for the config shifts.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 drivers/net/e1000/igb_pf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/e1000/igb_pf.c b/drivers/net/e1000/igb_pf.c
index 1d00dda..afe80f5 100644
--- a/drivers/net/e1000/igb_pf.c
+++ b/drivers/net/e1000/igb_pf.c
@@ -172,8 +172,8 @@ int igb_pf_host_configure(struct rte_eth_dev *eth_dev)
 	E1000_WRITE_REG(hw, E1000_VT_CTL, vtctl);
 
 	/* Enable pools reserved to PF only */
-	E1000_WRITE_REG(hw, E1000_VFRE, (~0) << vf_num);
-	E1000_WRITE_REG(hw, E1000_VFTE, (~0) << vf_num);
+	E1000_WRITE_REG(hw, E1000_VFRE, (~0U) << vf_num);
+	E1000_WRITE_REG(hw, E1000_VFTE, (~0U) << vf_num);
 
 	/* PFDMA Tx General Switch Control Enables VMDQ loopback */
 	if (hw->mac.type == e1000_i350)
-- 
2.5.0

Re: [dpdk-dev] [PATCH 6/8] drivers/net/e1000/igb: Signed left shift operator

[Panu Matilainen]

On 02/25/2016 08:48 PM, Aaron Conole wrote:
> Tell the compiler to use an unsigned constant for the config shifts.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   drivers/net/e1000/igb_pf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/e1000/igb_pf.c b/drivers/net/e1000/igb_pf.c
> index 1d00dda..afe80f5 100644
> --- a/drivers/net/e1000/igb_pf.c
> +++ b/drivers/net/e1000/igb_pf.c
> @@ -172,8 +172,8 @@ int igb_pf_host_configure(struct rte_eth_dev *eth_dev)
>   	E1000_WRITE_REG(hw, E1000_VT_CTL, vtctl);
>
>   	/* Enable pools reserved to PF only */
> -	E1000_WRITE_REG(hw, E1000_VFRE, (~0) << vf_num);
> -	E1000_WRITE_REG(hw, E1000_VFTE, (~0) << vf_num);
> +	E1000_WRITE_REG(hw, E1000_VFRE, (~0U) << vf_num);
> +	E1000_WRITE_REG(hw, E1000_VFTE, (~0U) << vf_num);
>
>   	/* PFDMA Tx General Switch Control Enables VMDQ loopback */
>   	if (hw->mac.type == e1000_i350)
>
Acked-by: Panu Matilainen <pmatilai@redhat.com>

CC'd the e1000 maintainer too.

	- Panu -

Re: [dpdk-dev] [PATCH 6/8] drivers/net/e1000/igb: Signed left shift operator

[Zhang, Helin]

> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: [PATCH 6/8] drivers/net/e1000/igb: Signed left shift operator
> 
> Tell the compiler to use an unsigned constant for the config shifts.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>
> ---
>  drivers/net/e1000/igb_pf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/e1000/igb_pf.c b/drivers/net/e1000/igb_pf.c index
> 1d00dda..afe80f5 100644
> --- a/drivers/net/e1000/igb_pf.c
> +++ b/drivers/net/e1000/igb_pf.c
> @@ -172,8 +172,8 @@ int igb_pf_host_configure(struct rte_eth_dev *eth_dev)
>  	E1000_WRITE_REG(hw, E1000_VT_CTL, vtctl);
> 
>  	/* Enable pools reserved to PF only */
> -	E1000_WRITE_REG(hw, E1000_VFRE, (~0) << vf_num);
> -	E1000_WRITE_REG(hw, E1000_VFTE, (~0) << vf_num);
> +	E1000_WRITE_REG(hw, E1000_VFRE, (~0U) << vf_num);
> +	E1000_WRITE_REG(hw, E1000_VFTE, (~0U) << vf_num);
> 
>  	/* PFDMA Tx General Switch Control Enables VMDQ loopback */
>  	if (hw->mac.type == e1000_i350)
> --
> 2.5.0

[dpdk-dev] [PATCH 7/8] drivers/net/ixgbe: Signed left shift operator

[Aaron Conole]

Tell the compiler to use an unsigned constant for the config shifts.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 drivers/net/ixgbe/ixgbe_pf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_pf.c b/drivers/net/ixgbe/ixgbe_pf.c
index 2ffbd1f..8b5119f 100644
--- a/drivers/net/ixgbe/ixgbe_pf.c
+++ b/drivers/net/ixgbe/ixgbe_pf.c
@@ -236,9 +236,9 @@ int ixgbe_pf_host_configure(struct rte_eth_dev *eth_dev)
 	vfre_slot = (vf_num >> VFRE_SHIFT) > 0 ? 1 : 0;
 
 	/* Enable pools reserved to PF only */
-	IXGBE_WRITE_REG(hw, IXGBE_VFRE(vfre_slot), (~0) << vfre_offset);
+	IXGBE_WRITE_REG(hw, IXGBE_VFRE(vfre_slot), (~0U) << vfre_offset);
 	IXGBE_WRITE_REG(hw, IXGBE_VFRE(vfre_slot ^ 1), vfre_slot - 1);
-	IXGBE_WRITE_REG(hw, IXGBE_VFTE(vfre_slot), (~0) << vfre_offset);
+	IXGBE_WRITE_REG(hw, IXGBE_VFTE(vfre_slot), (~0U) << vfre_offset);
 	IXGBE_WRITE_REG(hw, IXGBE_VFTE(vfre_slot ^ 1), vfre_slot - 1);
 
 	/* PFDMA Tx General Switch Control Enables VMDQ loopback */
-- 
2.5.0

Re: [dpdk-dev] [PATCH 7/8] drivers/net/ixgbe: Signed left shift operator

[Panu Matilainen]

On 02/25/2016 08:48 PM, Aaron Conole wrote:
> Tell the compiler to use an unsigned constant for the config shifts.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   drivers/net/ixgbe/ixgbe_pf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_pf.c b/drivers/net/ixgbe/ixgbe_pf.c
> index 2ffbd1f..8b5119f 100644
> --- a/drivers/net/ixgbe/ixgbe_pf.c
> +++ b/drivers/net/ixgbe/ixgbe_pf.c
> @@ -236,9 +236,9 @@ int ixgbe_pf_host_configure(struct rte_eth_dev *eth_dev)
>   	vfre_slot = (vf_num >> VFRE_SHIFT) > 0 ? 1 : 0;
>
>   	/* Enable pools reserved to PF only */
> -	IXGBE_WRITE_REG(hw, IXGBE_VFRE(vfre_slot), (~0) << vfre_offset);
> +	IXGBE_WRITE_REG(hw, IXGBE_VFRE(vfre_slot), (~0U) << vfre_offset);
>   	IXGBE_WRITE_REG(hw, IXGBE_VFRE(vfre_slot ^ 1), vfre_slot - 1);
> -	IXGBE_WRITE_REG(hw, IXGBE_VFTE(vfre_slot), (~0) << vfre_offset);
> +	IXGBE_WRITE_REG(hw, IXGBE_VFTE(vfre_slot), (~0U) << vfre_offset);
>   	IXGBE_WRITE_REG(hw, IXGBE_VFTE(vfre_slot ^ 1), vfre_slot - 1);
>
>   	/* PFDMA Tx General Switch Control Enables VMDQ loopback */
>
Acked-by: Panu Matilainen <pmatilai@redhat.com>

CC'd the ixgbe maintainers...

	- Panu -

Re: [dpdk-dev] [PATCH 7/8] drivers/net/ixgbe: Signed left shift operator

[Zhang, Helin]

> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: [PATCH 7/8] drivers/net/ixgbe: Signed left shift operator
> 
> Tell the compiler to use an unsigned constant for the config shifts.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>

> ---
>  drivers/net/ixgbe/ixgbe_pf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_pf.c b/drivers/net/ixgbe/ixgbe_pf.c index
> 2ffbd1f..8b5119f 100644
> --- a/drivers/net/ixgbe/ixgbe_pf.c
> +++ b/drivers/net/ixgbe/ixgbe_pf.c
> @@ -236,9 +236,9 @@ int ixgbe_pf_host_configure(struct rte_eth_dev
> *eth_dev)
>  	vfre_slot = (vf_num >> VFRE_SHIFT) > 0 ? 1 : 0;
> 
>  	/* Enable pools reserved to PF only */
> -	IXGBE_WRITE_REG(hw, IXGBE_VFRE(vfre_slot), (~0) << vfre_offset);
> +	IXGBE_WRITE_REG(hw, IXGBE_VFRE(vfre_slot), (~0U) << vfre_offset);
>  	IXGBE_WRITE_REG(hw, IXGBE_VFRE(vfre_slot ^ 1), vfre_slot - 1);
> -	IXGBE_WRITE_REG(hw, IXGBE_VFTE(vfre_slot), (~0) << vfre_offset);
> +	IXGBE_WRITE_REG(hw, IXGBE_VFTE(vfre_slot), (~0U) << vfre_offset);
>  	IXGBE_WRITE_REG(hw, IXGBE_VFTE(vfre_slot ^ 1), vfre_slot - 1);
> 
>  	/* PFDMA Tx General Switch Control Enables VMDQ loopback */
> --
> 2.5.0

[dpdk-dev] [PATCH 8/8] drivers/net/ixgbe: Fix uninitialized warning

[Aaron Conole]

Silence a compiler warning that this variable may be used uninitialized.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index e95e6b7..775edc7 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -1563,7 +1563,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
 		struct ixgbe_rx_entry *rxe;
 		struct ixgbe_scattered_rx_entry *sc_entry;
 		struct ixgbe_scattered_rx_entry *next_sc_entry;
-		struct ixgbe_rx_entry *next_rxe;
+		struct ixgbe_rx_entry *next_rxe = NULL;
 		struct rte_mbuf *first_seg;
 		struct rte_mbuf *rxm;
 		struct rte_mbuf *nmb;
@@ -1740,7 +1740,7 @@ next_desc:
 		 * the pointer to the first mbuf at the NEXTP entry in the
 		 * sw_sc_ring and continue to parse the RX ring.
 		 */
-		if (!eop) {
+		if (!eop && next_rxe) {
 			rxm->next = next_rxe->mbuf;
 			next_sc_entry->fbuf = first_seg;
 			goto next_desc;
-- 
2.5.0

Re: [dpdk-dev] [PATCH 8/8] drivers/net/ixgbe: Fix uninitialized warning

[Panu Matilainen]

On 02/25/2016 08:48 PM, Aaron Conole wrote:
> Silence a compiler warning that this variable may be used uninitialized.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>   drivers/net/ixgbe/ixgbe_rxtx.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index e95e6b7..775edc7 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -1563,7 +1563,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
>   		struct ixgbe_rx_entry *rxe;
>   		struct ixgbe_scattered_rx_entry *sc_entry;
>   		struct ixgbe_scattered_rx_entry *next_sc_entry;
> -		struct ixgbe_rx_entry *next_rxe;
> +		struct ixgbe_rx_entry *next_rxe = NULL;
>   		struct rte_mbuf *first_seg;
>   		struct rte_mbuf *rxm;
>   		struct rte_mbuf *nmb;
> @@ -1740,7 +1740,7 @@ next_desc:
>   		 * the pointer to the first mbuf at the NEXTP entry in the
>   		 * sw_sc_ring and continue to parse the RX ring.
>   		 */
> -		if (!eop) {
> +		if (!eop && next_rxe) {
>   			rxm->next = next_rxe->mbuf;
>   			next_sc_entry->fbuf = first_seg;
>   			goto next_desc;
>

The patch looks ok as such, but then again warning looks like a false 
positive to me: assignment and dereferencing depend on the same value of 
eop, which cannot change between the two.

CC'ing the maintainers for attention...

	- Panu -

Re: [dpdk-dev] [PATCH 8/8] drivers/net/ixgbe: Fix uninitialized warning

[Remy Horton]

On 10/03/2016 13:42, Panu Matilainen wrote:
> On 02/25/2016 08:48 PM, Aaron Conole wrote:
>> Silence a compiler warning that this variable may be used uninitialized.
>>
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
[..]
>
> The patch looks ok as such, but then again warning looks like a false
> positive to me: assignment and dereferencing depend on the same value of
> eop, which cannot change between the two.

In two minds about this. It is a logical impossibility, but these days 
optimising compilers are getting very aggressive. For instance GCC has a 
delightfully-named -fdelete-null-pointer-checks option, which caused 
security holes..

..Remy

Re: [dpdk-dev] [PATCH 8/8] drivers/net/ixgbe: Fix uninitialized warning

[Panu Matilainen]

On 03/10/2016 04:45 PM, Remy Horton wrote:
>
> On 10/03/2016 13:42, Panu Matilainen wrote:
>> On 02/25/2016 08:48 PM, Aaron Conole wrote:
>>> Silence a compiler warning that this variable may be used uninitialized.
>>>
>>> Signed-off-by: Aaron Conole <aconole@redhat.com>
> [..]
>>
>> The patch looks ok as such, but then again warning looks like a false
>> positive to me: assignment and dereferencing depend on the same value of
>> eop, which cannot change between the two.
>
> In two minds about this. It is a logical impossibility, but these days
> optimising compilers are getting very aggressive. For instance GCC has a
> delightfully-named -fdelete-null-pointer-checks option, which caused
> security holes..

Indeed, that's why silencing a false positive (assuming it actually is 
one) by throwing some more NULL-checks for the allegedly impossible 
makes me a bit nervous. Besides compiler optimizations going crazy, I've 
seen such extra NULL-checks turn into actual bugs when surroundings 
subtly change.

	- Panu -

Re: [dpdk-dev] [PATCH 8/8] drivers/net/ixgbe: Fix uninitialized warning

[Remy Horton]

On 10/03/2016 15:03, Panu Matilainen wrote:
> On 03/10/2016 04:45 PM, Remy Horton wrote:
[...]
>> In two minds about this. It is a logical impossibility, but these days
>> optimising compilers are getting very aggressive. For instance GCC has a
>> delightfully-named -fdelete-null-pointer-checks option, which caused
>> security holes..
>
> Indeed, that's why silencing a false positive (assuming it actually is
> one) by throwing some more NULL-checks for the allegedly impossible
> makes me a bit nervous. Besides compiler optimizations going crazy, I've
> seen such extra NULL-checks turn into actual bugs when surroundings
> subtly change.

It cuts both ways. To anyone who is not an active compiler engineer, 
fixing a warning being /more/ likley to screw things up is quite a big 
thing. Do we want to turn off warnings or turn off optimisations.. :)

..Remy

Re: [dpdk-dev] [PATCH 8/8] drivers/net/ixgbe: Fix uninitialized warning

[Zhang, Helin]

> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Friday, February 26, 2016 2:49 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhang, Helin
> <helin.zhang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: [PATCH 8/8] drivers/net/ixgbe: Fix uninitialized warning
> 
> Silence a compiler warning that this variable may be used uninitialized.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
Acked-by: Helin Zhang <helin.zhang@intel.com>

> ---
>  drivers/net/ixgbe/ixgbe_rxtx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index e95e6b7..775edc7 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -1563,7 +1563,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf
> **rx_pkts, uint16_t nb_pkts,
>  		struct ixgbe_rx_entry *rxe;
>  		struct ixgbe_scattered_rx_entry *sc_entry;
>  		struct ixgbe_scattered_rx_entry *next_sc_entry;
> -		struct ixgbe_rx_entry *next_rxe;
> +		struct ixgbe_rx_entry *next_rxe = NULL;
>  		struct rte_mbuf *first_seg;
>  		struct rte_mbuf *rxm;
>  		struct rte_mbuf *nmb;
> @@ -1740,7 +1740,7 @@ next_desc:
>  		 * the pointer to the first mbuf at the NEXTP entry in the
>  		 * sw_sc_ring and continue to parse the RX ring.
>  		 */
> -		if (!eop) {
> +		if (!eop && next_rxe) {
>  			rxm->next = next_rxe->mbuf;
>  			next_sc_entry->fbuf = first_seg;
>  			goto next_desc;
> --
> 2.5.0

Re: [dpdk-dev] [PATCH 0/8] Various fixes to compile with gcc6

[Thomas Monjalon]

2016-02-25 13:48, Aaron Conole:
> This series brings a number of code cleanups to allow building using gcc6,
> with various legitimate warnings being fixed.
> 
> In particular, patch 3 ("drivers/net/e1000: Fix missing brackets") should be
> checked for correctness (it does not alter any behavior from a functional
> standpoint, but it may be required to do so for a correct fix).

Wenzhuo, Helin, Konstantin, Bruce, we need your opinion for some
of these patches.
Thanks

Re: [dpdk-dev] [PATCH 0/8] Various fixes to compile with gcc6

[Thomas Monjalon]

RE fixing email addresses...

2016-03-17 16:39, Thomas Monjalon:
> 2016-02-25 13:48, Aaron Conole:
> > This series brings a number of code cleanups to allow building using gcc6,
> > with various legitimate warnings being fixed.
> > 
> > In particular, patch 3 ("drivers/net/e1000: Fix missing brackets") should be
> > checked for correctness (it does not alter any behavior from a functional
> > standpoint, but it may be required to do so for a correct fix).
> 
> Wenzhuo, Helin, Konstantin, Bruce, we need your opinion for some
> of these patches.
> Thanks