[PATCH for-next 0/2] Bugfixes for eq support in hns

[PATCH for-next 0/2] Bugfixes for eq support in hns

[Yixian Liu]

This patch set fixes the bugs according to Jason's two comments
on my previous patch set, which listed in the NOTE.

NOTE: This patch set is dependent on the following patches.
   "RDMA/hns: Add eq support of hip08"
   "RDMA/hns: Refactor eq code for hip06"

Yixian Liu (2):
  RDMA/hns: Add detailed comments for mb() call
  RDMA/hns: Fix alignment problem of the doorbell

 drivers/infiniband/hw/hns/hns_roce_device.h |  5 ++++
 drivers/infiniband/hw/hns/hns_roce_hw_v1.c  | 19 +++++++++++----
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c  | 38 ++++++++++++++---------------
 3 files changed, 38 insertions(+), 24 deletions(-)

-- 
1.9.1

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

[PATCH for-next 1/2] RDMA/hns: Add detailed comments for mb() call

[Yixian Liu]

This patch adds more detailed comments when we call the
memory barrier function, such as rmb, wmb and mb. Three
mb() callers are deleted since they are unnecessary.

Signed-off-by: Yixian Liu <liuyixian-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Lijun Ou <oulijun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 19 ++++++++++++++-----
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 22 +++++++++++-----------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
index 6100ace..abcc64e 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
@@ -3963,7 +3963,10 @@ static void set_eq_cons_index_v1(struct hns_roce_eq *eq, int req_not)
 {
 	roce_raw_write((eq->cons_index & HNS_ROCE_V1_CONS_IDX_M) |
 		      (req_not << eq->log_entries), eq->doorbell);
-	/* Memory barrier */
+
+	/* Make sure we update the consumer index of EQ before
+	 * accessing it later
+	 */
 	mb();
 }
 
@@ -4156,13 +4159,16 @@ static int hns_roce_v1_aeq_int(struct hns_roce_dev *hr_dev,
 	int event_type;
 
 	while ((aeqe = next_aeqe_sw_v1(eq))) {
+
+		/* Make sure we read the AEQ entry after we have checked the
+		 * ownership bit
+		 */
+		rmb();
+
 		dev_dbg(dev, "aeqe = %p, aeqe->asyn.event_type = 0x%lx\n", aeqe,
 			roce_get_field(aeqe->asyn,
 				       HNS_ROCE_AEQE_U32_4_EVENT_TYPE_M,
 				       HNS_ROCE_AEQE_U32_4_EVENT_TYPE_S));
-		/* Memory barrier */
-		rmb();
-
 		event_type = roce_get_field(aeqe->asyn,
 					    HNS_ROCE_AEQE_U32_4_EVENT_TYPE_M,
 					    HNS_ROCE_AEQE_U32_4_EVENT_TYPE_S);
@@ -4260,7 +4266,10 @@ static int hns_roce_v1_ceq_int(struct hns_roce_dev *hr_dev,
 	u32 cqn;
 
 	while ((ceqe = next_ceqe_sw_v1(eq))) {
-		/* Memory barrier */
+
+		/* Make sure we read CEQ entry after we have checked the
+		 * ownership bit
+		 */
 		rmb();
 		cqn = roce_get_field(ceqe->comp,
 				     HNS_ROCE_CEQE_CEQE_COMP_CQN_M,
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 04281d0..7f393f6 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -3200,7 +3200,9 @@ static void set_eq_cons_index_v2(struct hns_roce_eq *eq)
 
 	hns_roce_write64_k(doorbell, eq->doorbell);
 
-	/* Memory barrier */
+	/* Make sure we update the consumer index of EQ before
+	 * accessing it later
+	 */
 	mb();
 
 }
@@ -3384,7 +3386,10 @@ static int hns_roce_v2_aeq_int(struct hns_roce_dev *hr_dev,
 	int event_type;
 
 	while ((aeqe = next_aeqe_sw_v2(eq))) {
-		/* Memory barrier */
+
+		/* Make sure we read AEQ entry after we have checked the
+		 * ownership bit
+		 */
 		rmb();
 
 		event_type = roce_get_field(aeqe->asyn,
@@ -3500,8 +3505,11 @@ static int hns_roce_v2_ceq_int(struct hns_roce_dev *hr_dev,
 
 	while ((ceqe = next_ceqe_sw_v2(eq))) {
 
-		/* Memory barrier */
+		/* Make sure we read CEQ entry after we have checked the
+		 * ownership bit
+		 */
 		rmb();
+
 		cqn = roce_get_field(ceqe->comp,
 				     HNS_ROCE_V2_CEQE_COMP_CQN_M,
 				     HNS_ROCE_V2_CEQE_COMP_CQN_S);
@@ -3556,9 +3564,6 @@ static irqreturn_t hns_roce_v2_msix_interrupt_abn(int irq, void *dev_id)
 		roce_set_bit(int_st, HNS_ROCE_V2_VF_INT_ST_AEQ_OVERFLOW_S, 1);
 		roce_write(hr_dev, ROCEE_VF_ABN_INT_ST_REG, int_st);
 
-		/* Memory barrier */
-		mb();
-
 		roce_set_bit(int_en, HNS_ROCE_V2_VF_ABN_INT_EN_S, 1);
 		roce_write(hr_dev, ROCEE_VF_ABN_INT_EN_REG, int_en);
 
@@ -3569,9 +3574,6 @@ static irqreturn_t hns_roce_v2_msix_interrupt_abn(int irq, void *dev_id)
 		roce_set_bit(int_st, HNS_ROCE_V2_VF_INT_ST_BUS_ERR_S, 1);
 		roce_write(hr_dev, ROCEE_VF_ABN_INT_ST_REG, int_st);
 
-		/* Memory barrier */
-		mb();
-
 		roce_set_bit(int_en, HNS_ROCE_V2_VF_ABN_INT_EN_S, 1);
 		roce_write(hr_dev, ROCEE_VF_ABN_INT_EN_REG, int_en);
 
@@ -3582,8 +3584,6 @@ static irqreturn_t hns_roce_v2_msix_interrupt_abn(int irq, void *dev_id)
 		roce_set_bit(int_st, HNS_ROCE_V2_VF_INT_ST_OTHER_ERR_S, 1);
 		roce_write(hr_dev, ROCEE_VF_ABN_INT_ST_REG, int_st);
 
-		/* Memory barrier */
-		mb();
 		roce_set_bit(int_en, HNS_ROCE_V2_VF_ABN_INT_EN_S, 1);
 		roce_write(hr_dev, ROCEE_VF_ABN_INT_EN_REG, int_en);
 
-- 
1.9.1

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

[PATCH for-next 2/2] RDMA/hns: Fix alignment problem of the doorbell

[Yixian Liu]

The array defined for doorbell is not guaranteed to be 64 bit
aligned while we write it to the hardware with 64 bit
alignment required.

This patch fixes this problem by defining a union for doorbell
to make sure it 64 bit alignment.

Signed-off-by: Yixian Liu <liuyixian-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Lijun Ou <oulijun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/hns/hns_roce_device.h |  5 +++++
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c  | 16 ++++++++--------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
index dde5178..defc4ee 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -569,6 +569,11 @@ struct hns_roce_eq_table {
 	void __iomem		**eqc_base; /* only for hw v1 */
 };
 
+union hns_roce_db {
+	u64	doorbell64;
+	u32	doorbell[2];
+};
+
 struct hns_roce_caps {
 	u8		num_ports;
 	int		gid_table_len[HNS_ROCE_MAX_PORTS];
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 7f393f6..ee26bbd 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -3172,33 +3172,33 @@ static int hns_roce_v2_modify_cq(struct ib_cq *cq, u16 cq_count, u16 cq_period)
 
 static void set_eq_cons_index_v2(struct hns_roce_eq *eq)
 {
-	u32 doorbell[2];
+	union hns_roce_db db;
 
-	doorbell[0] = 0;
-	doorbell[1] = 0;
+	db.doorbell[0] = 0;
+	db.doorbell[1] = 0;
 
 	if (eq->type_flag == HNS_ROCE_AEQ) {
-		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
+		roce_set_field(db.doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
 			       HNS_ROCE_V2_EQ_DB_CMD_S,
 			       eq->arm_st == HNS_ROCE_V2_EQ_ALWAYS_ARMED ?
 			       HNS_ROCE_EQ_DB_CMD_AEQ :
 			       HNS_ROCE_EQ_DB_CMD_AEQ_ARMED);
 	} else {
-		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_TAG_M,
+		roce_set_field(db.doorbell[0], HNS_ROCE_V2_EQ_DB_TAG_M,
 			       HNS_ROCE_V2_EQ_DB_TAG_S, eq->eqn);
 
-		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
+		roce_set_field(db.doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
 			       HNS_ROCE_V2_EQ_DB_CMD_S,
 			       eq->arm_st == HNS_ROCE_V2_EQ_ALWAYS_ARMED ?
 			       HNS_ROCE_EQ_DB_CMD_CEQ :
 			       HNS_ROCE_EQ_DB_CMD_CEQ_ARMED);
 	}
 
-	roce_set_field(doorbell[1], HNS_ROCE_V2_EQ_DB_PARA_M,
+	roce_set_field(db.doorbell[1], HNS_ROCE_V2_EQ_DB_PARA_M,
 		       HNS_ROCE_V2_EQ_DB_PARA_S,
 		       (eq->cons_index & HNS_ROCE_V2_CONS_IDX_M));
 
-	hns_roce_write64_k(doorbell, eq->doorbell);
+	hns_roce_write64_k(db.doorbell, eq->doorbell);
 
 	/* Make sure we update the consumer index of EQ before
 	 * accessing it later
-- 
1.9.1

--
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: [PATCH for-next 1/2] RDMA/hns: Add detailed comments for mb() call

[Leon Romanovsky]

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

On Mon, Dec 25, 2017 at 09:37:18PM +0800, Yixian Liu wrote:
> This patch adds more detailed comments when we call the
> memory barrier function, such as rmb, wmb and mb. Three
> mb() callers are deleted since they are unnecessary.
>
> Signed-off-by: Yixian Liu <liuyixian-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Lijun Ou <oulijun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 19 ++++++++++++++-----
>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 22 +++++++++++-----------
>  2 files changed, 25 insertions(+), 16 deletions(-)
>

Thanks,
Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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

Re: [PATCH for-next 2/2] RDMA/hns: Fix alignment problem of the doorbell

[Leon Romanovsky]

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

On Mon, Dec 25, 2017 at 09:37:19PM +0800, Yixian Liu wrote:
> The array defined for doorbell is not guaranteed to be 64 bit
> aligned while we write it to the hardware with 64 bit
> alignment required.
>
> This patch fixes this problem by defining a union for doorbell
> to make sure it 64 bit alignment.

Are you sure that it gives you alignment?
The macros ALIGN/PTR_ALIGN are usually used for that.

Thanks

>
> Signed-off-by: Yixian Liu <liuyixian-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Lijun Ou <oulijun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/hns/hns_roce_device.h |  5 +++++
>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c  | 16 ++++++++--------
>  2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
> index dde5178..defc4ee 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> @@ -569,6 +569,11 @@ struct hns_roce_eq_table {
>  	void __iomem		**eqc_base; /* only for hw v1 */
>  };
>
> +union hns_roce_db {
> +	u64	doorbell64;
> +	u32	doorbell[2];
> +};
> +
>  struct hns_roce_caps {
>  	u8		num_ports;
>  	int		gid_table_len[HNS_ROCE_MAX_PORTS];
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> index 7f393f6..ee26bbd 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> @@ -3172,33 +3172,33 @@ static int hns_roce_v2_modify_cq(struct ib_cq *cq, u16 cq_count, u16 cq_period)
>
>  static void set_eq_cons_index_v2(struct hns_roce_eq *eq)
>  {
> -	u32 doorbell[2];
> +	union hns_roce_db db;
>
> -	doorbell[0] = 0;
> -	doorbell[1] = 0;
> +	db.doorbell[0] = 0;
> +	db.doorbell[1] = 0;
>
>  	if (eq->type_flag == HNS_ROCE_AEQ) {
> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
> +		roce_set_field(db.doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
>  			       HNS_ROCE_V2_EQ_DB_CMD_S,
>  			       eq->arm_st == HNS_ROCE_V2_EQ_ALWAYS_ARMED ?
>  			       HNS_ROCE_EQ_DB_CMD_AEQ :
>  			       HNS_ROCE_EQ_DB_CMD_AEQ_ARMED);
>  	} else {
> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_TAG_M,
> +		roce_set_field(db.doorbell[0], HNS_ROCE_V2_EQ_DB_TAG_M,
>  			       HNS_ROCE_V2_EQ_DB_TAG_S, eq->eqn);
>
> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
> +		roce_set_field(db.doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
>  			       HNS_ROCE_V2_EQ_DB_CMD_S,
>  			       eq->arm_st == HNS_ROCE_V2_EQ_ALWAYS_ARMED ?
>  			       HNS_ROCE_EQ_DB_CMD_CEQ :
>  			       HNS_ROCE_EQ_DB_CMD_CEQ_ARMED);
>  	}
>
> -	roce_set_field(doorbell[1], HNS_ROCE_V2_EQ_DB_PARA_M,
> +	roce_set_field(db.doorbell[1], HNS_ROCE_V2_EQ_DB_PARA_M,
>  		       HNS_ROCE_V2_EQ_DB_PARA_S,
>  		       (eq->cons_index & HNS_ROCE_V2_CONS_IDX_M));
>
> -	hns_roce_write64_k(doorbell, eq->doorbell);
> +	hns_roce_write64_k(db.doorbell, eq->doorbell);
>
>  	/* Make sure we update the consumer index of EQ before
>  	 * accessing it later
> --
> 1.9.1
>

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

Re: [PATCH for-next 2/2] RDMA/hns: Fix alignment problem of the doorbell

[Liuyixian (Eason)]

On 2017/12/26 16:15, Leon Romanovsky wrote:
> On Mon, Dec 25, 2017 at 09:37:19PM +0800, Yixian Liu wrote:
>> The array defined for doorbell is not guaranteed to be 64 bit
>> aligned while we write it to the hardware with 64 bit
>> alignment required.
>>
>> This patch fixes this problem by defining a union for doorbell
>> to make sure it 64 bit alignment.
> 
> Are you sure that it gives you alignment?
> The macros ALIGN/PTR_ALIGN are usually used for that.

Hi Leon,

I have considered your doubt combined with Jason's previous comment.
With a union defined with u64 member and assign the wanted value to it,
then we can deref *(u64 *)doorbell for the correct value.

In this patch set, the assignment is missed.
Thanks for your comment and I will fix it in v2.

> 
> Thanks
> 
>>
>> Signed-off-by: Yixian Liu <liuyixian-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Lijun Ou <oulijun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/infiniband/hw/hns/hns_roce_device.h |  5 +++++
>>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c  | 16 ++++++++--------
>>  2 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
>> index dde5178..defc4ee 100644
>> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
>> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
>> @@ -569,6 +569,11 @@ struct hns_roce_eq_table {
>>  	void __iomem		**eqc_base; /* only for hw v1 */
>>  };
>>
>> +union hns_roce_db {
>> +	u64	doorbell64;
>> +	u32	doorbell[2];
>> +};
>> +
>>  struct hns_roce_caps {
>>  	u8		num_ports;
>>  	int		gid_table_len[HNS_ROCE_MAX_PORTS];
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> index 7f393f6..ee26bbd 100644
>> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> @@ -3172,33 +3172,33 @@ static int hns_roce_v2_modify_cq(struct ib_cq *cq, u16 cq_count, u16 cq_period)
>>
>>  static void set_eq_cons_index_v2(struct hns_roce_eq *eq)
>>  {
>> -	u32 doorbell[2];
>> +	union hns_roce_db db;
>>
>> -	doorbell[0] = 0;
>> -	doorbell[1] = 0;
>> +	db.doorbell[0] = 0;
>> +	db.doorbell[1] = 0;
>>
>>  	if (eq->type_flag == HNS_ROCE_AEQ) {
>> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
>> +		roce_set_field(db.doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
>>  			       HNS_ROCE_V2_EQ_DB_CMD_S,
>>  			       eq->arm_st == HNS_ROCE_V2_EQ_ALWAYS_ARMED ?
>>  			       HNS_ROCE_EQ_DB_CMD_AEQ :
>>  			       HNS_ROCE_EQ_DB_CMD_AEQ_ARMED);
>>  	} else {
>> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_TAG_M,
>> +		roce_set_field(db.doorbell[0], HNS_ROCE_V2_EQ_DB_TAG_M,
>>  			       HNS_ROCE_V2_EQ_DB_TAG_S, eq->eqn);
>>
>> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
>> +		roce_set_field(db.doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
>>  			       HNS_ROCE_V2_EQ_DB_CMD_S,
>>  			       eq->arm_st == HNS_ROCE_V2_EQ_ALWAYS_ARMED ?
>>  			       HNS_ROCE_EQ_DB_CMD_CEQ :
>>  			       HNS_ROCE_EQ_DB_CMD_CEQ_ARMED);
>>  	}
>>
>> -	roce_set_field(doorbell[1], HNS_ROCE_V2_EQ_DB_PARA_M,
>> +	roce_set_field(db.doorbell[1], HNS_ROCE_V2_EQ_DB_PARA_M,
>>  		       HNS_ROCE_V2_EQ_DB_PARA_S,
>>  		       (eq->cons_index & HNS_ROCE_V2_CONS_IDX_M));
>>
>> -	hns_roce_write64_k(doorbell, eq->doorbell);
>> +	hns_roce_write64_k(db.doorbell, eq->doorbell);
>>
>>  	/* Make sure we update the consumer index of EQ before
>>  	 * accessing it later
>> --
>> 1.9.1
>>

--
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: [PATCH for-next 2/2] RDMA/hns: Fix alignment problem of the doorbell

[Jason Gunthorpe]

On Wed, Dec 27, 2017 at 06:01:25PM +0800, Liuyixian (Eason) wrote:
> 
> 
> On 2017/12/26 16:15, Leon Romanovsky wrote:
> > On Mon, Dec 25, 2017 at 09:37:19PM +0800, Yixian Liu wrote:
> >> The array defined for doorbell is not guaranteed to be 64 bit
> >> aligned while we write it to the hardware with 64 bit
> >> alignment required.
> >>
> >> This patch fixes this problem by defining a union for doorbell
> >> to make sure it 64 bit alignment.
> > 
> > Are you sure that it gives you alignment?
> > The macros ALIGN/PTR_ALIGN are usually used for that.
> 
> Hi Leon,
> 
> I have considered your doubt combined with Jason's previous comment.
> With a union defined with u64 member and assign the wanted value to it,
> then we can deref *(u64 *)doorbell for the correct value.
> 
> In this patch set, the assignment is missed.
> Thanks for your comment and I will fix it in v2.

You are not supposed to assign to doorbell64, you are supposed to read
from it inside hns_roce_write64_k instead of using a case.

The compiler guarentees it can read from all union members without
causing an alignment fault, which is all we want here. It does not
guarantee that the union is aligned to anything particular.

The v2 patch is very confusing.

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: [PATCH for-next 2/2] RDMA/hns: Fix alignment problem of the doorbell

[Ruhl, Michael J]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Yixian Liu
> Sent: Monday, December 25, 2017 8:37 AM
> To: jgg-uk2M96/98Pc@public.gmane.org; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: [PATCH for-next 2/2] RDMA/hns: Fix alignment problem of the
> doorbell
> 
> The array defined for doorbell is not guaranteed to be 64 bit
> aligned while we write it to the hardware with 64 bit
> alignment required.
> 
> This patch fixes this problem by defining a union for doorbell
> to make sure it 64 bit alignment.
> 
> Signed-off-by: Yixian Liu <liuyixian-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Lijun Ou <oulijun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/hns/hns_roce_device.h |  5 +++++
>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c  | 16 ++++++++--------
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h
> b/drivers/infiniband/hw/hns/hns_roce_device.h
> index dde5178..defc4ee 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> @@ -569,6 +569,11 @@ struct hns_roce_eq_table {
>  	void __iomem		**eqc_base; /* only for hw v1 */
>  };
> 
> +union hns_roce_db {
> +	u64	doorbell64;

Are there any concerns with big endian/little ending for this value?

Mike

> +	u32	doorbell[2];
> +};
> +
>  struct hns_roce_caps {
>  	u8		num_ports;
>  	int		gid_table_len[HNS_ROCE_MAX_PORTS];
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> index 7f393f6..ee26bbd 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> @@ -3172,33 +3172,33 @@ static int hns_roce_v2_modify_cq(struct ib_cq
> *cq, u16 cq_count, u16 cq_period)
> 
>  static void set_eq_cons_index_v2(struct hns_roce_eq *eq)
>  {
> -	u32 doorbell[2];
> +	union hns_roce_db db;
> 
> -	doorbell[0] = 0;
> -	doorbell[1] = 0;
> +	db.doorbell[0] = 0;
> +	db.doorbell[1] = 0;
> 
>  	if (eq->type_flag == HNS_ROCE_AEQ) {
> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
> +		roce_set_field(db.doorbell[0],
> HNS_ROCE_V2_EQ_DB_CMD_M,
>  			       HNS_ROCE_V2_EQ_DB_CMD_S,
>  			       eq->arm_st ==
> HNS_ROCE_V2_EQ_ALWAYS_ARMED ?
>  			       HNS_ROCE_EQ_DB_CMD_AEQ :
>  			       HNS_ROCE_EQ_DB_CMD_AEQ_ARMED);
>  	} else {
> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_TAG_M,
> +		roce_set_field(db.doorbell[0],
> HNS_ROCE_V2_EQ_DB_TAG_M,
>  			       HNS_ROCE_V2_EQ_DB_TAG_S, eq->eqn);
> 
> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
> +		roce_set_field(db.doorbell[0],
> HNS_ROCE_V2_EQ_DB_CMD_M,
>  			       HNS_ROCE_V2_EQ_DB_CMD_S,
>  			       eq->arm_st ==
> HNS_ROCE_V2_EQ_ALWAYS_ARMED ?
>  			       HNS_ROCE_EQ_DB_CMD_CEQ :
>  			       HNS_ROCE_EQ_DB_CMD_CEQ_ARMED);
>  	}
> 
> -	roce_set_field(doorbell[1], HNS_ROCE_V2_EQ_DB_PARA_M,
> +	roce_set_field(db.doorbell[1], HNS_ROCE_V2_EQ_DB_PARA_M,
>  		       HNS_ROCE_V2_EQ_DB_PARA_S,
>  		       (eq->cons_index & HNS_ROCE_V2_CONS_IDX_M));
> 
> -	hns_roce_write64_k(doorbell, eq->doorbell);
> +	hns_roce_write64_k(db.doorbell, eq->doorbell);
> 
>  	/* Make sure we update the consumer index of EQ before
>  	 * accessing it later
> --
> 1.9.1
> 
> --
> 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
--
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: [PATCH for-next 2/2] RDMA/hns: Fix alignment problem of the doorbell

[Liuyixian (Eason)]

On 2017/12/28 0:44, Jason Gunthorpe wrote:
> On Wed, Dec 27, 2017 at 06:01:25PM +0800, Liuyixian (Eason) wrote:
>>
>>
>> On 2017/12/26 16:15, Leon Romanovsky wrote:
>>> On Mon, Dec 25, 2017 at 09:37:19PM +0800, Yixian Liu wrote:
>>>> The array defined for doorbell is not guaranteed to be 64 bit
>>>> aligned while we write it to the hardware with 64 bit
>>>> alignment required.
>>>>
>>>> This patch fixes this problem by defining a union for doorbell
>>>> to make sure it 64 bit alignment.
>>>
>>> Are you sure that it gives you alignment?
>>> The macros ALIGN/PTR_ALIGN are usually used for that.
>>
>> Hi Leon,
>>
>> I have considered your doubt combined with Jason's previous comment.
>> With a union defined with u64 member and assign the wanted value to it,
>> then we can deref *(u64 *)doorbell for the correct value.
>>
>> In this patch set, the assignment is missed.
>> Thanks for your comment and I will fix it in v2.
> 
> You are not supposed to assign to doorbell64, you are supposed to read
> from it inside hns_roce_write64_k instead of using a case.
> 
> The compiler guarentees it can read from all union members without
> causing an alignment fault, which is all we want here. It does not
> guarantee that the union is aligned to anything particular.
> 
> The v2 patch is very confusing.
> 
> Jason
> 
> 

Hi Jason,

You are right that we could read doorbell64 inside hns_roce_write64_k,
then *(u64 *) can be deleted. I will update this in patch v3.

However, in order to get a correct value (both doorbell[0] and doorbell[1]
from doorbell64, the assignment before reading it seems necessary. I have
tested the cases w/o assignment, the results verified my conclusion.

And I also find a discussion on the usage of union. It also mentions that
we can only get the "active" member from an union.
https://stackoverflow.com/questions/2310483/purpose-of-unions-in-c-and-c

-------my test code for assignment---------------------------

Case 1 without assignment:
	int main() {
		union doorbell {
		u64 db64;
		u32 db32[2];
		} db;
	
		db.db32[0] = 0xBA98;
		db.db32[1] = 0xFEDC;
	//	db.db64 = (u64)db.db32[0] << 32 | db.db32[1];

		printf("db.db32 address is %p\n", &db.db32);
		printf("db.db64 address is %p\n", &db.db64);
		printf("db.db64 value is 0x%llx\n", db.db64);
	
		return 0;
	}

*Output1 for case 1*:
	db.db32 address is 0x7ffc0654f930
	db.db64 address is 0x7ffc0654f930
	db.db64 value is 0xba98

Case 2 with assignment: Uncomment the following line in case 1:
		db.db64 = (u64)db.db32[0] << 32 | db.db32[1];

*Output2 for case 2*:
	db.db32 address is 0x7fff63293a80
	db.db64 address is 0x7fff63293a80
	db.db64 value is 0xba980000fedc
---------------end of test-----------------------------------------
Output2 is what we want to get.

--
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: [PATCH for-next 2/2] RDMA/hns: Fix alignment problem of the doorbell

[Liuyixian (Eason)]

On 2017/12/28 1:40, Ruhl, Michael J wrote:
>> -----Original Message-----
>> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
>> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Yixian Liu
>> Sent: Monday, December 25, 2017 8:37 AM
>> To: jgg-uk2M96/98Pc@public.gmane.org; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
>> Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: [PATCH for-next 2/2] RDMA/hns: Fix alignment problem of the
>> doorbell
>>
>> The array defined for doorbell is not guaranteed to be 64 bit
>> aligned while we write it to the hardware with 64 bit
>> alignment required.
>>
>> This patch fixes this problem by defining a union for doorbell
>> to make sure it 64 bit alignment.
>>
>> Signed-off-by: Yixian Liu <liuyixian-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Lijun Ou <oulijun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/infiniband/hw/hns/hns_roce_device.h |  5 +++++
>>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c  | 16 ++++++++--------
>>  2 files changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h
>> b/drivers/infiniband/hw/hns/hns_roce_device.h
>> index dde5178..defc4ee 100644
>> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
>> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
>> @@ -569,6 +569,11 @@ struct hns_roce_eq_table {
>>  	void __iomem		**eqc_base; /* only for hw v1 */
>>  };
>>
>> +union hns_roce_db {
>> +	u64	doorbell64;
> 
> Are there any concerns with big endian/little ending for this value?
> 
> Mike
> 

I will consider this issue in patch v3, Mike, thank you!

>> +	u32	doorbell[2];
>> +};
>> +
>>  struct hns_roce_caps {
>>  	u8		num_ports;
>>  	int		gid_table_len[HNS_ROCE_MAX_PORTS];
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> index 7f393f6..ee26bbd 100644
>> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> @@ -3172,33 +3172,33 @@ static int hns_roce_v2_modify_cq(struct ib_cq
>> *cq, u16 cq_count, u16 cq_period)
>>
>>  static void set_eq_cons_index_v2(struct hns_roce_eq *eq)
>>  {
>> -	u32 doorbell[2];
>> +	union hns_roce_db db;
>>
>> -	doorbell[0] = 0;
>> -	doorbell[1] = 0;
>> +	db.doorbell[0] = 0;
>> +	db.doorbell[1] = 0;
>>
>>  	if (eq->type_flag == HNS_ROCE_AEQ) {
>> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
>> +		roce_set_field(db.doorbell[0],
>> HNS_ROCE_V2_EQ_DB_CMD_M,
>>  			       HNS_ROCE_V2_EQ_DB_CMD_S,
>>  			       eq->arm_st ==
>> HNS_ROCE_V2_EQ_ALWAYS_ARMED ?
>>  			       HNS_ROCE_EQ_DB_CMD_AEQ :
>>  			       HNS_ROCE_EQ_DB_CMD_AEQ_ARMED);
>>  	} else {
>> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_TAG_M,
>> +		roce_set_field(db.doorbell[0],
>> HNS_ROCE_V2_EQ_DB_TAG_M,
>>  			       HNS_ROCE_V2_EQ_DB_TAG_S, eq->eqn);
>>
>> -		roce_set_field(doorbell[0], HNS_ROCE_V2_EQ_DB_CMD_M,
>> +		roce_set_field(db.doorbell[0],
>> HNS_ROCE_V2_EQ_DB_CMD_M,
>>  			       HNS_ROCE_V2_EQ_DB_CMD_S,
>>  			       eq->arm_st ==
>> HNS_ROCE_V2_EQ_ALWAYS_ARMED ?
>>  			       HNS_ROCE_EQ_DB_CMD_CEQ :
>>  			       HNS_ROCE_EQ_DB_CMD_CEQ_ARMED);
>>  	}
>>
>> -	roce_set_field(doorbell[1], HNS_ROCE_V2_EQ_DB_PARA_M,
>> +	roce_set_field(db.doorbell[1], HNS_ROCE_V2_EQ_DB_PARA_M,
>>  		       HNS_ROCE_V2_EQ_DB_PARA_S,
>>  		       (eq->cons_index & HNS_ROCE_V2_CONS_IDX_M));
>>
>> -	hns_roce_write64_k(doorbell, eq->doorbell);
>> +	hns_roce_write64_k(db.doorbell, eq->doorbell);
>>
>>  	/* Make sure we update the consumer index of EQ before
>>  	 * accessing it later
>> --
>> 1.9.1
>>
>> --
>> 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
> 
> .
> 

--
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: [PATCH for-next 2/2] RDMA/hns: Fix alignment problem of the doorbell

[Liuyixian (Eason)]

On 2017/12/28 14:48, Liuyixian (Eason) wrote:
> 
> 
> On 2017/12/28 0:44, Jason Gunthorpe wrote:
>> On Wed, Dec 27, 2017 at 06:01:25PM +0800, Liuyixian (Eason) wrote:
>>>
>>>
>>> On 2017/12/26 16:15, Leon Romanovsky wrote:
>>>> On Mon, Dec 25, 2017 at 09:37:19PM +0800, Yixian Liu wrote:
>>>>> The array defined for doorbell is not guaranteed to be 64 bit
>>>>> aligned while we write it to the hardware with 64 bit
>>>>> alignment required.
>>>>>
>>>>> This patch fixes this problem by defining a union for doorbell
>>>>> to make sure it 64 bit alignment.
>>>>
>>>> Are you sure that it gives you alignment?
>>>> The macros ALIGN/PTR_ALIGN are usually used for that.
>>>
>>> Hi Leon,
>>>
>>> I have considered your doubt combined with Jason's previous comment.
>>> With a union defined with u64 member and assign the wanted value to it,
>>> then we can deref *(u64 *)doorbell for the correct value.
>>>
>>> In this patch set, the assignment is missed.
>>> Thanks for your comment and I will fix it in v2.
>>
>> You are not supposed to assign to doorbell64, you are supposed to read
>> from it inside hns_roce_write64_k instead of using a case.
>>
>> The compiler guarentees it can read from all union members without
>> causing an alignment fault, which is all we want here. It does not
>> guarantee that the union is aligned to anything particular.
>>
>> The v2 patch is very confusing.
>>
>> Jason
>>
>>
> 
> Hi Jason,
> 
> You are right that we could read doorbell64 inside hns_roce_write64_k,
> then *(u64 *) can be deleted. I will update this in patch v3.
> 
> However, in order to get a correct value (both doorbell[0] and doorbell[1]
> from doorbell64, the assignment before reading it seems necessary. I have
> tested the cases w/o assignment, the results verified my conclusion.
> 
> And I also find a discussion on the usage of union. It also mentions that
> we can only get the "active" member from an union.
> https://stackoverflow.com/questions/2310483/purpose-of-unions-in-c-and-c
> 
> -------my test code for assignment---------------------------
> 
> Case 1 without assignment:
> 	int main() {
> 		union doorbell {
> 		u64 db64;
> 		u32 db32[2];
> 		} db;
> 	
> 		db.db32[0] = 0xBA98;
> 		db.db32[1] = 0xFEDC;
> 	//	db.db64 = (u64)db.db32[0] << 32 | db.db32[1];
> 
> 		printf("db.db32 address is %p\n", &db.db32);
> 		printf("db.db64 address is %p\n", &db.db64);
> 		printf("db.db64 value is 0x%llx\n", db.db64);
> 	
> 		return 0;
> 	}
> 
> *Output1 for case 1*:
> 	db.db32 address is 0x7ffc0654f930
> 	db.db64 address is 0x7ffc0654f930
> 	db.db64 value is 0xba98
> 

Sorry, I have made a mistake.
I define u32 as "unsigned long" and my test code runs on a 64-bit platform.
This definition leads to the test result for case 1 to be wrong as Output1.
Without assignment, we could also get the correct value from doorbell64.

> Case 2 with assignment: Uncomment the following line in case 1:
> 		db.db64 = (u64)db.db32[0] << 32 | db.db32[1];
> 
> *Output2 for case 2*:
> 	db.db32 address is 0x7fff63293a80
> 	db.db64 address is 0x7fff63293a80
> 	db.db64 value is 0xba980000fedc
> ---------------end of test-----------------------------------------
> Output2 is what we want to get.
> 
> --
> 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
> 
> .
> 

--
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: [PATCH for-next 2/2] RDMA/hns: Fix alignment problem of the doorbell

[Jason Gunthorpe]

On Thu, Dec 28, 2017 at 02:48:10PM +0800, Liuyixian (Eason) wrote:

> And I also find a discussion on the usage of union. It also mentions that
> we can only get the "active" member from an union.
> https://stackoverflow.com/questions/2310483/purpose-of-unions-in-c-and-c

linux requires a compiler that supports the C99 model of allowing type
punning with unions.

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