[PATCH] IB/lmx4: silence GCC warning

[PATCH] IB/lmx4: silence GCC warning

[Paul Bolle]

Building qp.o (part of the "Mellanox ConnectX HCA support" driver)
triggers this GCC warning:
    drivers/infiniband/hw/mlx4/qp.c: In function ‘mlx4_ib_post_send’:
    drivers/infiniband/hw/mlx4/qp.c:1828:30: warning: ‘vlan’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    drivers/infiniband/hw/mlx4/qp.c:1718:6: note: ‘vlan’ was declared here

Looking at the code it is clear 'vlan' is only set and used if 'is_eth'
is non-zero. But there's no harm in initializing 'vlan' to 0 (which
matches ib_get_cached_gid()'s default return) to silence GCC.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
0) I noticed this warning while building v3.6-rc7 on current Fedora 17,
using Fedora's default config.

1) Compile tested only. I tested against v3.6-rc7, with commit
a41262bb5721f2b708ee8b23f67be2f2e16a2fab ("IB/mlx4: SR-IOV IB context
objects and proxy/tunnel SQP") from linux-next cherry-picked, to take
into account a trivial context change in linux-next.

 drivers/infiniband/hw/mlx4/qp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index a862251..71fdda6 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -1715,7 +1715,7 @@ static int build_mlx_header(struct mlx4_ib_sqp *sqp, struct ib_send_wr *wr,
 	int is_eth;
 	int is_vlan = 0;
 	int is_grh;
-	u16 vlan;
+	u16 vlan = 0;
 	int err = 0;
 
 	send_size = 0;
-- 
1.7.11.4

Re: [PATCH] IB/lmx4: silence GCC warning

[Jack Morgenstein]

You could use:

   u16 uninitialized_var(vlan);

instead.

Although this in the special QP data flow, I still prefer to avoid adding extra code (even setting
initial values at procedure entry).  The line above will also do the job.  "uninitialized_var"
is used elsewhere in the driver.  See, for example, mlx4_ib_post_send() in the same file (qp.c).

-Jack

On Friday 28 September 2012 14:48, Paul Bolle wrote:
> Building qp.o (part of the "Mellanox ConnectX HCA support" driver)
> triggers this GCC warning:
>     drivers/infiniband/hw/mlx4/qp.c: In function ‘mlx4_ib_post_send’:
>     drivers/infiniband/hw/mlx4/qp.c:1828:30: warning: ‘vlan’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>     drivers/infiniband/hw/mlx4/qp.c:1718:6: note: ‘vlan’ was declared here
> 
> Looking at the code it is clear 'vlan' is only set and used if 'is_eth'
> is non-zero. But there's no harm in initializing 'vlan' to 0 (which
> matches ib_get_cached_gid()'s default return) to silence GCC.
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
> 0) I noticed this warning while building v3.6-rc7 on current Fedora 17,
> using Fedora's default config.
> 
> 1) Compile tested only. I tested against v3.6-rc7, with commit
> a41262bb5721f2b708ee8b23f67be2f2e16a2fab ("IB/mlx4: SR-IOV IB context
> objects and proxy/tunnel SQP") from linux-next cherry-picked, to take
> into account a trivial context change in linux-next.
> 
>  drivers/infiniband/hw/mlx4/qp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
> index a862251..71fdda6 100644
> --- a/drivers/infiniband/hw/mlx4/qp.c
> +++ b/drivers/infiniband/hw/mlx4/qp.c
> @@ -1715,7 +1715,7 @@ static int build_mlx_header(struct mlx4_ib_sqp *sqp, struct ib_send_wr *wr,
>  	int is_eth;
>  	int is_vlan = 0;
>  	int is_grh;
> -	u16 vlan;
> +	u16 vlan = 0;
>  	int err = 0;
>  
>  	send_size = 0;

Re: [PATCH] IB/lmx4: silence GCC warning

[Paul Bolle]

On Wed, 2012-10-10 at 09:23 +0200, Jack Morgenstein wrote:
> You could use:
> 
>    u16 uninitialized_var(vlan);
> 
> instead.

I guess we'd better just wait and see whether uninitialized_var()
survives before discussing your suggestion (see the thread starting at
https://lkml.org/lkml/2012/10/26/508 ).

> Although this in the special QP data flow, I still prefer to avoid adding extra code (even setting
> initial values at procedure entry).  The line above will also do the job.  "uninitialized_var"
> is used elsewhere in the driver.  See, for example, mlx4_ib_post_send() in the same file (qp.c).


Paul Bolle

Re: [PATCH] IB/lmx4: silence GCC warning

[Bart Van Assche]

On 10/29/12 10:50, Paul Bolle wrote:
> On Wed, 2012-10-10 at 09:23 +0200, Jack Morgenstein wrote:
>> You could use:
>>
>>     u16 uninitialized_var(vlan);
>>
>> instead.
>
> I guess we'd better just wait and see whether uninitialized_var()
> survives before discussing your suggestion (see the thread starting at
> https://lkml.org/lkml/2012/10/26/508 ).
>
>> Although this in the special QP data flow, I still prefer to avoid adding extra code (even setting
>> initial values at procedure entry).  The line above will also do the job.  "uninitialized_var"
>> is used elsewhere in the driver.  See, for example, mlx4_ib_post_send() in the same file (qp.c).

(replying to an e-mail of a few months ago)

If there are no further objections I'd like to see this patch to go 
upstream. It fixes an annoying compiler warning and I don't think that 
this patch has a negative performance impact.

Thanks,

Bart.

[PATCH v2] IB/mlx4: silence GCC warning

[Paul Bolle]

Building qp.o (part of the "Mellanox ConnectX HCA support" driver)
triggers this GCC warning:
    drivers/infiniband/hw/mlx4/qp.c: In function ‘mlx4_ib_post_send’:
    drivers/infiniband/hw/mlx4/qp.c:1862:30: warning: ‘vlan’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    drivers/infiniband/hw/mlx4/qp.c:1752:6: note: ‘vlan’ was declared here

Looking at the code it is clear 'vlan' is only set and used if 'is_eth'
is non-zero. But by, basically, initializing 'vlan' to 0xffff, instead
of the equivalent of initializing 'is_vlan' to zero, we can make this
warning go away.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
0) Jack wanted to use uninitialized_var() after I posted the first
version of this trivial patch. I suggested to see what happened with
Ingo's idea to remove uninitialized_var() entirely. Nothing seems to
have happened, so I decided to try a different approach.

1) Still compile tested only, but now against v3.8.

 drivers/infiniband/hw/mlx4/qp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index 19e0637..512fde3 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -1747,9 +1747,9 @@ static int build_mlx_header(struct mlx4_ib_sqp *sqp, struct ib_send_wr *wr,
 	int spc;
 	int i;
 	int is_eth;
-	int is_vlan = 0;
+	int is_vlan;
 	int is_grh;
-	u16 vlan;
+	u16 vlan = 0xffff; /* invalid vlan id */
 	int err = 0;
 
 	send_size = 0;
@@ -1778,8 +1778,8 @@ static int build_mlx_header(struct mlx4_ib_sqp *sqp, struct ib_send_wr *wr,
 		}
 
 		vlan = rdma_get_vlan_id(&sgid);
-		is_vlan = vlan < 0x1000;
 	}
+	is_vlan = vlan < 0x1000;
 	ib_ud_header_init(send_size, !is_eth, is_eth, is_vlan, is_grh, 0, &sqp->ud_header);
 
 	if (!is_eth) {
-- 
1.7.11.7

Re: [PATCH v2] IB/mlx4: silence GCC warning

[Jack Morgenstein]

On Thursday 21 February 2013 11:02, Paul Bolle wrote:

> diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
> index 19e0637..512fde3 100644
> --- a/drivers/infiniband/hw/mlx4/qp.c
> +++ b/drivers/infiniband/hw/mlx4/qp.c
> @@ -1778,8 +1778,8 @@ static int build_mlx_header(struct mlx4_ib_sqp *sqp, struct ib_send_wr *wr,
>  		}
>  
>  		vlan = rdma_get_vlan_id(&sgid);
> -		is_vlan = vlan < 0x1000;
>  	}
Nice try!
However, this approach does add the line below to processing for an IB port (ETH/RoCE port stays same, more or less).
Processing time is therefore increased (at least on the IB side) relative to just living with the warning.

Roland?

> +	is_vlan = vlan < 0x1000;  <=== Code line added to IB-side processing.

>  	ib_ud_header_init(send_size, !is_eth, is_eth, is_vlan, is_grh, 0, &sqp->ud_header);
>  
>  	if (!is_eth) {

-Jack

Re: [PATCH v2] IB/mlx4: silence GCC warning

[Roland Dreier]

On Sun, Feb 24, 2013 at 4:34 AM, Jack Morgenstein
<jackm@dev.mellanox.co.il> wrote:
> However, this approach does add the line below to processing for an IB port (ETH/RoCE port stays same, more or less).
> Processing time is therefore increased (at least on the IB side) relative to just living with the warning.
>
> Roland?

I'm finally noticing that this is in the build_mlx_header() function,
which is pretty much a slow path.  Certainly another compare isn't
going to change performance given all the other stuff we do there.

Let me look at the patches that have gone by and see what the cleanest
way to handle this is.

 - R.

Re: [PATCH v2] IB/mlx4: silence GCC warning

[Roland Dreier]

On Mon, Feb 25, 2013 at 8:54 AM, Roland Dreier <roland@kernel.org> wrote:
> I'm finally noticing that this is in the build_mlx_header() function,
> which is pretty much a slow path.  Certainly another compare isn't
> going to change performance given all the other stuff we do there.
>
> Let me look at the patches that have gone by and see what the cleanest
> way to handle this is.

OK, after playing around a bit, I see that just initializing vlan
doesn't really change the generated code (my gcc at least was already
if effect setting vlan in the generated assembly code), so I'll just
merge that.

 - R.

Re: [PATCH v2] IB/mlx4: silence GCC warning

[Jack Morgenstein]

On Monday 25 February 2013 19:23, Roland Dreier wrote:
> On Mon, Feb 25, 2013 at 8:54 AM, Roland Dreier <roland@kernel.org> wrote:
> > I'm finally noticing that this is in the build_mlx_header() function,
> > which is pretty much a slow path.  Certainly another compare isn't
> > going to change performance given all the other stuff we do there.
> >
> > Let me look at the patches that have gone by and see what the cleanest
> > way to handle this is.
> 
> OK, after playing around a bit, I see that just initializing vlan
> doesn't really change the generated code (my gcc at least was already
> if effect setting vlan in the generated assembly code), so I'll just
> merge that.
> 
>  - R.

Thanks!

-Jack