[PATCH 4/42] drivers/infiniband/hw/nes: Adjust confusing if indentation

[PATCH 4/42] drivers/infiniband/hw/nes: Adjust confusing if indentation

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

It is not clear whether the assignment should be part of the if branch
suggested by its indentation.  The patch preserves the current semantics.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r disable braces4@
position p1,p2;
statement S1,S2;
@@

(
if (...) { ... }
|
if (...) S1@p1 S2@p2
)

@script:python@
p1 << r.p1;
p2 << r.p2;
@@

if (p1[0].column = p2[0].column):
  cocci.print_main("branch",p1)
  cocci.print_secs("after",p2)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/infiniband/hw/nes/nes_hw.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/nes/nes_hw.c b/drivers/infiniband/hw/nes/nes_hw.c
index f41d890..9a379d4 100644
--- a/drivers/infiniband/hw/nes/nes_hw.c
+++ b/drivers/infiniband/hw/nes/nes_hw.c
@@ -2740,7 +2740,7 @@ void nes_nic_ce_handler(struct nes_device *nesdev, struct nes_hw_nic_cq *cq)
 				/* restart the queue if it had been stopped */
 				if (netif_queue_stopped(nesvnic->netdev))
 					netif_wake_queue(nesvnic->netdev);
-					sq_cqes = 0;
+				sq_cqes = 0;
 				}
 			} else {
 				rqes_processed ++;

[PATCH 4/42] drivers/infiniband/hw/nes: Adjust confusing if indentation

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

It is not clear whether the assignment should be part of the if branch
suggested by its indentation.  The patch preserves the current semantics.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r disable braces4@
position p1,p2;
statement S1,S2;
@@

(
if (...) { ... }
|
if (...) S1@p1 S2@p2
)

@script:python@
p1 << r.p1;
p2 << r.p2;
@@

if (p1[0].column == p2[0].column):
  cocci.print_main("branch",p1)
  cocci.print_secs("after",p2)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/infiniband/hw/nes/nes_hw.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/nes/nes_hw.c b/drivers/infiniband/hw/nes/nes_hw.c
index f41d890..9a379d4 100644
--- a/drivers/infiniband/hw/nes/nes_hw.c
+++ b/drivers/infiniband/hw/nes/nes_hw.c
@@ -2740,7 +2740,7 @@ void nes_nic_ce_handler(struct nes_device *nesdev, struct nes_hw_nic_cq *cq)
 				/* restart the queue if it had been stopped */
 				if (netif_queue_stopped(nesvnic->netdev))
 					netif_wake_queue(nesvnic->netdev);
-					sq_cqes = 0;
+				sq_cqes = 0;
 				}
 			} else {
 				rqes_processed ++;

Re: [PATCH 4/42] drivers/infiniband/hw/nes: Adjust confusing if indentation

[Roland Dreier]

 > It is not clear whether the assignment should be part of the if branch
 > suggested by its indentation.  The patch preserves the current semantics.

Thanks.  Chien/Faisal -- are the current semantics correct or should we
add braces { } to match the indentation?  From a quick look at the code,
I think the patch we actually want is:

 drivers/infiniband/hw/nes/nes_hw.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/nes/nes_hw.c b/drivers/infiniband/hw/nes/nes_hw.c
index 07c4004..f8233c8 100644
--- a/drivers/infiniband/hw/nes/nes_hw.c
+++ b/drivers/infiniband/hw/nes/nes_hw.c
@@ -2737,9 +2737,9 @@ void nes_nic_ce_handler(struct nes_device *nesdev, struct nes_hw_nic_cq *cq)
 				nesnic->sq_tail &= nesnic->sq_size-1;
 				if (sq_cqes > 128) {
 					barrier();
-				/* restart the queue if it had been stopped */
-				if (netif_queue_stopped(nesvnic->netdev))
-					netif_wake_queue(nesvnic->netdev);
+					/* restart the queue if it had been stopped */
+					if (netif_queue_stopped(nesvnic->netdev))
+						netif_wake_queue(nesvnic->netdev);
 					sq_cqes = 0;
 				}
 			} else {

-- 
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Re: [PATCH 4/42] drivers/infiniband/hw/nes: Adjust confusing if indentation

[Roland Dreier]

 > It is not clear whether the assignment should be part of the if branch
 > suggested by its indentation.  The patch preserves the current semantics.

Thanks.  Chien/Faisal -- are the current semantics correct or should we
add braces { } to match the indentation?  From a quick look at the code,
I think the patch we actually want is:

 drivers/infiniband/hw/nes/nes_hw.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/nes/nes_hw.c b/drivers/infiniband/hw/nes/nes_hw.c
index 07c4004..f8233c8 100644
--- a/drivers/infiniband/hw/nes/nes_hw.c
+++ b/drivers/infiniband/hw/nes/nes_hw.c
@@ -2737,9 +2737,9 @@ void nes_nic_ce_handler(struct nes_device *nesdev, struct nes_hw_nic_cq *cq)
 				nesnic->sq_tail &= nesnic->sq_size-1;
 				if (sq_cqes > 128) {
 					barrier();
-				/* restart the queue if it had been stopped */
-				if (netif_queue_stopped(nesvnic->netdev))
-					netif_wake_queue(nesvnic->netdev);
+					/* restart the queue if it had been stopped */
+					if (netif_queue_stopped(nesvnic->netdev))
+						netif_wake_queue(nesvnic->netdev);
 					sq_cqes = 0;
 				}
 			} else {

-- 
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

[PATCH 4/42 v2] drivers/infiniband/hw/nes: Adjust confusing if

[Dan Carpenter]

I think the white space is meant to look like this.  I did look at
whether the "sq_cqes = 0;" should only be done if netif_queue_stopped().
In the end I decided this was what was intended, but it would be
better if someone more familiar with the code reviewed it.

Reported-by: Julia Lawall <julia@diku.dk>
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
v2: changed different indents

diff --git a/drivers/infiniband/hw/nes/nes_hw.c b/drivers/infiniband/hw/nes/nes_hw.c
index 57874a1..293977b 100644
--- a/drivers/infiniband/hw/nes/nes_hw.c
+++ b/drivers/infiniband/hw/nes/nes_hw.c
@@ -2737,9 +2737,9 @@ void nes_nic_ce_handler(struct nes_device *nesdev, struct nes_hw_nic_cq *cq)
 				nesnic->sq_tail &= nesnic->sq_size-1;
 				if (sq_cqes > 128) {
 					barrier();
-				/* restart the queue if it had been stopped */
-				if (netif_queue_stopped(nesvnic->netdev))
-					netif_wake_queue(nesvnic->netdev);
+					/* restart the queue if it had been stopped */
+					if (netif_queue_stopped(nesvnic->netdev))
+						netif_wake_queue(nesvnic->netdev);
 					sq_cqes = 0;
 				}
 			} else {

[PATCH 4/42 v2] drivers/infiniband/hw/nes: Adjust confusing if indentation

[Dan Carpenter]

I think the white space is meant to look like this.  I did look at
whether the "sq_cqes = 0;" should only be done if netif_queue_stopped().
In the end I decided this was what was intended, but it would be
better if someone more familiar with the code reviewed it.

Reported-by: Julia Lawall <julia@diku.dk>
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
v2: changed different indents

diff --git a/drivers/infiniband/hw/nes/nes_hw.c b/drivers/infiniband/hw/nes/nes_hw.c
index 57874a1..293977b 100644
--- a/drivers/infiniband/hw/nes/nes_hw.c
+++ b/drivers/infiniband/hw/nes/nes_hw.c
@@ -2737,9 +2737,9 @@ void nes_nic_ce_handler(struct nes_device *nesdev, struct nes_hw_nic_cq *cq)
 				nesnic->sq_tail &= nesnic->sq_size-1;
 				if (sq_cqes > 128) {
 					barrier();
-				/* restart the queue if it had been stopped */
-				if (netif_queue_stopped(nesvnic->netdev))
-					netif_wake_queue(nesvnic->netdev);
+					/* restart the queue if it had been stopped */
+					if (netif_queue_stopped(nesvnic->netdev))
+						netif_wake_queue(nesvnic->netdev);
 					sq_cqes = 0;
 				}
 			} else {

RE: [PATCH 4/42 v2] drivers/infiniband/hw/nes: Adjust confusing if

[Tung, Chien Tin]

> I think the white space is meant to look like this.  I did look at
> whether the "sq_cqes = 0;" should only be done if netif_queue_stopped().
> In the end I decided this was what was intended, but it would be
> better if someone more familiar with the code reviewed it.
> 
> Reported-by: Julia Lawall <julia@diku.dk>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> v2: changed different indents

Looks to me.  Thanks for the patch.

Acked-by: Chien Tung <chien.tin.tung@intel.com>

Chien

RE: [PATCH 4/42 v2] drivers/infiniband/hw/nes: Adjust confusing if indentation

[Tung, Chien Tin]

> I think the white space is meant to look like this.  I did look at
> whether the "sq_cqes = 0;" should only be done if netif_queue_stopped().
> In the end I decided this was what was intended, but it would be
> better if someone more familiar with the code reviewed it.
> 
> Reported-by: Julia Lawall <julia@diku.dk>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> v2: changed different indents

Looks to me.  Thanks for the patch.

Acked-by: Chien Tung <chien.tin.tung@intel.com>

Chien

RE: [PATCH 4/42 v2] drivers/infiniband/hw/nes: Adjust confusing if indentation

[Tung, Chien Tin]

> I think the white space is meant to look like this.  I did look at
> whether the "sq_cqes = 0;" should only be done if netif_queue_stopped().
> In the end I decided this was what was intended, but it would be
> better if someone more familiar with the code reviewed it.
> 
> Reported-by: Julia Lawall <julia-dAYI7NvHqcQ@public.gmane.org>
> Signed-off-by: Dan Carpenter <error27-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> v2: changed different indents

Looks to me.  Thanks for the patch.

Acked-by: Chien Tung <chien.tin.tung-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Chien




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