Message ID | 1380633877-24034-2-git-send-email-prarit@redhat.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, 1 Oct 2013 09:24:37 -0400 Prarit Bhargava <prarit@redhat.com> wrote: Hi, You missed some needed changes (You did not get compile warnings, because indeed "r" was initialized in the paths below. However, in these error paths, "err" was ignored. You should be setting "r" to the error return values: diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c index 343206b..c4a0a32 100644 --- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c @@ -1085,9 +1085,9 @@ static struct res_cq *cq_res_start_move_to(struct mlx4_dev *dev, spin_lock_irq(mlx4_tlock(dev)); r = res_tracker_lookup(&tracker->res_tree[RES_CQ], cqn); if (!r) - err = -ENOENT; + r = ERR_PTR(-ENOENT); else if (r->com.owner != slave) - err = -EPERM; + r = ERR_PTR(-EPERM); else { switch (state) { case RES_CQ_BUSY: @@ -1140,9 +1140,9 @@ static struct res_srq *srq_res_start_move_to(struct mlx4_dev *dev, int slave, spin_lock_irq(mlx4_tlock(dev)); r = res_tracker_lookup(&tracker->res_tree[RES_SRQ], index); if (!r) - err = -ENOENT; + r = ERR_PTR(-ENOENT); else if (r->com.owner != slave) - err = -EPERM; + r = ERR_PTR(-EPERM); else { switch (state) { case RES_SRQ_BUSY: There are also other calls which need to be changed in a similar fashion (eq_res_start_move_to(), and one or two others -- I'm surprised that these others did not also generate uninitialized-var warnings). If we change cq and srq, we should also change the others. Since we are in the middle of submitting patches which touch the file "resource_tracker.c", I would really like to hold off on these warning fixes for a bit, and I'll handle the changes for all the functions at once to conform (correctly!) to the format suggested by Dave Miller. -Jack > Fix unitialized variable warnings. > > drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function > ‘mlx4_HW2SW_CQ_wrapper’: > drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:2551:16: error: > ‘cq’ may be used uninitialized in this function > [-Werror=maybe-uninitialized] atomic_dec(&cq->mtt->ref_count); ^ > drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function > ‘mlx4_HW2SW_SRQ_wrapper’: > drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:2734:17: error: > ‘srq’ may be used uninitialized in this function > [-Werror=maybe-uninitialized] atomic_dec(&srq->mtt->ref_count); > > [v2]: davem suggested making cq_res_start_move_to() return 'cq' as an > error pointer instead of setting 'cq' by reference. I also did the > same for srq. > > Signed-off-by: Prarit Bhargava <prarit@redhat.com> > Cc: dledford@redhat.com > Cc: amirv@mellanox.com > Cc: davem@davemloft.net > Cc: ogerlitz@mellanox.com > Cc: jackm@dev.mellanox.co.il > --- > .../net/ethernet/mellanox/mlx4/resource_tracker.c | 46 > ++++++++++---------- 1 file changed, 24 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c > b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c index > dd68763..343206b 100644 --- > a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +++ > b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c @@ -1073,8 > +1073,9 @@ static int eq_res_start_move_to(struct mlx4_dev *dev, int > slave, int index, return err; } > > -static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int > cqn, > - enum res_cq_states state, struct > res_cq **cq) +static struct res_cq *cq_res_start_move_to(struct > mlx4_dev *dev, > + int slave, int cqn, > + enum res_cq_states > state) { > struct mlx4_priv *priv = mlx4_priv(dev); > struct mlx4_resource_tracker *tracker = > &priv->mfunc.master.res_tracker; @@ -1117,18 +1118,19 @@ static int > cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn, > r->com.from_state = r->com.state; r->com.to_state = state; > r->com.state = RES_CQ_BUSY; > - if (cq) > - *cq = r; > + } else { > + r = ERR_PTR(err); > } > } > > spin_unlock_irq(mlx4_tlock(dev)); > > - return err; > + return r; > } > > -static int srq_res_start_move_to(struct mlx4_dev *dev, int slave, > int index, > - enum res_cq_states state, struct > res_srq **srq) +static struct res_srq *srq_res_start_move_to(struct > mlx4_dev *dev, int slave, > + int index, > + enum res_cq_states > state) { > struct mlx4_priv *priv = mlx4_priv(dev); > struct mlx4_resource_tracker *tracker = > &priv->mfunc.master.res_tracker; @@ -1167,14 +1169,14 @@ static int > srq_res_start_move_to(struct mlx4_dev *dev, int slave, int index, > r->com.from_state = r->com.state; r->com.to_state = state; > r->com.state = RES_SRQ_BUSY; > - if (srq) > - *srq = r; > + } else { > + r = ERR_PTR(err); > } > } > > spin_unlock_irq(mlx4_tlock(dev)); > > - return err; > + return r; > } > > static void res_abort_move(struct mlx4_dev *dev, int slave, > @@ -2530,9 +2532,9 @@ int mlx4_SW2HW_CQ_wrapper(struct mlx4_dev *dev, > int slave, struct res_cq *cq; > struct res_mtt *mtt; > > - err = cq_res_start_move_to(dev, slave, cqn, RES_CQ_HW, &cq); > - if (err) > - return err; > + cq = cq_res_start_move_to(dev, slave, cqn, RES_CQ_HW); > + if (IS_ERR(cq)) > + return PTR_ERR(cq); > err = get_res(dev, slave, mtt_base, RES_MTT, &mtt); > if (err) > goto out_move; > @@ -2565,9 +2567,9 @@ int mlx4_HW2SW_CQ_wrapper(struct mlx4_dev *dev, > int slave, int cqn = vhcr->in_modifier; > struct res_cq *cq; > > - err = cq_res_start_move_to(dev, slave, cqn, > RES_CQ_ALLOCATED, &cq); > - if (err) > - return err; > + cq = cq_res_start_move_to(dev, slave, cqn, RES_CQ_ALLOCATED); > + if (IS_ERR(cq)) > + return PTR_ERR(cq); > err = mlx4_DMA_wrapper(dev, slave, vhcr, inbox, outbox, cmd); > if (err) > goto out_move; > @@ -2709,9 +2711,9 @@ int mlx4_SW2HW_SRQ_wrapper(struct mlx4_dev > *dev, int slave, if (srqn != (be32_to_cpu(srqc->state_logsize_srqn) & > 0xffffff)) return -EINVAL; > > - err = srq_res_start_move_to(dev, slave, srqn, RES_SRQ_HW, > &srq); > - if (err) > - return err; > + srq = srq_res_start_move_to(dev, slave, srqn, > RES_SRQ_ALLOCATED); > + if (IS_ERR(srq)) > + return PTR_ERR(srq); > err = get_res(dev, slave, mtt_base, RES_MTT, &mtt); > if (err) > goto ex_abort; > @@ -2748,9 +2750,9 @@ int mlx4_HW2SW_SRQ_wrapper(struct mlx4_dev > *dev, int slave, int srqn = vhcr->in_modifier; > struct res_srq *srq; > > - err = srq_res_start_move_to(dev, slave, srqn, > RES_SRQ_ALLOCATED, &srq); > - if (err) > - return err; > + srq = srq_res_start_move_to(dev, slave, srqn, > RES_SRQ_ALLOCATED); > + if (IS_ERR(srq)) > + return PTR_ERR(srq); > err = mlx4_DMA_wrapper(dev, slave, vhcr, inbox, outbox, cmd); > if (err) > goto ex_abort; -- 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 10/01/2013 11:17 AM, Jack Morgenstein wrote: > Since we are in the middle of submitting patches which touch the file > "resource_tracker.c", I would really like to hold off on these warning > fixes for a bit, and I'll handle the changes for all the functions at > once to conform (correctly!) to the format suggested by Dave Miller. > > -Jack Hey Jack, np. Thanks for looking. P. -- 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
From: Prarit Bhargava <prarit@redhat.com> Date: Tue, 01 Oct 2013 11:25:28 -0400 > > > On 10/01/2013 11:17 AM, Jack Morgenstein wrote: >> Since we are in the middle of submitting patches which touch the file >> "resource_tracker.c", I would really like to hold off on these warning >> fixes for a bit, and I'll handle the changes for all the functions at >> once to conform (correctly!) to the format suggested by Dave Miller. >> >> -Jack > > Hey Jack, np. Thanks for looking. Please repost both patches when you've sorted this out, thanks! -- 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 dd68763..343206b 100644 --- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c +++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c @@ -1073,8 +1073,9 @@ static int eq_res_start_move_to(struct mlx4_dev *dev, int slave, int index, return err; } -static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn, - enum res_cq_states state, struct res_cq **cq) +static struct res_cq *cq_res_start_move_to(struct mlx4_dev *dev, + int slave, int cqn, + enum res_cq_states state) { struct mlx4_priv *priv = mlx4_priv(dev); struct mlx4_resource_tracker *tracker = &priv->mfunc.master.res_tracker; @@ -1117,18 +1118,19 @@ static int cq_res_start_move_to(struct mlx4_dev *dev, int slave, int cqn, r->com.from_state = r->com.state; r->com.to_state = state; r->com.state = RES_CQ_BUSY; - if (cq) - *cq = r; + } else { + r = ERR_PTR(err); } } spin_unlock_irq(mlx4_tlock(dev)); - return err; + return r; } -static int srq_res_start_move_to(struct mlx4_dev *dev, int slave, int index, - enum res_cq_states state, struct res_srq **srq) +static struct res_srq *srq_res_start_move_to(struct mlx4_dev *dev, int slave, + int index, + enum res_cq_states state) { struct mlx4_priv *priv = mlx4_priv(dev); struct mlx4_resource_tracker *tracker = &priv->mfunc.master.res_tracker; @@ -1167,14 +1169,14 @@ static int srq_res_start_move_to(struct mlx4_dev *dev, int slave, int index, r->com.from_state = r->com.state; r->com.to_state = state; r->com.state = RES_SRQ_BUSY; - if (srq) - *srq = r; + } else { + r = ERR_PTR(err); } } spin_unlock_irq(mlx4_tlock(dev)); - return err; + return r; } static void res_abort_move(struct mlx4_dev *dev, int slave, @@ -2530,9 +2532,9 @@ int mlx4_SW2HW_CQ_wrapper(struct mlx4_dev *dev, int slave, struct res_cq *cq; struct res_mtt *mtt; - err = cq_res_start_move_to(dev, slave, cqn, RES_CQ_HW, &cq); - if (err) - return err; + cq = cq_res_start_move_to(dev, slave, cqn, RES_CQ_HW); + if (IS_ERR(cq)) + return PTR_ERR(cq); err = get_res(dev, slave, mtt_base, RES_MTT, &mtt); if (err) goto out_move; @@ -2565,9 +2567,9 @@ int mlx4_HW2SW_CQ_wrapper(struct mlx4_dev *dev, int slave, int cqn = vhcr->in_modifier; struct res_cq *cq; - err = cq_res_start_move_to(dev, slave, cqn, RES_CQ_ALLOCATED, &cq); - if (err) - return err; + cq = cq_res_start_move_to(dev, slave, cqn, RES_CQ_ALLOCATED); + if (IS_ERR(cq)) + return PTR_ERR(cq); err = mlx4_DMA_wrapper(dev, slave, vhcr, inbox, outbox, cmd); if (err) goto out_move; @@ -2709,9 +2711,9 @@ int mlx4_SW2HW_SRQ_wrapper(struct mlx4_dev *dev, int slave, if (srqn != (be32_to_cpu(srqc->state_logsize_srqn) & 0xffffff)) return -EINVAL; - err = srq_res_start_move_to(dev, slave, srqn, RES_SRQ_HW, &srq); - if (err) - return err; + srq = srq_res_start_move_to(dev, slave, srqn, RES_SRQ_ALLOCATED); + if (IS_ERR(srq)) + return PTR_ERR(srq); err = get_res(dev, slave, mtt_base, RES_MTT, &mtt); if (err) goto ex_abort; @@ -2748,9 +2750,9 @@ int mlx4_HW2SW_SRQ_wrapper(struct mlx4_dev *dev, int slave, int srqn = vhcr->in_modifier; struct res_srq *srq; - err = srq_res_start_move_to(dev, slave, srqn, RES_SRQ_ALLOCATED, &srq); - if (err) - return err; + srq = srq_res_start_move_to(dev, slave, srqn, RES_SRQ_ALLOCATED); + if (IS_ERR(srq)) + return PTR_ERR(srq); err = mlx4_DMA_wrapper(dev, slave, vhcr, inbox, outbox, cmd); if (err) goto ex_abort;
Fix unitialized variable warnings. drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function ‘mlx4_HW2SW_CQ_wrapper’: drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:2551:16: error: ‘cq’ may be used uninitialized in this function [-Werror=maybe-uninitialized] atomic_dec(&cq->mtt->ref_count); ^ drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function ‘mlx4_HW2SW_SRQ_wrapper’: drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:2734:17: error: ‘srq’ may be used uninitialized in this function [-Werror=maybe-uninitialized] atomic_dec(&srq->mtt->ref_count); [v2]: davem suggested making cq_res_start_move_to() return 'cq' as an error pointer instead of setting 'cq' by reference. I also did the same for srq. Signed-off-by: Prarit Bhargava <prarit@redhat.com> Cc: dledford@redhat.com Cc: amirv@mellanox.com Cc: davem@davemloft.net Cc: ogerlitz@mellanox.com Cc: jackm@dev.mellanox.co.il --- .../net/ethernet/mellanox/mlx4/resource_tracker.c | 46 ++++++++++---------- 1 file changed, 24 insertions(+), 22 deletions(-)