| Message ID | 1389099678.15032.19.camel@x41 |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; > } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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));
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(-)