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: 3562 bytes --]

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?

> 
>  		} 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 --]