Re: [PATCH for-next 1/2] IB/core: Fix build warnings

[Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>]

On 31/10/2013 9:21, Or Gerlitz wrote:
> Fix the below few "make W=1" build warnings we have on the IB core.
>
> drivers/infiniband/core/sysfs.c: In function ‘state_show’:
> drivers/infiniband/core/sysfs.c:107: warning: comparison of unsigned expression >= 0 is always true
> drivers/infiniband/core/verbs.c: In function ‘ib_modify_qp_is_ok’:
> drivers/infiniband/core/verbs.c:783: warning: comparison of unsigned expression < 0 is always false
> drivers/infiniband/core/verbs.c:784: warning: comparison of unsigned expression < 0 is always false
> drivers/infiniband/core/iwcm.c: In function ‘destroy_cm_id’:
> drivers/infiniband/core/iwcm.c:330: warning: variable ‘ret’ set but not used
>
> Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>   drivers/infiniband/core/iwcm.c  |    2 --
>   drivers/infiniband/core/sysfs.c |    2 +-
>   drivers/infiniband/core/verbs.c |    3 +--
>   3 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
> index c47c203..ab8ebf8 100644
> --- a/drivers/infiniband/core/iwcm.c
> +++ b/drivers/infiniband/core/iwcm.c
> @@ -327,7 +327,6 @@ static void destroy_cm_id(struct iw_cm_id *cm_id)
>   {
>   	struct iwcm_id_private *cm_id_priv;
>   	unsigned long flags;
> -	int ret;
>
>   	cm_id_priv = container_of(cm_id, struct iwcm_id_private, id);
>   	/*
> @@ -343,7 +342,6 @@ static void destroy_cm_id(struct iw_cm_id *cm_id)
>   		cm_id_priv->state = IW_CM_STATE_DESTROYING;
>   		spin_unlock_irqrestore(&cm_id_priv->lock, flags);
>   		/* destroy the listening endpoint */
> -		ret = cm_id->device->iwcm->destroy_listen(cm_id);
>   		spin_lock_irqsave(&cm_id_priv->lock, flags);
>   		break;
>   	case IW_CM_STATE_ESTABLISHED:
> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> index cde1e7b..1184050 100644
> --- a/drivers/infiniband/core/sysfs.c
> +++ b/drivers/infiniband/core/sysfs.c
> @@ -104,7 +104,7 @@ static ssize_t state_show(struct ib_port *p, struct port_attribute *unused,
>   		return ret;
>
>   	return sprintf(buf, "%d: %s\n", attr.state,
> -		       attr.state >= 0 && attr.state < ARRAY_SIZE(state_name) ?
> +		       attr.state < ARRAY_SIZE(state_name) ?
>   		       state_name[attr.state] : "UNKNOWN");
>   }
>
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index a321df2..7b0c1f4 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -780,8 +780,7 @@ int ib_modify_qp_is_ok(enum ib_qp_state cur_state, enum ib_qp_state next_state,
>   {
>   	enum ib_qp_attr_mask req_param, opt_param;
>
> -	if (cur_state  < 0 || cur_state  > IB_QPS_ERR ||
> -	    next_state < 0 || next_state > IB_QPS_ERR)
> +	if (cur_state  > IB_QPS_ERR || next_state > IB_QPS_ERR)
>   		return 0;
>
>   	if (mask & IB_QP_CUR_STATE  &&
>

I agree that removing unused variables is good because it makes source 
code easier to read. However, removing the "attr.state >= 0" check from 
state_show() looks dangerous to me. If the type of that variable would 
ever be changed from unsigned to signed then that would break 
state_show() in a very subtle way. It could take a long time before such 
breakage is detected. How about modifying the behavior of W=1 such that 
it doesn't warn about comparisons that are always true (-Wno-type-limits) ?

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html