Skip to contentSkip to conversation

drivers/infiniband/hw/nes: Adjust confusing if indentation

PATCH Applied Julia Lawall 4 participants 4 messages Aug 5, 2010 – Aug 6, 2010

Subsystems (2)
Kernel janitors InfiniBand
lore
Share & tools
lore

Julia Lawall

author
#0 lore raw

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

From: Julia Lawall <[email protected]>

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 <[email protected]>
---
 drivers/infiniband/hw/nes/nes_hw.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/hw/nes/nes_hw.c +1 -1
index f41d890..9a379d4 100644
@@ -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 ++;
#1 lore raw
Quoted 2 lines

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(-)
drivers/infiniband/hw/nes/nes_hw.c +3 -3
index 07c4004..f8233c8 100644
@@ -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 {
Signature · 2 lines
Roland Dreier <[email protected]> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
#2 lore raw

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

In reply to Julia Lawall

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 <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>

---
v2: changed different indents

drivers/infiniband/hw/nes/nes_hw.c +3 -3
index 57874a1..293977b 100644
@@ -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 {
Acked-by on v2 4/42
#3 lore raw
Quoted 9 lines

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 <[email protected]>
Signed-off-by Dan Carpenter <[email protected]>

---
v2: changed different indents

Looks to me. Thanks for the patch.

Acked-by Chien Tung <[email protected]>

Chien