Skip to contentSkip to conversation

net/mlx4_core: clean up cq_res_start_move_to()

PATCH In review Paul Bolle 4 participants 9 messages Jan 7, 2014 – Jan 17, 2014

Subsystems (3)
Networking Network drivers Mellanox MLX4 core VPI driver
lore
Share & tools
lore

Paul Bolle

author
#0 lore raw

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

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 <[email protected]>
---
 .../net/ethernet/mellanox/mlx4/resource_tracker.c  | 51 ++++++++--------------
 1 file changed, 19 insertions(+), 32 deletions(-)
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +19 -32
index 2f3f2bc..a41f01e 100644
@@ -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));
Signature · 1 line
1.8.4.2
#1 lore raw
Quoted 6 lines · 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);
^

Quoted 8 lines

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!

#2 lore raw
In reply to Paul Bolle

This time, replying to the list as well.

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

drivers/net/ethernet/mellanox/mlx4/resource_tracker.c static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn, · line 1356

Quoted 3 lines · On Tue, 07 Jan 2014 14:01:18 +0100 Paul Bolle <[email protected]> wrote:
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +3 -0
+	} 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.

Quoted 7 lines
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +3 -2
 			err = -EINVAL;
-		}
+		else
+			err = 0;
+	}
 
-		if (!err) {

drivers/net/ethernet/mellanox/mlx4/resource_tracker.c static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn, · line 1365

Quoted 10 lines
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +4 -6
-			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.

Quoted 2 lines
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +1 -0
+		*cq = r;
 	}
author
#3 lore raw
Quoted 10 lines · Jack Morgenstein

drivers/net/ethernet/mellanox/mlx4/resource_tracker.c static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn, · line 1356

Quoted 3 lines · On Tue, 07 Jan 2014 14:01:18 +0100 Paul Bolle <[email protected]> wrote:
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +3 -0
+	} 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.

Quoted 22 lines

This also preserves the behavior of the switch statement.

Quoted 7 lines
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +3 -2
 			err = -EINVAL;
-		}
+		else
+			err = 0;
+	}
 
-		if (!err) {

drivers/net/ethernet/mellanox/mlx4/resource_tracker.c static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn, · line 1365

Quoted 10 lines
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +4 -6
-			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.

Quoted 2 lines

drivers/net/ethernet/mellanox/mlx4/resource_tracker.c static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn, · line 1367

Quoted 2 lines
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +1 -0
+		*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

author
#4 lore raw

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

In reply to Jack Morgenstein

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 <[email protected]>

---
v2: adjust to Jack's review.

 .../net/ethernet/mellanox/mlx4/resource_tracker.c  | 52 ++++++++--------------
 1 file changed, 19 insertions(+), 33 deletions(-)
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +19 -33
index 2f3f2bc..15cd659 100644
@@ -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));
Signature · 1 line
1.8.4.2
maintainer
#5 lore raw

From: Paul Bolle <[email protected]>
Date: Tue, 14 Jan 2014 20:45:36 +0100

Quoted 11 lines

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.

Quoted 5 lines

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

Signed-off-by Paul Bolle <[email protected]>

---
v2: adjust to Jack's review.

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

#6 lore raw
In reply to Paul Bolle

ACK. OK.

-Jack

Quoted 90 lines · On Tue, 14 Jan 2014 20:45:36 +0100 Paul Bolle <[email protected]> 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 <[email protected]>

---
v2: adjust to Jack's review.

 .../net/ethernet/mellanox/mlx4/resource_tracker.c  | 52

++++++++-------------- 1 file changed, 19 insertions(+), 33 deletions(-)

(quoted diff) +0 -0

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));

drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +9 -26
 	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) {

drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +10 -7
+		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));
maintainer
#7 lore raw

From: Jack Morgenstein <[email protected]>
Date: Thu, 16 Jan 2014 09:46:39 +0200

Quoted 1 line

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.

maintainer Applied
#8 lore raw
In reply to Paul Bolle

From: Paul Bolle <[email protected]>
Date: Tue, 14 Jan 2014 20:45:36 +0100

Quoted 6 lines

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);
^

Quoted 8 lines

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 <[email protected]>

Applied.