Skip to contentSkip to conversation

RDMA/cxgb3: logical-/bit-or confusion?

PATCH Roel Kluin 3 participants 4 messages Feb 18, 2009 – Feb 19, 2009 1 message not in the archive

Subsystems (3)
Networking Network drivers InfiniBand
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Roel Kluin

author
#1 lore raw

Dropped [email protected] since it bounces, and I missed one in my previously sent patch --------------------------->8-------------8<------------------------------ Logical-/bit-or typo

Signed-off-by Roel Kluin <[email protected]>

---

drivers/infiniband/hw/cxgb3/iwch_cm.c +2 -2
index 44e936e..21c6145 100644
@@ -890,7 +890,7 @@ static void process_mpa_reply(struct iwch_ep *ep, struct sk_buff *skb)
 	 */
 	state_set(&ep->com, FPDU_MODE);
 	ep->mpa_attr.initiator = 1;
-	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) | crc_enabled ? 1 : 0;
+	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) || crc_enabled ? 1 : 0;
 	ep->mpa_attr.recv_marker_enabled = markers_enabled;
 	ep->mpa_attr.xmit_marker_enabled = mpa->flags & MPA_MARKERS ? 1 : 0;
 	ep->mpa_attr.version = mpa_rev;

@@ -1013,7 +1013,7 @@ static void process_mpa_request(struct iwch_ep *ep, struct sk_buff *skb)
 	 * start reply message including private data.
 	 */
 	ep->mpa_attr.initiator = 0;
-	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) | crc_enabled ? 1 : 0;
+	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) || crc_enabled ? 1 : 0;
 	ep->mpa_attr.recv_marker_enabled = markers_enabled;
 	ep->mpa_attr.xmit_marker_enabled = mpa->flags & MPA_MARKERS ? 1 : 0;
 	ep->mpa_attr.version = mpa_rev;
Reviewed-by
#2 lore raw
Reviewed-by Steve Wise <[email protected]>
Quoted 34 lines · Roel Kluin

Dropped [email protected] since it bounces, and I missed one in my previously sent patch --------------------------->8-------------8<------------------------------ Logical-/bit-or typo

Signed-off-by Roel Kluin <[email protected]>

---

drivers/infiniband/hw/cxgb3/iwch_cm.c +2 -3
index 44e936e..21c6145 100644
@@ -890,7 +890,7 @@ static void process_mpa_reply(struct iwch_ep *ep, struct sk_buff *skb)
 	 */
 	state_set(&ep->com, FPDU_MODE);
 	ep->mpa_attr.initiator = 1;
-	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) | crc_enabled ? 1 : 0;
+	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) || crc_enabled ? 1 : 0;
 	ep->mpa_attr.recv_marker_enabled = markers_enabled;
 	ep->mpa_attr.xmit_marker_enabled = mpa->flags & MPA_MARKERS ? 1 : 0;
 	ep->mpa_attr.version = mpa_rev;

@@ -1013,7 +1013,7 @@ static void process_mpa_request(struct iwch_ep *ep, struct sk_buff *skb)
 	 * start reply message including private data.
 	 */
 	ep->mpa_attr.initiator = 0;
-	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) | crc_enabled ? 1 : 0;
+	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) || crc_enabled ? 1 : 0;
 	ep->mpa_attr.recv_marker_enabled = markers_enabled;
 	ep->mpa_attr.xmit_marker_enabled = mpa->flags & MPA_MARKERS ? 1 : 0;
 	ep->mpa_attr.version = mpa_rev;
--

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
In reply to Roel Kluin

Review

Quoted 5 lines
(quoted diff) +2 -2
-	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) | crc_enabled ? 1 : 0;
+	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) || crc_enabled ? 1 : 0;
 
-	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) | crc_enabled ? 1 : 0;
+	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) || crc_enabled ? 1 : 0;

So as I said before, I guess this change is fine, except:

add/remove: 0/0 grow/shrink: 1/0 up/down: 18/0 (18)
function old new delta
rx_data 1237 1255 +18

ie it makes the object code 18 bytes bigger on x86-64 (gcc 4.3.2). Given that the code works the same either way, is this change a net win?

- R.

#4 lore raw
Quoted 8 lines · Roland Dreier

Review

Quoted 5 lines
(quoted diff) +2 -2
-	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) | crc_enabled ? 1 : 0;
+	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) || crc_enabled ? 1 : 0;
 
-	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) | crc_enabled ? 1 : 0;
+	ep->mpa_attr.crc_enabled = (mpa->flags & MPA_CRC) || crc_enabled ? 1 : 0;

So as I said before, I guess this change is fine, except:

Quoted 9 lines

add/remove: 0/0 grow/shrink: 1/0 up/down: 18/0 (18)
function old new delta
rx_data 1237 1255 +18

ie it makes the object code 18 bytes bigger on x86-64 (gcc 4.3.2). Given that the code works the same either way, is this change a net win?

- R.

It does not bother me to leave the code as-is.

Roel,

Was this found by code inspection or some tool that is run on the code?