[PATCH for-next 0/2] Fix build warnings

[PATCH for-next 0/2] Fix build warnings

[Or Gerlitz]

Hi Roland,

This little patch set removes the current "make W=1" build warnings from the IB core
and the mlx4_ib driver, which would help when we examine new patches to make sure they
don't add any warnings..

Or.

Or Gerlitz (2):
  IB/core: Fix build warnings
  IB/mlx4: Fix build warnings

 drivers/infiniband/core/iwcm.c  |    2 --
 drivers/infiniband/core/sysfs.c |    2 +-
 drivers/infiniband/core/verbs.c |    3 +--
 drivers/infiniband/hw/mlx4/qp.c |    2 +-
 4 files changed, 3 insertions(+), 6 deletions(-)

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

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

[Or Gerlitz]

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  &&
-- 
1.7.1

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

[PATCH for-next 2/2] IB/mlx4: Fix build warnings

[Or Gerlitz]

Fix the below "make W=1" build warning we have on the mlx4_ib

drivers/infiniband/hw/mlx4/qp.c: In function ‘mlx4_ib_post_send’:
drivers/infiniband/hw/mlx4/qp.c:2463: warning: comparison of unsigned expression < 0 is always false

Signed-off-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/hw/mlx4/qp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index 4f10af2..da25e4d 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -2460,7 +2460,7 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		 */
 		wmb();
 
-		if (wr->opcode < 0 || wr->opcode >= ARRAY_SIZE(mlx4_ib_opcode)) {
+		if (wr->opcode >= ARRAY_SIZE(mlx4_ib_opcode)) {
 			*bad_wr = wr;
 			err = -EINVAL;
 			goto out;
-- 
1.7.1

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

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

[Bart Van Assche]

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

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

[Hefty, Sean]

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

removing 'ret' is one thing, removing the call to destroy_listen() is another.

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

[David Dillow]

On Thu, 2013-10-31 at 10:02 -0700, Bart Van Assche wrote:
> On 31/10/2013 9:21, Or Gerlitz wrote:
> > --- 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");
> >   }

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

We want to know about those comparisons, because they often indicate a
bug -- either in one's thinking, or in the code they've written.
Changing attr.state from unsigned to signed is unlikely to ever happen,
as a massive audit would be needed -- this is not an uncommon pattern in
the kernel.

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

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

[Or Gerlitz]

On Thu, Oct 31, 2013 at 7:59 PM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> removing 'ret' is one thing, removing the call to destroy_listen() is another.

sure, I will fix and resubmit
--
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

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

[Or Gerlitz]

On 31/10/2013 21:28, David Dillow wrote:
> We want to know about those comparisons, because they often indicate a
> bug -- either in one's thinking, or in the code they've written.
> Changing attr.state from unsigned to signed is unlikely to ever happen,
> as a massive audit would be needed -- this is not an uncommon pattern in
> the kernel.
I agree with Dave.
--
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