[dpdk-dev] [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
2016-03-24 08:54, Panu Matilainen: > --- a/drivers/net/e1000/Makefile > +++ b/drivers/net/e1000/Makefile > @@ -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?
[dpdk-dev] [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
Thomas Monjalon writes: > 2016-03-24 08:54, Panu Matilainen: >> --- a/drivers/net/e1000/Makefile >> +++ b/drivers/net/e1000/Makefile >> @@ -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? 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
[dpdk-dev] [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
On 03/24/2016 02:35 AM, Lu, Wenzhuo wrote: > Hi Thomas, Aaron, >>> 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) --- a/drivers/net/e1000/Makefile +++ b/drivers/net/e1000/Makefile @@ -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 -
[dpdk-dev] [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
Hi Thomas, Aaron, > > 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.
[dpdk-dev] [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
fixing Wenzhuo email address...
2016-03-23 11:38, Thomas Monjalon:
> 2016-03-22 17:37, 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.
> >
> > Fixes commit 38db3f7f50bd ("e1000: update base driver")
> >
> > Signed-off-by: Aaron Conole
> > ---
> > v2:
> > * Changed from "whitespace-only" fix to a functional change
> > moving the phy writes into protection of the `if (locked)`
> > code
> > * Added "Fixes" line.
>
> Wenzhuo, do you agree to fix the base driver here?
[dpdk-dev] [PATCH v2 2/7] drivers/net/e1000: Fix missing brackets
2016-03-22 17:37, 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.
>
> Fixes commit 38db3f7f50bd ("e1000: update base driver")
>
> Signed-off-by: Aaron Conole
> ---
> v2:
> * Changed from "whitespace-only" fix to a functional change
> moving the phy writes into protection of the `if (locked)`
> code
> * Added "Fixes" line.
Wenzhuo, do you agree to fix the base driver here?
[dpdk-dev] [PATCH v2 2/7] 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.
Fixes commit 38db3f7f50bd ("e1000: update base driver")
Signed-off-by: Aaron Conole
---
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(-)
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;
}
--
2.8.0.rc2.35.gc2c5f6b.dirty
