On Thursday, May 05/17/18, 2018 at 00:37:37 +0530, Jason Gunthorpe wrote:
> On Thu, Apr 26, 2018 at 12:52:50PM +0530, Potnuri Bharat Teja wrote:
> > Adds iw_cxgb4 functionality to support RDMA_WRITE_WITH_IMMEDATE opcode.
> >
> > Signed-off-by: Potnuri Bharat Teja <bharat@xxxxxxxxxxx>
> > Signed-off-by: Steve Wise <swise@xxxxxxxxxxxxxxxxxxxxx>
> > drivers/infiniband/hw/cxgb4/cq.c | 23 +++++++++++++++++++---
> > drivers/infiniband/hw/cxgb4/qp.c | 32 +++++++++++++++++++++++--------
> > drivers/infiniband/hw/cxgb4/t4.h | 6 ++++++
> > drivers/infiniband/hw/cxgb4/t4fw_ri_api.h | 8 +++++---
> > include/uapi/rdma/cxgb4-abi.h | 3 ++-
> > 5 files changed, 57 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c
> > index cc95a9c270ae..1ccef76e2ae5 100644
> > +++ b/drivers/infiniband/hw/cxgb4/cq.c
> > @@ -791,15 +791,32 @@ static int c4iw_poll_cq_one(struct c4iw_cq *chp, struct ib_wc *wc)
> > wc->byte_len = CQE_LEN(&cqe);
> > else
> > wc->byte_len = 0;
> > - wc->opcode = IB_WC_RECV;
> > - if (CQE_OPCODE(&cqe) == FW_RI_SEND_WITH_INV ||
> > - CQE_OPCODE(&cqe) == FW_RI_SEND_WITH_SE_INV) {
> > +
> > + switch (CQE_OPCODE(&cqe)) {
> > + case FW_RI_SEND:
> > + wc->opcode = IB_WC_RECV;
> > + break;
> > + case FW_RI_SEND_WITH_INV:
> > + case FW_RI_SEND_WITH_SE_INV:
> > + wc->opcode = IB_WC_RECV;
> > wc->ex.invalidate_rkey = CQE_WRID_STAG(&cqe);
> > wc->wc_flags |= IB_WC_WITH_INVALIDATE;
> > c4iw_invalidate_mr(qhp->rhp, wc->ex.invalidate_rkey);
> > + break;
> > + case FW_RI_WRITE_IMMEDIATE:
> > + wc->opcode = IB_WC_RECV_RDMA_WITH_IMM;
> > + wc->ex.imm_data = CQE_IMM_DATA(&cqe);
>
> wc->ex.imm_data is a be32
>
> While the struct here is:
>
> > + struct {
> > + __be32 mo;
> > + __be32 msn;
> > + __u64 imm_data;
> > + } imm_data_rcqe;
>
> Is that right?
>
> This seems like a great way to make an endianess oopsie, ie it
> probably doesn't work on BE.
>
> The struct should define imm_data as a be32 at the right offset.
>
Agreed.
I beleive, immd_data field is not supposed to be BE. It is opaque to the
driver and iwarp protocol, perhaps its made BE to help interop between BE
and LE machines. Moreover iWARP protocol defines immediate data to be 64b.
So that is why the WR has it as a __u64.
Another concern here is, RDMA stack has it defined as __be32, I am not sure if
it is an IB limitation.
One possible way to fix this would be to change the __u64 to __be64 as iWARP
spec recommends it to be 64. and the force cast it from ib WR to iw_cxgb4 WR.
like this:
enum fw_ri_mpa_attrs {
@@ -546,7 +548,7 @@ struct fw_ri_rdma_write_wr {
__u16 wrid;
__u8 r1[3];
__u8 len16;
- __be64 r2;
+ __be64 immd_data;
----
+ if (wr->opcode == IB_WR_RDMA_WRITE_WITH_IMM)
+ wqe->write.immd_data = (__force __be64)wr->ex.imm_data;
+ else
----
+ case FW_RI_WRITE_IMMEDIATE:
+ wc->opcode = IB_WC_RECV_RDMA_WITH_IMM;
+ wc->ex.imm_data = (__force __be32)CQE_IMM_DATA(&cqe);
+ wc->wc_flags |= IB_WC_WITH_IMM;
> > enum {
> > - C4IW_QPF_ONCHIP = (1 << 0)
> > + C4IW_QPF_ONCHIP = (1 << 0),
> > + C4IW_QPF_WRITE_W_IMM = (1 << 1)
>
> Add the trailing , and don't neededlessly add white space (eg do not
> column align)
I'll fix it.
Thanks for the Review!
Bharat
>
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
