[PATCH 1/2] net/mlx4_core: clean up cq_res_start_move_to()

[PATCH 1/2] net/mlx4_core: clean up cq_res_start_move_to()

[Paul Bolle]

Building resource_tracker.o triggers a GCC warning:
    drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function 'mlx4_HW2SW_CQ_wrapper':
    drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3019:16: warning: 'cq' may be used uninitialized in this function [-Wmaybe-uninitialized]
      atomic_dec(&cq->mtt->ref_count);
                    ^

This is a false positive. But a cleanup of cq_res_start_move_to() can
help GCC here. The code currently uses a switch statement where a plain
if/else would do, since only two of the switch's four cases can ever
occur. Dropping that switch makes the warning go away.

While we're at it, do some coding style cleanups (missing braces), and
drop a test that always evaluates to true.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
 .../net/ethernet/mellanox/mlx4/resource_tracker.c  | 51 ++++++++--------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
index 2f3f2bc..a41f01e 100644
--- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
+++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
@@ -1340,43 +1340,30 @@ static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn,
 
 	spin_lock_irq(mlx4_tlock(dev));
 	r = res_tracker_lookup(&tracker->res_tree[RES_CQ], cqn);
-	if (!r)
+	if (!r) {
 		err = -ENOENT;
-	else if (r->com.owner != slave)
+	} else if (r->com.owner != slave) {
 		err = -EPERM;
-	else {
-		switch (state) {
-		case RES_CQ_BUSY:
+	} else if (state == RES_CQ_ALLOCATED) {
+		if (r->com.state != RES_CQ_HW)
+			err = -EINVAL;
+		else if (atomic_read(&r->ref_count))
 			err = -EBUSY;
-			break;
-
-		case RES_CQ_ALLOCATED:
-			if (r->com.state != RES_CQ_HW)
-				err = -EINVAL;
-			else if (atomic_read(&r->ref_count))
-				err = -EBUSY;
-			else
-				err = 0;
-			break;
-
-		case RES_CQ_HW:
-			if (r->com.state != RES_CQ_ALLOCATED)
-				err = -EINVAL;
-			else
-				err = 0;
-			break;
-
-		default:
+		else
+			err = 0;
+	} else {
+		/* state == RES_CQ_HW */
+		if (r->com.state != RES_CQ_ALLOCATED)
 			err = -EINVAL;
-		}
+		else
+			err = 0;
+	}
 
-		if (!err) {
-			r->com.from_state = r->com.state;
-			r->com.to_state = state;
-			r->com.state = RES_CQ_BUSY;
-			if (cq)
-				*cq = r;
-		}
+	if (!err) {
+		r->com.from_state = r->com.state;
+		r->com.to_state = state;
+		r->com.state = RES_CQ_BUSY;
+		*cq = r;
 	}
 
 	spin_unlock_irq(mlx4_tlock(dev));
-- 
1.8.4.2

Re: [PATCH 1/2] net/mlx4_core: clean up cq_res_start_move_to()

[Or Gerlitz]

On 07/01/2014 15:01, Paul Bolle wrote:
> Building resource_tracker.o triggers a GCC warning:
>      drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function 'mlx4_HW2SW_CQ_wrapper':
>      drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3019:16: warning: 'cq' may be used uninitialized in this function [-Wmaybe-uninitialized]
>        atomic_dec(&cq->mtt->ref_count);
>                      ^
>
> This is a false positive. But a cleanup of cq_res_start_move_to() can
> help GCC here. The code currently uses a switch statement where a plain
> if/else would do, since only two of the switch's four cases can ever
> occur. Dropping that switch makes the warning go away.
>
> While we're at it, do some coding style cleanups (missing braces), and
> drop a test that always evaluates to true.
>

Hi Paul,

Our maintainer of that area of the code (SRIOV resource tracker) is busy 
now, but we will definitely look on these two patches in the coming 
days, thanks for posting them!

Re: [PATCH 1/2] net/mlx4_core: clean up cq_res_start_move_to()

[Jack Morgenstein]

This time, replying to the list as well.

-Jack
P.S. sorry for the delay, I was swamped.

On Tue, 07 Jan 2014 14:01:18 +0100
Paul Bolle <pebolle@tiscali.nl> wrote:

> +	} else {
> +		/* state == RES_CQ_HW */
> +		if (r->com.state != RES_CQ_ALLOCATED)

if (state != RES_CQ_HW || r->com.state != RES_CQ_ALLOCATED)
to protect against any bad calls to this function
(although I know that currently there are none).

This also preserves the behavior of the switch statement.

>  			err = -EINVAL;
> -		}
> +		else
> +			err = 0;
> +	}
>  
> -		if (!err) {
> -			r->com.from_state = r->com.state;
> -			r->com.to_state = state;
> -			r->com.state = RES_CQ_BUSY;
> -			if (cq)
> -				*cq = r;
> -		}
> +	if (!err) {
> +		r->com.from_state = r->com.state;
> +		r->com.to_state = state;
> +		r->com.state = RES_CQ_BUSY;

Please keep the "if" here.  Protects against (future) bad calls.

> +		*cq = r;
>  	}

Re: [PATCH 1/2] net/mlx4_core: clean up cq_res_start_move_to()

[Paul Bolle]

On Tue, 2014-01-14 at 08:47 +0200, Jack Morgenstein wrote:
> On Tue, 07 Jan 2014 14:01:18 +0100
> Paul Bolle <pebolle@tiscali.nl> wrote:
> 
> > +	} else {
> > +		/* state == RES_CQ_HW */
> > +		if (r->com.state != RES_CQ_ALLOCATED)
> 
> if (state != RES_CQ_HW || r->com.state != RES_CQ_ALLOCATED)
> to protect against any bad calls to this function
> (although I know that currently there are none).

So we end up with
         } else if (state != RES_CQ_HW || r->com.state != RES_CQ_ALLOCATED) {
                 err = -EINVAL;
         } else {
                 err = 0;
         }

don't we? Which is fine with me, as GCC still is then able to correctly
analyze this function.

> This also preserves the behavior of the switch statement.
> 
> >  			err = -EINVAL;
> > -		}
> > +		else
> > +			err = 0;
> > +	}
> >  
> > -		if (!err) {
> > -			r->com.from_state = r->com.state;
> > -			r->com.to_state = state;
> > -			r->com.state = RES_CQ_BUSY;
> > -			if (cq)
> > -				*cq = r;
> > -		}
> > +	if (!err) {
> > +		r->com.from_state = r->com.state;
> > +		r->com.to_state = state;
> > +		r->com.state = RES_CQ_BUSY;
> 
> Please keep the "if" here.  Protects against (future) bad calls.
> 
> > +		*cq = r;
> >  	}

There seems to be a school of thought that says it's better to trigger
an Oops if a programming error is made (in this case by passing a NULL
cq) then silently handle that (future) programming error and make
debugging harder. But, even it that school of thought really exists,
this is up to you. Besides, it's only a triviality I added to my
patches.

Thanks for the review! I hope to send in a v2 of my patches shortly.


Paul Bolle

[PATCH v2 1/2] net/mlx4_core: clean up cq_res_start_move_to()

[Paul Bolle]

Building resource_tracker.o triggers a GCC warning:
    drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function 'mlx4_HW2SW_CQ_wrapper':
    drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3019:16: warning: 'cq' may be used uninitialized in this function [-Wmaybe-uninitialized]
      atomic_dec(&cq->mtt->ref_count);
                    ^

This is a false positive. But a cleanup of cq_res_start_move_to() can
help GCC here. The code currently uses a switch statement where an
if/else construct would do too, since only two of the switch's four
cases can ever occur. Dropping that switch makes the warning go away.

While we're at it, add some missing braces.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
v2: adjust to Jack's review.

 .../net/ethernet/mellanox/mlx4/resource_tracker.c  | 52 ++++++++--------------
 1 file changed, 19 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
index 2f3f2bc..15cd659 100644
--- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
+++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
@@ -1340,43 +1340,29 @@ static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn,
 
 	spin_lock_irq(mlx4_tlock(dev));
 	r = res_tracker_lookup(&tracker->res_tree[RES_CQ], cqn);
-	if (!r)
+	if (!r) {
 		err = -ENOENT;
-	else if (r->com.owner != slave)
+	} else if (r->com.owner != slave) {
 		err = -EPERM;
-	else {
-		switch (state) {
-		case RES_CQ_BUSY:
-			err = -EBUSY;
-			break;
-
-		case RES_CQ_ALLOCATED:
-			if (r->com.state != RES_CQ_HW)
-				err = -EINVAL;
-			else if (atomic_read(&r->ref_count))
-				err = -EBUSY;
-			else
-				err = 0;
-			break;
-
-		case RES_CQ_HW:
-			if (r->com.state != RES_CQ_ALLOCATED)
-				err = -EINVAL;
-			else
-				err = 0;
-			break;
-
-		default:
+	} else if (state == RES_CQ_ALLOCATED) {
+		if (r->com.state != RES_CQ_HW)
 			err = -EINVAL;
-		}
+		else if (atomic_read(&r->ref_count))
+			err = -EBUSY;
+		else
+			err = 0;
+	} else if (state != RES_CQ_HW || r->com.state != RES_CQ_ALLOCATED) {
+		err = -EINVAL;
+	} else {
+		err = 0;
+	}
 
-		if (!err) {
-			r->com.from_state = r->com.state;
-			r->com.to_state = state;
-			r->com.state = RES_CQ_BUSY;
-			if (cq)
-				*cq = r;
-		}
+	if (!err) {
+		r->com.from_state = r->com.state;
+		r->com.to_state = state;
+		r->com.state = RES_CQ_BUSY;
+		if (cq)
+			*cq = r;
 	}
 
 	spin_unlock_irq(mlx4_tlock(dev));
-- 
1.8.4.2

Re: [PATCH v2 1/2] net/mlx4_core: clean up cq_res_start_move_to()

[David Miller]

From: Paul Bolle <pebolle@tiscali.nl>
Date: Tue, 14 Jan 2014 20:45:36 +0100

> Building resource_tracker.o triggers a GCC warning:
>     drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function 'mlx4_HW2SW_CQ_wrapper':
>     drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3019:16: warning: 'cq' may be used uninitialized in this function [-Wmaybe-uninitialized]
>       atomic_dec(&cq->mtt->ref_count);
>                     ^
> 
> This is a false positive. But a cleanup of cq_res_start_move_to() can
> help GCC here. The code currently uses a switch statement where an
> if/else construct would do too, since only two of the switch's four
> cases can ever occur. Dropping that switch makes the warning go away.
> 
> While we're at it, add some missing braces.
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
> v2: adjust to Jack's review.

Can I please get some review of these two patches, thank you.

Re: [PATCH v2 1/2] net/mlx4_core: clean up cq_res_start_move_to()

[Jack Morgenstein]

ACK.  OK.

-Jack

On Tue, 14 Jan 2014 20:45:36 +0100
Paul Bolle <pebolle@tiscali.nl> wrote:

> Building resource_tracker.o triggers a GCC warning:
>     drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In
> function 'mlx4_HW2SW_CQ_wrapper':
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3019:16:
> warning: 'cq' may be used uninitialized in this function
> [-Wmaybe-uninitialized] atomic_dec(&cq->mtt->ref_count); ^
> 
> This is a false positive. But a cleanup of cq_res_start_move_to() can
> help GCC here. The code currently uses a switch statement where an
> if/else construct would do too, since only two of the switch's four
> cases can ever occur. Dropping that switch makes the warning go away.
> 
> While we're at it, add some missing braces.
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
> v2: adjust to Jack's review.
> 
>  .../net/ethernet/mellanox/mlx4/resource_tracker.c  | 52
> ++++++++-------------- 1 file changed, 19 insertions(+), 33
> deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c index
> 2f3f2bc..15cd659 100644 ---
> a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +++
> b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c @@ -1340,43
> +1340,29 @@ static int cq_res_start_move_to(struct mlx4_dev *dev, int
> slave, int cqn, spin_lock_irq(mlx4_tlock(dev));
>  	r = res_tracker_lookup(&tracker->res_tree[RES_CQ], cqn);
> -	if (!r)
> +	if (!r) {
>  		err = -ENOENT;
> -	else if (r->com.owner != slave)
> +	} else if (r->com.owner != slave) {
>  		err = -EPERM;
> -	else {
> -		switch (state) {
> -		case RES_CQ_BUSY:
> -			err = -EBUSY;
> -			break;
> -
> -		case RES_CQ_ALLOCATED:
> -			if (r->com.state != RES_CQ_HW)
> -				err = -EINVAL;
> -			else if (atomic_read(&r->ref_count))
> -				err = -EBUSY;
> -			else
> -				err = 0;
> -			break;
> -
> -		case RES_CQ_HW:
> -			if (r->com.state != RES_CQ_ALLOCATED)
> -				err = -EINVAL;
> -			else
> -				err = 0;
> -			break;
> -
> -		default:
> +	} else if (state == RES_CQ_ALLOCATED) {
> +		if (r->com.state != RES_CQ_HW)
>  			err = -EINVAL;
> -		}
> +		else if (atomic_read(&r->ref_count))
> +			err = -EBUSY;
> +		else
> +			err = 0;
> +	} else if (state != RES_CQ_HW || r->com.state !=
> RES_CQ_ALLOCATED) {
> +		err = -EINVAL;
> +	} else {
> +		err = 0;
> +	}
>  
> -		if (!err) {
> -			r->com.from_state = r->com.state;
> -			r->com.to_state = state;
> -			r->com.state = RES_CQ_BUSY;
> -			if (cq)
> -				*cq = r;
> -		}
> +	if (!err) {
> +		r->com.from_state = r->com.state;
> +		r->com.to_state = state;
> +		r->com.state = RES_CQ_BUSY;
> +		if (cq)
> +			*cq = r;
>  	}
>  
>  	spin_unlock_irq(mlx4_tlock(dev));

Re: [PATCH v2 1/2] net/mlx4_core: clean up cq_res_start_move_to()

[David Miller]

From: Jack Morgenstein <jackm@dev.mellanox.co.il>
Date: Thu, 16 Jan 2014 09:46:39 +0200

> ACK.  OK.

This is not the correct way to ack a patch.

First of all, you should not top post.  Instead you should quote
the relevant parts of the email you are replying to, and then add
your new content underneath.

In this circumstance, the "relevant parts" are just the commit log
message from the patch submitter.  There is no reason ever to quote
the patch itself unless you are making comments on specific parts.

And you specify your ACK using a properly formed "Acked-by: "
line.

Please look at how other reviewers ACK patches.

Thank you.

Re: [PATCH v2 1/2] net/mlx4_core: clean up cq_res_start_move_to()

[David Miller]

From: Paul Bolle <pebolle@tiscali.nl>
Date: Tue, 14 Jan 2014 20:45:36 +0100

> Building resource_tracker.o triggers a GCC warning:
>     drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function 'mlx4_HW2SW_CQ_wrapper':
>     drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3019:16: warning: 'cq' may be used uninitialized in this function [-Wmaybe-uninitialized]
>       atomic_dec(&cq->mtt->ref_count);
>                     ^
> 
> This is a false positive. But a cleanup of cq_res_start_move_to() can
> help GCC here. The code currently uses a switch statement where an
> if/else construct would do too, since only two of the switch's four
> cases can ever occur. Dropping that switch makes the warning go away.
> 
> While we're at it, add some missing braces.
> 
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>

Applied.