RDMA/cxgb4: Remove unnecessary conversion from __be32 to cpu format

PATCH In review Leon Romanovsky 3 participants 5 messages Oct 24, 2017 – Oct 26, 2017 2 messages not in the archive

Subsystems (2)

InfiniBand CXGB4 iWARP RNIC driver (IW_CXGB4)

lore

Share & tools

lore

Leon Romanovsky

author

2017-10-24 18:28 UTC #0 lore raw

PATCH 1/1 RDMA/cxgb4: Remove unnecessary conversion from __be32 to cpu format

The scqe.stag is actually declared in u32 format and don't need conversion from __be32 to cpu format.

Remove the be32_to_cpu() and avoid the following sparse error.
drivers/infiniband/hw/cxgb4/cq.c:754:52: warning: cast to restricted __be32

Cc Steve Wise <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/hw/cxgb4/t4.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/infiniband/hw/cxgb4/t4.h +1 -1

index 2b44fa850bbb..a16f3596f5a8 100644
@@ -234,7 +234,7 @@ struct t4_cqe {
 
 /* used for SQ completion processing */
 #define CQE_WRID_SQ_IDX(x)	((x)->u.scqe.cidx)
-#define CQE_WRID_FR_STAG(x)     (be32_to_cpu((x)->u.scqe.stag))
+#define CQE_WRID_FR_STAG(x)     ((x)->u.scqe.stag)
 
 /* generic accessor macros */
 #define CQE_WRID_HI(x)		(be32_to_cpu((x)->u.gen.wrid_hi))

--
2.14.2

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

A message that is not in the archive (sent off-list or not yet received): 20171024182848.7945-1-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Steve Wise

2017-10-24 18:42 UTC #2 lore raw

Quoted 7 lines

The scqe.stag is actually declared in u32 format and don't need conversion from __be32 to cpu format.
Remove the be32_to_cpu() and avoid the following sparse error.
drivers/infiniband/hw/cxgb4/cq.c:754:52: warning: cast to restricted __be32

Review

Quoted 7 lines

Cc Steve Wise <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/cxgb4/t4.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0

b/drivers/infiniband/hw/cxgb4/t4.h

drivers/infiniband/hw/cxgb4/t4.h struct t4_cqe { · line 237

Quoted 9 lines

index 2b44fa850bbb..a16f3596f5a8 100644

drivers/infiniband/hw/cxgb4/t4.h +1 -1

@@ -234,7 +234,7 @@ struct t4_cqe {
 
 /* used for SQ completion processing */
 #define CQE_WRID_SQ_IDX(x)	((x)->u.scqe.cidx)
-#define CQE_WRID_FR_STAG(x)     (be32_to_cpu((x)->u.scqe.stag))
+#define CQE_WRID_FR_STAG(x)     ((x)->u.scqe.stag)

This is incorrect. The stag is filled in by HW which is BE. The declaration of scqe.stag needs to be __be32.

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

Leon Romanovsky

author

2017-10-24 18:59 UTC #3 lore raw

Quoted 20 lines · Steve Wise

Quoted 7 lines
The scqe.stag is actually declared in u32 format and don't need conversion from __be32 to cpu format.
Remove the be32_to_cpu() and avoid the following sparse error.
drivers/infiniband/hw/cxgb4/cq.c:754:52: warning: cast to restricted __be32
Review
Quoted 7 lines
Cc Steve Wise <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/cxgb4/t4.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/infiniband/hw/cxgb4/t4.h
Quoted 5 lines
index 2b44fa850bbb..a16f3596f5a8 100644
drivers/infiniband/hw/cxgb4/t4.h +0 -0
@@ -234,7 +234,7 @@ struct t4_cqe {
 

Quoted 7 lines

drivers/infiniband/hw/cxgb4/t4.h struct t4_cqe { · line 237
Quoted 4 lines
drivers/infiniband/hw/cxgb4/t4.h +1 -1
 /* used for SQ completion processing */
 #define CQE_WRID_SQ_IDX(x)	((x)->u.scqe.cidx)
-#define CQE_WRID_FR_STAG(x)     (be32_to_cpu((x)->u.scqe.stag))
+#define CQE_WRID_FR_STAG(x)     ((x)->u.scqe.stag)
This is incorrect. The stag is filled in by HW which is BE. The declaration of scqe.stag needs to be __be32.

So why do you declare stag as u32?

drivers/infiniband/hw/cxgb4/cq.c:
676 static int c4iw_poll_cq_one(struct c4iw_cq *chp, struct ib_wc *wc)
...
679 struct t4_cqe uninitialized_var(cqe), *rd_cqe;
...
753 c4iw_invalidate_mr(qhp->rhp,
754 CQE_WRID_FR_STAG(&cqe));

....
drivers/infiniband/hw/cxgb4/t4.h:
162 /*
163 * CQE defs
164 */
165 struct t4_cqe {

166         __be32 header;
167         __be32 len;
168         union {

169 struct {
170 __be32 stag;
171 __be32 msn;
172 } rcqe;
173 struct {

174                         u32 stag;
175                         u16 nada2;
176                         u16 cidx;

177 } scqe;
178 struct {
179 __be32 wrid_hi;
180 __be32 wrid_low;
181 } gen;
182 u64 drain_cookie;

183         } u;
184         __be64 reserved;
185         __be64 bits_type_ts;

186 };

Thanks

Quoted 6 lines

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

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

Steve Wise

2017-10-25 14:46 UTC #5 lore raw

Quoted 8 lines

Quoted 8 lines
Review struct t4_cqe { · line 237
Quoted 6 lines
(quoted diff) +1 -1
@@ -234,7 +234,7 @@ struct t4_cqe {
 
 /* used for SQ completion processing */
 #define CQE_WRID_SQ_IDX(x)	((x)->u.scqe.cidx)
-#define CQE_WRID_FR_STAG(x)     (be32_to_cpu((x)->u.scqe.stag))
+#define CQE_WRID_FR_STAG(x)     ((x)->u.scqe.stag)
This is incorrect. The stag is filled in by HW which is BE. The

declaration of

Quoted 3 lines

Quoted 1 line
scqe.stag needs to be __be32.
So why do you declare stag as u32?

I'm saying it is a bug that stag is declared as u32. t4_cqe.u.scqe.stag should be declared as __be32.

So the fix for the sparse warning should be something like this:

drivers/infiniband/hw/cxgb4/t4.h +1 -1

index e765c00..bcb80ca6 100644
@@ -171,7 +171,7 @@ struct t4_cqe {
                        __be32 msn;
                } rcqe;
                struct {
-                       u32 stag;
+                       __be32 stag;
                        u16 nada2;
                        u16 cidx;
                } scqe;

Steve.

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

Tom Talpey

2017-10-26 05:00 UTC #6 lore raw

Quoted 23 lines · Steve Wise

Quoted 8 lines
Quoted 8 lines
Review struct t4_cqe { · line 237
Quoted 6 lines
(quoted diff) +1 -1
@@ -234,7 +234,7 @@ struct t4_cqe {
 
  /* used for SQ completion processing */
  #define CQE_WRID_SQ_IDX(x)	((x)->u.scqe.cidx)
-#define CQE_WRID_FR_STAG(x)     (be32_to_cpu((x)->u.scqe.stag))
+#define CQE_WRID_FR_STAG(x)     ((x)->u.scqe.stag)
This is incorrect. The stag is filled in by HW which is BE. The
declaration of
Quoted 3 lines
Quoted 1 line
scqe.stag needs to be __be32.
So why do you declare stag as u32?
I'm saying it is a bug that stag is declared as u32. t4_cqe.u.scqe.stag should be declared as __be32.
So the fix for the sparse warning should be something like this:
drivers/infiniband/hw/cxgb4/t4.h +0 -0
index e765c00..bcb80ca6 100644

drivers/infiniband/hw/cxgb4/t4.h struct t4_cqe { · line 174

Quoted 5 lines

drivers/infiniband/hw/cxgb4/t4.h +1 -1

@@ -171,7 +171,7 @@ struct t4_cqe {
                         __be32 msn;
                 } rcqe;
                 struct {
-                       u32 stag;
+                       __be32 stag;

Technically speaking, the stag is opaque to software and should be declared as a non-integer type, e.g. u8 stag[4]. However, most code treats it as a native 32-bit type, and performs integer stores to pass it in the WR. If declaring as __be32 achieves that without manipulating byte order, well, ok, but it's not perfectly accurate, type-wise.

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