Various fixes to compile with gcc6
v2 cover Various fixes to compile with gcc6
This series brings a number of code cleanups to allow building using gcc6, with various legitimate warnings being fixed.
Some of these fixes are to the drivers area, making this series a bit atypical. However, the fixes identified in patches 2 and 3 are actual bugs and should be fixed.
The first patch from the original series has been dropped. It is no longer needed, after commit 5ecdeba601d1 ("lpm: merge tbl24 and tbl8 structures").
Aaron Conole (7):
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 ++-- 7 files changed, 18 insertions(+), 15 deletions(-)
Signature · 1 line
2.8.0.rc2.35.gc2c5f6b.dirty
PATCH v2 1/7 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.
---
v2:
* No change (apart from adding ACKs)
app/test/test.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
app/test/test.c +2 -1
@@ -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 )
Signature · 1 line
2.8.0.rc2.35.gc2c5f6b.dirty
PATCH v2 2/7 drivers/net/e1000: Fix missing brackets
In reply to Aaron ConoleThe register read/write mphy functions have misleading whitespace around the locked check. This cleanup merely preserves the existing functionality while improving the ready check.
Fixes commit 38db3f7f50bd ("e1000: update base driver")
---
v2:
* Changed from "whitespace-only" fix to a functional change moving the phy writes into protection of the if (locked) code
* Added "Fixes" line.
drivers/net/e1000/base/e1000_phy.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/net/e1000/base/e1000_phy.c +6 -6
@@ -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 ; }
Signature · 1 line
2.8.0.rc2.35.gc2c5f6b.dirty
Re: [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
2016-03-22 17:37, Aaron Conole:
Quoted 6 lines
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.
Fixes commit
38db3f7f50bd("e1000: update base driver")
Quoted 7 lines
Signed-off-by Aaron Conole <[email protected]>---
v2:
* Changed from "whitespace-only" fix to a functional change moving the phy writes into protection of theif (locked)code
* Added "Fixes" line.
Wenzhuo, do you agree to fix the base driver here?
Re: [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
fixing Wenzhuo email address...
2016-03-23 11:38, Thomas Monjalon:
Quoted 16 lines
2016-03-22 17:37, Aaron Conole:
Quoted 6 lines
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.
Fixes commit
38db3f7f50bd("e1000: update base driver")Quoted 7 lines
Signed-off-by Aaron Conole <[email protected]>---
v2:
* Changed from "whitespace-only" fix to a functional change moving the phy writes into protection of theif (locked)code
* Added "Fixes" line.Wenzhuo, do you agree to fix the base driver here?
Re: [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
Hi Thomas, Aaron,
Quoted 1 line
Quoted 1 line
Wenzhuo, do you agree to fix the base driver here?
Honestly I find the logic has some problem and maybe more changes needed. I'm talking with our kernel driver contactors to make sure they have no concern about my idea. And Aaron, do we really need to fix this code? For I find this function is not called. So it has no real impact to us. Could we just wait until kernel driver fixes it and leverage the new version? Thanks.
Re: [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
Quoted 4 lines · Wenzhuo
Hi Thomas, Aaron,
Quoted 1 line
Quoted 1 line
Wenzhuo, do you agree to fix the base driver here?
Honestly I find the logic has some problem and maybe more changes needed. I'm talking with our kernel driver contactors to make sure they have no concern about my idea. And Aaron, do we really need to fix this code? For I find this function is not called. So it has no real impact to us. Could we just wait until kernel driver fixes it and leverage the new version? Thanks.
It breaks the build with gcc >= 6.0 due to -Werror so yes we need to fix it, one way or the other.
As spelled out in an earlier mail (http://dpdk.org/ml/archives/dev/2016-March/034290.html), for somebody building with gcc >= 6.0 the options basically are:
1) disable the driver entirely
2) build with -Wno-error
3) paper over it with local warning disablers
4) patch the thing locally
If the offending function really is not used at all in DPDK context then 3) is an entirely valid option for an upstream solution, ie something like (untested)
drivers/net/e1000/Makefile +3 -0
@@ -54,6 +54,9 @@ else # CFLAGS_BASE_DRIVER = -Wno-uninitialized -Wno-unused-parameter CFLAGS_BASE_DRIVER += -Wno-unused-variable +ifeq ($(shell test $(GCC_VERSION) -ge 60 && echo 1), 1) +CFLAGS_BASE_DRIVER += -Wno-misleading-indentation +endif endif #
- Panu -
Re: [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
2016-03-24 08:54, Panu Matilainen:
drivers/net/e1000/Makefile else · line 59
Quoted 9 lines
drivers/net/e1000/Makefile +3
-0@@ -54,6 +54,9 @@ else # CFLAGS_BASE_DRIVER = -Wno-uninitialized -Wno-unused-parameter CFLAGS_BASE_DRIVER += -Wno-unused-variable +ifeq ($(shell test $(GCC_VERSION) -ge 60 && echo 1), 1) +CFLAGS_BASE_DRIVER += -Wno-misleading-indentation +endif
Aaron, have you tested this solution?
Are you going to provide a v3?
Re: [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
Quoted 13 lines · Thomas Monjalon
2016-03-24 08:54, Panu Matilainen:
drivers/net/e1000/Makefile else · line 59
Quoted 9 lines
drivers/net/e1000/Makefile +3
-0@@ -54,6 +54,9 @@ else # CFLAGS_BASE_DRIVER = -Wno-uninitialized -Wno-unused-parameter CFLAGS_BASE_DRIVER += -Wno-unused-variable +ifeq ($(shell test $(GCC_VERSION) -ge 60 && echo 1), 1) +CFLAGS_BASE_DRIVER += -Wno-misleading-indentation +endifAaron, have you tested this solution?
Are you going to provide a v3?
I haven't yet tested this solution, but if folks are that opposed to changing the code, then I will test it and post a v3 of this particular patch in the series.
Thanks so much for the reviews and time on this (Panu AND Thomas :-)), -Aaron
PATCH v3 2/7 drivers/net/e1000: Suppress misleading indentation warning
In reply to Thomas MonjalonThe register read/write mphy functions have misleading whitespace around the locked check. This cleanup merely preserves the existing functionality and suppresses future gcc versions' "misleading indentation" warning.
---
v2:
* Changed from "whitespace-only" fix to a functional change moving the phy writes into protection of the if (locked) code
* Added "Fixes" line.
v3:
* Instead of changing the code, change to suppress the compiler warning when using gcc6+. This was tested with both gcc6 and gcc5 using gnu make 4.0 and gnu bash 4.3.42 on a fedora 23 system.
drivers/net/e1000/Makefile | 3 +++ 1 file changed, 3 insertions(+)
drivers/net/e1000/Makefile +3 -0
@@ -54,6 +54,9 @@ else # CFLAGS_BASE_DRIVER = -Wno-uninitialized -Wno-unused-parameter CFLAGS_BASE_DRIVER += -Wno-unused-variable +ifeq ($(shell test $(GCC_VERSION) -ge 60 && echo 1), 1) +CFLAGS_BASE_DRIVER += -Wno-misleading-indentation +endif endif #
Signature · 1 line
2.5.5
Re: [PATCH v3 2/7] drivers/net/e1000: Suppress misleading indentation warning
Quoted 26 lines · On Wed, 30 Mar 2016 10:06:36 -0400 Aaron Conole <[email protected]> wrote:
The register read/write mphy functions have misleading whitespace around the
lockedcheck. This cleanup merely preserves the existing functionality and suppresses future gcc versions' "misleading indentation" warning.Suggested-by Panu Matilainen <[email protected]>Signed-off-by Aaron Conole <[email protected]>---
v2:
* Changed from "whitespace-only" fix to a functional change moving the phy writes into protection of theif (locked)code
* Added "Fixes" line.v3:
* Instead of changing the code, change to suppress the compiler warning when using gcc6+. This was tested with both gcc6 and gcc5 using gnu make 4.0 and gnu bash 4.3.42 on a fedora 23 system.drivers/net/e1000/Makefile | 3 +++ 1 file changed, 3 insertions(+)drivers/net/e1000/Makefile +0
-0
drivers/net/e1000/Makefile else · line 62
Quoted 9 lines
drivers/net/e1000/Makefile +3
-0@@ -54,6 +54,9 @@ else # CFLAGS_BASE_DRIVER = -Wno-uninitialized -Wno-unused-parameter CFLAGS_BASE_DRIVER += -Wno-unused-variable +ifeq ($(shell test $(GCC_VERSION) -ge 60 && echo 1), 1) +CFLAGS_BASE_DRIVER += -Wno-misleading-indentation +endif endif #
NAK, don't do it to the whole file.
Fix the code (best option)
or use a pragma for the small area which is broken for other reasons.
Re: [PATCH v3 2/7] drivers/net/e1000: Suppress misleading indentation warning
2016-03-30 09:36, Stephen Hemminger:
Quoted 16 lines
drivers/net/e1000/Makefile else · line 60
Quoted 10 lines · On Wed, 30 Mar 2016 10:06:36 -0400 Aaron Conole <[email protected]> wrote:
drivers/net/e1000/Makefile +3
-0@@ -54,6 +54,9 @@ else # CFLAGS_BASE_DRIVER = -Wno-uninitialized -Wno-unused-parameter CFLAGS_BASE_DRIVER += -Wno-unused-variable +ifeq ($(shell test $(GCC_VERSION) -ge 60 && echo 1), 1) +CFLAGS_BASE_DRIVER += -Wno-misleading-indentation +endif endifNAK, don't do it to the whole file.
Fix the code (best option)
or use a pragma for the small area which is broken for other reasons.
Stephen, your solutions do not work because Aaron has not been allowed to modify this file.
As long as we are not allowed to modify the Intel base drivers, I don't see any problem to hide some warnings in them.
The warnings could help us to clean the code or fix some bugs but we are not allowed to...
It is the responsibility of the driver maintainer to keep this nasty code.
Re: [PATCH v3 2/7] drivers/net/e1000: Suppress misleading indentation warning
Quoted 18 lines · On Wed, 30 Mar 2016 19:12:39 +0200 Thomas Monjalon <[email protected]>…
2016-03-30 09:36, Stephen Hemminger:
Quoted 16 lines
drivers/net/e1000/Makefile else · line 60
Quoted 10 lines · On Wed, 30 Mar 2016 10:06:36 -0400 Aaron Conole <[email protected]> wrote:
drivers/net/e1000/Makefile +3
-0@@ -54,6 +54,9 @@ else # CFLAGS_BASE_DRIVER = -Wno-uninitialized -Wno-unused-parameter CFLAGS_BASE_DRIVER += -Wno-unused-variable +ifeq ($(shell test $(GCC_VERSION) -ge 60 && echo 1), 1) +CFLAGS_BASE_DRIVER += -Wno-misleading-indentation +endif endifNAK, don't do it to the whole file.
Fix the code (best option)
or use a pragma for the small area which is broken for other reasons.
Quoted 7 lines
Stephen, your solutions do not work because Aaron has not been allowed to modify this file.
As long as we are not allowed to modify the Intel base drivers, I don't see any problem to hide some warnings in them.
The warnings could help us to clean the code or fix some bugs but we are not allowed to...
It is the responsibility of the driver maintainer to keep this nasty code.
ok, but the policy of "base drivers are allowed to be unmaintainable" is an albatross around the neck of DPDK. There is a reason such a policy was rejected in Linux.
Re: [PATCH v3 2/7] drivers/net/e1000: Suppress misleading indentation warning
In reply to Aaron ConoleHi,
Quoted 8 lines
-----Original Message-----
From: Aaron Conole [mailto:[email protected]]
Sent: Wednesday, March 30, 2016 10:07 PM
To: Thomas Monjalon
Cc: Panu Matilainen; Lu, Wenzhuo; [email protected]; Richardson, Bruce
Subject: [PATCH v3 2/7]drivers/net/e1000: Suppress misleading indentation warning
Quoted 6 lines
The register read/write mphy functions have misleading whitespace around the
lockedcheck. This cleanup merely preserves the existing functionality and suppresses future gcc versions' "misleading indentation" warning.Suggested-by Panu Matilainen <[email protected]>Signed-off-by Aaron Conole <[email protected]>
PATCH v2 3/7 drivers/net/e1000: Fix missing lsc interrupt check brackets
In reply to Aaron ConoleThe device lsc interrupt 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.
Fixes commit 921a72008f76 ("e1000: add Rx interrupt handler")
---
v2:
* Added Fixes line.
drivers/net/e1000/em_ethdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/e1000/em_ethdev.c +2 -1
@@ -666,13 +666,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,
Signature · 1 line
2.8.0.rc2.35.gc2c5f6b.dirty
PATCH v2 4/7 drivers/net/ixgbe: Fix vlan filter missing brackets
In reply to Aaron ConoleThe ixgbe vlan filter code has an if check with an incorrect whitespace.
---
v2:
* No change (apart from adding ACKs)
drivers/net/ixgbe/ixgbe_ethdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ixgbe/ixgbe_ethdev.c +2 -1
@@ -4377,10 +4377,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;
Signature · 1 line
2.8.0.rc2.35.gc2c5f6b.dirty
PATCH v2 5/7 drivers/net/e1000/igb: Signed left shift operator
In reply to Aaron ConoleTell the compiler to use an unsigned constant for the config shifts.
---
v2:
* No change (apart from adding ACKs)
drivers/net/e1000/igb_pf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/e1000/igb_pf.c +2 -2
@@ -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)
Signature · 1 line
2.8.0.rc2.35.gc2c5f6b.dirty
PATCH v2 6/7 drivers/net/ixgbe: Signed left shift operator
In reply to Aaron ConoleTell the compiler to use an unsigned constant for the config shifts.
---
v2:
* No change (apart from adding ACKs)
drivers/net/ixgbe/ixgbe_pf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ixgbe/ixgbe_pf.c +2 -2
@@ -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 */
Signature · 1 line
2.8.0.rc2.35.gc2c5f6b.dirty
PATCH v2 7/7 drivers/net/ixgbe: Fix uninitialized warning
In reply to Aaron ConoleSilence a compiler warning that this variable may be used uninitialized.
---
v2:
* No change (apart from adding ACK)
drivers/net/ixgbe/ixgbe_rxtx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ixgbe/ixgbe_rxtx.c +2 -2
@@ -1614,7 +1614,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;
@@ -1791,7 +1791,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;
Signature · 1 line
2.8.0.rc2.35.gc2c5f6b.dirty
2016-03-22 17:37, Aaron Conole:
Quoted 12 lines
This series brings a number of code cleanups to allow building using gcc6, with various legitimate warnings being fixed.
Some of these fixes are to the drivers area, making this series a bit atypical. However, the fixes identified in patches 2 and 3 are actual bugs and should be fixed.
The first patch from the original series has been dropped. It is no longer needed, after commit
5ecdeba601d1("lpm: merge tbl24 and tbl8 structures").
Quoted 8 lines
Aaron Conole (7):
app/test/test: Fix missing bracketsdrivers/net/e1000: Fix missing bracketsdrivers/net/e1000: Fix missing lsc interrupt check bracketsdrivers/net/ixgbe: Fix vlan filter missing bracketsdrivers/net/e1000/igb: Signed left shift operatordrivers/net/ixgbe: Signed left shift operatordrivers/net/ixgbe: Fix uninitialized warning
Applied with v3 of patch 2, thanks
Quoted 22 lines · Thomas Monjalon
2016-03-22 17:37, Aaron Conole:
Quoted 12 lines
This series brings a number of code cleanups to allow building using gcc6, with various legitimate warnings being fixed.
Some of these fixes are to the drivers area, making this series a bit atypical. However, the fixes identified in patches 2 and 3 are actual bugs and should be fixed.
The first patch from the original series has been dropped. It is no longer needed, after commit
5ecdeba601d1("lpm: merge tbl24 and tbl8 structures").Quoted 8 lines
Aaron Conole (7):
app/test/test: Fix missing bracketsdrivers/net/e1000: Fix missing bracketsdrivers/net/e1000: Fix missing lsc interrupt check bracketsdrivers/net/ixgbe: Fix vlan filter missing bracketsdrivers/net/e1000/igb: Signed left shift operatordrivers/net/ixgbe: Signed left shift operatordrivers/net/ixgbe: Fix uninitialized warning
Quoted 2 lines
Applied with v3 of patch 2, thanks
...and so we finally have a warning-free build with gcc 6.
Thanks,
- Panu -