[PATCH rdma-next] RDMA/cxgb4: Remove unnecessary conversion from __be32 to cpu format

[PATCH rdma-next] RDMA/cxgb4: Remove unnecessary conversion from __be32 to cpu format

[Leon Romanovsky]

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 <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/hw/cxgb4/t4.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/cxgb4/t4.h b/drivers/infiniband/hw/cxgb4/t4.h
index 2b44fa850bbb..a16f3596f5a8 100644
--- a/drivers/infiniband/hw/cxgb4/t4.h
+++ b/drivers/infiniband/hw/cxgb4/t4.h
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH rdma-next] RDMA/cxgb4: Remove unnecessary conversion from __be32 to cpu format

[Steve Wise]

> 
> 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 <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/infiniband/hw/cxgb4/t4.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb4/t4.h
b/drivers/infiniband/hw/cxgb4/t4.h
> index 2b44fa850bbb..a16f3596f5a8 100644
> --- a/drivers/infiniband/hw/cxgb4/t4.h
> +++ b/drivers/infiniband/hw/cxgb4/t4.h
> @@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next] RDMA/cxgb4: Remove unnecessary conversion from __be32 to cpu format

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2531 bytes --]

On Tue, Oct 24, 2017 at 01:42:16PM -0500, Steve Wise wrote:
> >
> > 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 <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> > Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/infiniband/hw/cxgb4/t4.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/hw/cxgb4/t4.h
> b/drivers/infiniband/hw/cxgb4/t4.h
> > index 2b44fa850bbb..a16f3596f5a8 100644
> > --- a/drivers/infiniband/hw/cxgb4/t4.h
> > +++ b/drivers/infiniband/hw/cxgb4/t4.h
> > @@ -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.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH rdma-next] RDMA/cxgb4: Remove unnecessary conversion from __be32 to cpu format

[Steve Wise]

> > > @@ -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.
> 
> 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:

diff --git a/drivers/infiniband/hw/cxgb4/t4.h b/drivers/infiniband/hw/cxgb4/t4.h
index e765c00..bcb80ca6 100644
--- a/drivers/infiniband/hw/cxgb4/t4.h
+++ b/drivers/infiniband/hw/cxgb4/t4.h
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next] RDMA/cxgb4: Remove unnecessary conversion from __be32 to cpu format

[Tom Talpey]

On 10/25/2017 7:46 AM, Steve Wise wrote:
>>>> @@ -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.
>>
>> 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:
> 
> diff --git a/drivers/infiniband/hw/cxgb4/t4.h b/drivers/infiniband/hw/cxgb4/t4.h
> index e765c00..bcb80ca6 100644
> --- a/drivers/infiniband/hw/cxgb4/t4.h
> +++ b/drivers/infiniband/hw/cxgb4/t4.h
> @@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html