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

Commit Message

Steve Wise Oct. 25, 2017, 2:46 p.m. UTC

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


Steve.

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

Comments

Tom Talpey Oct. 26, 2017, 5 a.m. UTC | #1

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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;