On Thu, 2018-02-01 at 16:14 +0800, oulijun wrote:
> 在 2018/1/31 23:44, Doug Ledford 写道:
> > On Tue, 2018-01-30 at 20:20 +0800, Lijun Ou wrote:
> > > The hip06 and hip08 run on a little endian ARM, it needs to
> > > revise the annotations to indicate that the HW uses little
> > > endian data in the various DMA buffers, and flow the necessary
> > > swaps throughout.
> > >
> > > The imm_data use big endian mode. The cpu_to_le32/le32_to_cpu
> > > swaps are no-op for this, which makes the only substantive
> > > change the handling of imm_data which is now mandatory swapped.
> > >
> > > This also keep match with the userspace hns driver and resolve
> > > the warning by sparse.
> > >
> > > Signed-off-by: Lijun Ou <oulijun@xxxxxxxxxx>
> > > ---
> > > drivers/infiniband/hw/hns/hns_roce_common.h | 6 +-
> > > drivers/infiniband/hw/hns/hns_roce_device.h | 2 +-
> > > drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 57 ++++--
> > > drivers/infiniband/hw/hns/hns_roce_hw_v1.h | 258 ++++++++++++-------------
> > > drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 51 +++--
> > > drivers/infiniband/hw/hns/hns_roce_hw_v2.h | 283 ++++++++++++++--------------
> > > drivers/infiniband/hw/hns/hns_roce_main.c | 2 +-
> > > drivers/infiniband/hw/hns/hns_roce_qp.c | 18 +-
> > > 8 files changed, 357 insertions(+), 320 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/hns/hns_roce_common.h b/drivers/infiniband/hw/hns/hns_roce_common.h
> > > index dd67faf..319cb74 100644
> > > --- a/drivers/infiniband/hw/hns/hns_roce_common.h
> > > +++ b/drivers/infiniband/hw/hns/hns_roce_common.h
> > > @@ -43,15 +43,15 @@
> > > __raw_writel((__force u32)cpu_to_le32(value), (addr))
> > >
> > > #define roce_get_field(origin, mask, shift) \
> > > - (((origin) & (mask)) >> (shift))
> > > + (((le32_to_cpu(origin)) & (mask)) >> (shift))
> > >
> > > #define roce_get_bit(origin, shift) \
> > > roce_get_field((origin), (1ul << (shift)), (shift))
> > >
> > > #define roce_set_field(origin, mask, shift, val) \
> > > do { \
> > > - (origin) &= (~(mask)); \
> > > - (origin) |= (((u32)(val) << (shift)) & (mask)); \
> > > + (origin) &= ~cpu_to_le32(mask); \
> > > + (origin) |= cpu_to_le32(((u32)(val) << (shift)) & (mask)); \
> > > } while (0)
> > >
> > > #define roce_set_bit(origin, shift, val) \
> > > diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
> > > index 42c3b5a..2503d7f 100644
> > > --- a/drivers/infiniband/hw/hns/hns_roce_device.h
> > > +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> > > @@ -466,7 +466,7 @@ struct hns_roce_qp {
> > > struct ib_qp ibqp;
> > > struct hns_roce_buf hr_buf;
> > > struct hns_roce_wq rq;
> > > - __le64 doorbell_qpn;
> > > + u32 doorbell_qpn;
> >
> > Why the change in size here? Did you mean to go from 64bits down to
> > 32bits?
> >
>
> Maybe the 64bit is wasted after anlaysis. because the qpn of qpc and wqe are 24bit. the max value of qpn
> is 2 ^ 24 - 1 and the 32bit is enough.
Ok, I'm fine with that. I just wanted to make sure it was intentional.
> > >
> > > } else if (ibqp->qp_type == IB_QPT_RC) {
> > > ctrl = wqe;
> > > memset(ctrl, 0, sizeof(struct hns_roce_wqe_ctrl_seg));
> > > for (i = 0; i < wr->num_sge; i++)
> > > - ctrl->msg_length += wr->sg_list[i].length;
> > > + ctrl->msg_length =
> > > + cpu_to_le32(le32_to_cpu(ctrl->msg_length) +
> > > + wr->sg_list[i].length);
> >
> > Minor nit:
> >
> > Doing le32_to_cpu and cpu_to_le32 over and over again in a loop is
> > horribly inefficient. It would be much better IMO if you had a local
> > variable to use for the length, used that in the loop, and then only at
> > the end of the loop do a single cpu_to_le32 of the local variable to
> > store in msg_length. Same comment applies to the other spot in this
> > patch that does the same loop.
> >
>
>
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDDAttachment:
signature.asc
Description: This is a digitally signed message part
