Re: [PATCH for-next 1/4] RDMA/hns: Fix the endian problem for hns

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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

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-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]