Re: [PATCH 2/2] net, mellanox mlx4 Fix compile warnings [v4]

[Jack Morgenstein <jackm@dev.mellanox.co.il>]

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;