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

login
register
mail settings
Submitter roel kluin
Date Feb. 18, 2009, 12:38 p.m.
Message ID <499C0143.1080703@gmail.com>
Download mbox | patch
Permalink /patch/23341/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

roel kluin - Feb. 18, 2009, 12:38 p.m.
Dropped general@lists.openfabrics.org since it bounces, and I missed one
in my previously sent patch
--------------------------->8-------------8<------------------------------
Logical-/bit-or typo

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Wise - Feb. 18, 2009, 3:13 p.m.
Reviewed-by: Steve Wise <swise@opengridcomputing.com>

Roel Kluin wrote:
> Dropped general@lists.openfabrics.org since it bounces, and I missed one
> in my previously sent patch
> --------------------------->8-------------8<------------------------------
> Logical-/bit-or typo
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.c b/drivers/infiniband/hw/cxgb3/iwch_cm.c
> index 44e936e..21c6145 100644
> --- a/drivers/infiniband/hw/cxgb3/iwch_cm.c
> +++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roland Dreier - Feb. 19, 2009, 12:37 a.m.
> -	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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve Wise - Feb. 19, 2009, 12:53 a.m.
Roland Dreier wrote:
>  > -	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.
>   
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?

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

Patch

diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.c b/drivers/infiniband/hw/cxgb3/iwch_cm.c
index 44e936e..21c6145 100644
--- a/drivers/infiniband/hw/cxgb3/iwch_cm.c
+++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c
@@ -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;