Endianess in cxgb4

Endianess in cxgb4

[Leon Romanovsky]

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

Hi Steve,

I reviewed my reports from various static checkers and saw strange code
in build_tpte_memreg() function that in drivers/infiniband/hw/cxgb4/qp.c.

That function has following code:
 614         fr->r2 = cpu_to_be32(0);
 615         fr->stag = cpu_to_be32(mhp->ibmr.rkey);

The r2, stag and rkey are u32, why do you need cpu_to_be32() conversion?

The error report which I got is:

 134 drivers/infiniband/hw/cxgb4/qp.c:614:16: warning: incorrect type in assignment (different base types)
 135 drivers/infiniband/hw/cxgb4/qp.c:614:16:    expected unsigned int [unsigned] [usertype] r2
 136 drivers/infiniband/hw/cxgb4/qp.c:614:16:    got restricted __be32 [usertype] <noident>
 137 drivers/infiniband/hw/cxgb4/qp.c:615:18: warning: incorrect type in assignment (different base types)
 138 drivers/infiniband/hw/cxgb4/qp.c:615:18:    expected unsigned int [unsigned] [usertype] stag
 139 drivers/infiniband/hw/cxgb4/qp.c:615:18:    got restricted __be32 [usertype] <noident>

Thanks

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

RE: Endianess in cxgb4

[Steve Wise]

> 
> Hi Steve,
> 
> I reviewed my reports from various static checkers and saw strange code
> in build_tpte_memreg() function that in drivers/infiniband/hw/cxgb4/qp.c.
> 
> That function has following code:
>  614         fr->r2 = cpu_to_be32(0);
>  615         fr->stag = cpu_to_be32(mhp->ibmr.rkey);
> 
> The r2, stag and rkey are u32, why do you need cpu_to_be32() conversion?
> 

Hey Leon,

The r2 and stag fields in struct fw_ri_fr_nsmr_tpte_wr should be __be32.

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: Endianess in cxgb4

[Leon Romanovsky]

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

On Tue, Oct 24, 2017 at 01:45:52PM -0500, Steve Wise wrote:
> >
> > Hi Steve,
> >
> > I reviewed my reports from various static checkers and saw strange code
> > in build_tpte_memreg() function that in drivers/infiniband/hw/cxgb4/qp.c.
> >
> > That function has following code:
> >  614         fr->r2 = cpu_to_be32(0);
> >  615         fr->stag = cpu_to_be32(mhp->ibmr.rkey);
> >
> > The r2, stag and rkey are u32, why do you need cpu_to_be32() conversion?
> >
>
> Hey Leon,
>
> The r2 and stag fields in struct fw_ri_fr_nsmr_tpte_wr should be __be32.

it is not how it is declared in drivers/infiniband/hw/cxgb4/t4fw_ri_api.h

672 struct fw_ri_fr_nsmr_tpte_wr {
673         __u8    opcode;
674         __u8   flags;
675         __u16  wrid;
676         __u8   r1[3];
677         __u8   len16;
678         __u32  r2;
679         __u32  stag;
680         struct fw_ri_tpte tpte;
681         __u64  pbl[2];
682 };

Thanks

>
> Steve.
>

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

RE: Endianess in cxgb4

[Steve Wise]

> On Tue, Oct 24, 2017 at 01:45:52PM -0500, Steve Wise wrote:
> > >
> > > Hi Steve,
> > >
> > > I reviewed my reports from various static checkers and saw strange code
> > > in build_tpte_memreg() function that in drivers/infiniband/hw/cxgb4/qp.c.
> > >
> > > That function has following code:
> > >  614         fr->r2 = cpu_to_be32(0);
> > >  615         fr->stag = cpu_to_be32(mhp->ibmr.rkey);
> > >
> > > The r2, stag and rkey are u32, why do you need cpu_to_be32() conversion?
> > >
> >
> > Hey Leon,
> >
> > The r2 and stag fields in struct fw_ri_fr_nsmr_tpte_wr should be __be32.
> 
> it is not how it is declared in drivers/infiniband/hw/cxgb4/t4fw_ri_api.h
> 
> 672 struct fw_ri_fr_nsmr_tpte_wr {
> 673         __u8    opcode;
> 674         __u8   flags;
> 675         __u16  wrid;
> 676         __u8   r1[3];
> 677         __u8   len16;
> 678         __u32  r2;
> 679         __u32  stag;
> 680         struct fw_ri_tpte tpte;
> 681         __u64  pbl[2];
> 682 };
> 
> Thanks
> 

I'm not communicating effectively.  Sorry.  

This is a bug in t4fw_ri_api.h.  You need create a patch to change
fw_ri_fr_nsmr_tpte_wr to make the stag and r2 fields to be __be32. 

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: Endianess in cxgb4

[Leon Romanovsky]

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

On Wed, Oct 25, 2017 at 09:48:08AM -0500, Steve Wise wrote:
> > On Tue, Oct 24, 2017 at 01:45:52PM -0500, Steve Wise wrote:
> > > >
> > > > Hi Steve,
> > > >
> > > > I reviewed my reports from various static checkers and saw strange code
> > > > in build_tpte_memreg() function that in drivers/infiniband/hw/cxgb4/qp.c.
> > > >
> > > > That function has following code:
> > > >  614         fr->r2 = cpu_to_be32(0);
> > > >  615         fr->stag = cpu_to_be32(mhp->ibmr.rkey);
> > > >
> > > > The r2, stag and rkey are u32, why do you need cpu_to_be32() conversion?
> > > >
> > >
> > > Hey Leon,
> > >
> > > The r2 and stag fields in struct fw_ri_fr_nsmr_tpte_wr should be __be32.
> >
> > it is not how it is declared in drivers/infiniband/hw/cxgb4/t4fw_ri_api.h
> >
> > 672 struct fw_ri_fr_nsmr_tpte_wr {
> > 673         __u8    opcode;
> > 674         __u8   flags;
> > 675         __u16  wrid;
> > 676         __u8   r1[3];
> > 677         __u8   len16;
> > 678         __u32  r2;
> > 679         __u32  stag;
> > 680         struct fw_ri_tpte tpte;
> > 681         __u64  pbl[2];
> > 682 };
> >
> > Thanks
> >
>
> I'm not communicating effectively.  Sorry.
>
> This is a bug in t4fw_ri_api.h.  You need create a patch to change
> fw_ri_fr_nsmr_tpte_wr to make the stag and r2 fields to be __be32.

Thanks, I'll send a fix.

>
> Steve.
>
>
>

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

RE: Endianess in cxgb4

[Steve Wise]

> > I'm not communicating effectively.  Sorry.
> >
> > This is a bug in t4fw_ri_api.h.  You need create a patch to change
> > fw_ri_fr_nsmr_tpte_wr to make the stag and r2 fields to be __be32.
> 
> Thanks, I'll send a fix.
> 

Thanks for doing this work! 

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: Endianess in cxgb4

[Jason Gunthorpe]

On Tue, Oct 24, 2017 at 09:37:59PM +0300, Leon Romanovsky wrote:
> Hi Steve,
> 
> I reviewed my reports from various static checkers and saw strange code
> in build_tpte_memreg() function that in drivers/infiniband/hw/cxgb4/qp.c.
> 
> That function has following code:
>  614         fr->r2 = cpu_to_be32(0);
>  615         fr->stag = cpu_to_be32(mhp->ibmr.rkey);
> 
> The r2, stag and rkey are u32, why do you need cpu_to_be32() conversion?
> 
> The error report which I got is:
> 
>  134 drivers/infiniband/hw/cxgb4/qp.c:614:16: warning: incorrect type in assignment (different base types)
>  135 drivers/infiniband/hw/cxgb4/qp.c:614:16:    expected unsigned int [unsigned] [usertype] r2
>  136 drivers/infiniband/hw/cxgb4/qp.c:614:16:    got restricted __be32 [usertype] <noident>
>  137 drivers/infiniband/hw/cxgb4/qp.c:615:18: warning: incorrect type in assignment (different base types)
>  138 drivers/infiniband/hw/cxgb4/qp.c:615:18:    expected unsigned int [unsigned] [usertype] stag
>  139 drivers/infiniband/hw/cxgb4/qp.c:615:18:    got restricted __be32 [usertype] <noident>

I looked at cxgb in userspace and gave up. It seemed to be storing be
and host values in the same memory locations :(

Jason
--
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: Endianess in cxgb4

[Steve Wise]

> On Tue, Oct 24, 2017 at 09:37:59PM +0300, Leon Romanovsky wrote:
> > Hi Steve,
> >
> > I reviewed my reports from various static checkers and saw strange code
> > in build_tpte_memreg() function that in drivers/infiniband/hw/cxgb4/qp.c.
> >
> > That function has following code:
> >  614         fr->r2 = cpu_to_be32(0);
> >  615         fr->stag = cpu_to_be32(mhp->ibmr.rkey);
> >
> > The r2, stag and rkey are u32, why do you need cpu_to_be32() conversion?
> >
> > The error report which I got is:
> >
> >  134 drivers/infiniband/hw/cxgb4/qp.c:614:16: warning: incorrect type in
> assignment (different base types)
> >  135 drivers/infiniband/hw/cxgb4/qp.c:614:16:    expected unsigned int
[unsigned]
> [usertype] r2
> >  136 drivers/infiniband/hw/cxgb4/qp.c:614:16:    got restricted __be32
[usertype]
> <noident>
> >  137 drivers/infiniband/hw/cxgb4/qp.c:615:18: warning: incorrect type in
> assignment (different base types)
> >  138 drivers/infiniband/hw/cxgb4/qp.c:615:18:    expected unsigned int
[unsigned]
> [usertype] stag
> >  139 drivers/infiniband/hw/cxgb4/qp.c:615:18:    got restricted __be32
[usertype]
> <noident>
> 
> I looked at cxgb in userspace and gave up. It seemed to be storing be
> and host values in the same memory locations :(

The Terminator HW/FW WRs are in BE always, with the exception that something
"opaque and owned by the host" is in HBO.  The only one I can think of is the
wrid field in each Terminator SQE.  It is provided by the host in each WR, and
is opaque to the HW/FW.  That is, the HW/FW does not interpret this 16b field,
but simply reflects it in the corresponding CQE.  

See my reply to Leon regarding the above issue...

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