[PATCH for-next V1 0/2] Fix build warnings in the IB core and mlx4_ib

[PATCH for-next V1 0/2] Fix build warnings in the IB core and mlx4_ib

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

changes from V0:
 - returned call to iwcm->destroy_listen which was wrongly removed, 
   spotted by Sean.

Or.

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

 drivers/infiniband/core/iwcm.c  |    3 +--
 drivers/infiniband/core/sysfs.c |    2 +-
 drivers/infiniband/core/verbs.c |    3 +--
 drivers/infiniband/hw/mlx4/qp.c |    2 +-
 4 files changed, 4 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 V1 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  |    3 +--
 drivers/infiniband/core/sysfs.c |    2 +-
 drivers/infiniband/core/verbs.c |    3 +--
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index c47c203..bdb4d50 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,7 @@ 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);
+		(void)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

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

[Yann Droneaud]

Hi,

Le dimanche 03 novembre 2013 à 10:20 +0200, Or Gerlitz a écrit :
> 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>

Reviewed-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

PS: Perhaps you could split the patch in two parts: one to remove the
unused variable, and another to remove the check on unsigned variables ?

Regards.

-- 
Yann Droneaud
OPTEYA


--
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 V1 1/2] IB/core: Fix build warnings

[Or Gerlitz]

On 23/01/2014 11:47, Yann Droneaud wrote:
> Hi,
>
> Le dimanche 03 novembre 2013 à 10:20 +0200, Or Gerlitz a écrit :
>> >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>
> Reviewed-by: Yann Droneaud<ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
>
> PS: Perhaps you could split the patch in two parts: one to remove the
> unused variable, and another to remove the check on unsigned variables ?
Roland decided not to take the  unsigned expression < 0 patches, writing 
to me

"I applied the unused variable fix, but the others seem not needed,
because the latest kernel seems to include -Wno-sign-compare."

what's your thinking?

Or.
--
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 V1 1/2] IB/core: Fix build warnings

[Yann Droneaud]

Hi Or,

Le jeudi 23 janvier 2014 à 14:29 +0200, Or Gerlitz a écrit :
> On 23/01/2014 11:47, Yann Droneaud wrote:
> > Le dimanche 03 novembre 2013 à 10:20 +0200, Or Gerlitz a écrit :
> >> >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>
> > Reviewed-by: Yann Droneaud<ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> >
> > PS: Perhaps you could split the patch in two parts: one to remove the
> > unused variable, and another to remove the check on unsigned variables ?

> Roland decided not to take the  unsigned expression < 0 patches, writing 
> to me
> 
> "I applied the unused variable fix, but the others seem not needed,
> because the latest kernel seems to include -Wno-sign-compare."
> 
> what's your thinking?
> 

I think Roland should write to the list instead of answering privately,
but you're probably not asking my opinion about this matter.

Regarding the warnings, I've double check GCC behavor. AFAICT it's not
related to -Wno-sign-compare enabled or not, but some subtle issue.

The actual option to trigger the warning is -Wtype-limits, and it's
implied with -Wextra. And -Wextra is enabled with make W=1.

A simple test case such as the one below will trigger the warning:

  int f(unsigned int ui)
  {
      if (ui < 0)
         return -1;

      return 0;
  }

Tested with GCC 4.8.2 under Fedora 20, compiled for x86_64,
cross-compiled for ARM and Power (ppc64), it produces the warning:

$ gcc -Wtype-limits -c warning-simple.c 
warning-simple.c: In function 'f':
warning-simple.c:3:3: warning: comparison of unsigned expression < 0 is
always false [-Wtype-limits]
   if (ui < 0)
   ^

The same will happen with a more elaborate condition:

$ diff -u1 warning-simple.c warning-elaborate.c
--- warning-simple.c
+++ warning-elaborate.c
@@ -2,3 +2,3 @@
 {
-  if (ui < 0)
+  if (ui < 0 || ui > 1000)
     return -1;

$ gcc -Wtype-limits  -c warning-elaborate.c 
warning-elaborate.c: In function 'f':
warning-elaborate.c:3:3: warning: comparison of unsigned expression < 0
is always false [-Wtype-limits]
   if (ui < 0 || ui > 1000)
   ^

But as noted by Roland, the error is not shown when compiling the kernel
InfiniBand modules. Which might sound surprising since the code your
patch was about looks really like my simplified test case.
 
One must note that your patch is about fixing comparaison of enum type
against 0.

So, let's try with the following test case which replaces unsigned int
with an enum:

  enum e {
    E0,
    E1,
  };

  int f(enum e ee)
  {
    if (ee < 0)
      return -1;

    if (ee > 1000)
      return -1;

    return 0;
  }

Then no warning is reported while compiling this testcase with GCC 4.8
under Fedora 20, compiled for x86_64, cross-compiled for ARM and Power
(ppc64).

Indeed, an enum is mostly like an int.

That explains why Roland and I weren't able to notice the warning when
compiling with W=1.

To trigger the warning, because it can triggered, one should built for
an ABI that mandate either unsigned enums or short enums.

For those who want to try this other side of the ABI world, short enums
can be enabled for free with -fshort-enums (use with care).

In this case multiple warnings are reported:

$ gcc -fshort-enums -Wtype-limits -c warning-enum.c 
warning-enum.c: In function 'f':
warning-enum.c:8:3: warning: comparison is always false due to limited
range of data type [-Wtype-limits]
   if (ee < 0) {
   ^
warning-enum.c:11:3: warning: comparison is always false due to limited
range of data type [-Wtype-limits]
   if (ee > 1000) {
   ^

(For C++, have a look at -fstrict-enums).

To conclude, I think you will have to give some details on your build
environment so that we could reproduce the warning.

In the mean time, Roland was right to apply on the subset of your patch
that remove the unused variable.

Regards.

-- 
Yann Droneaud
OPTEYA


--
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 V1 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 V1 0/2] Fix build warnings in the IB core and mlx4_ib

[Or Gerlitz]

On Sun, Nov 3, 2013 at 10:20 AM, Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> 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..

Hi Roland,

Can this tiny patch set  be added for 3.14?

Or.

>
> changes from V0:
>  - returned call to iwcm->destroy_listen which was wrongly removed,
>    spotted by Sean.
>
> Or.
>
> Or Gerlitz (2):
>   IB/core: Fix build warnings
>   IB/mlx4: Fix build warnings
>
>  drivers/infiniband/core/iwcm.c  |    3 +--
>  drivers/infiniband/core/sysfs.c |    2 +-
>  drivers/infiniband/core/verbs.c |    3 +--
>  drivers/infiniband/hw/mlx4/qp.c |    2 +-
>  4 files changed, 4 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
--
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