diff mbox series

[v2,for-next,04/10] RDMA/hns: Optimize hns_roce_alloc_vf_resource()

Message ID 1584674622-52773-5-git-send-email-liweihang@huawei.com (mailing list archive)
State Mainlined
Commit 99e713f8daf8e0ddb728ba543a05a2b67d8c47cc
Delegated to: Jason Gunthorpe
Headers show
Series RDMA/hns: Various cleanups | expand

Commit Message

Weihang Li March 20, 2020, 3:23 a.m. UTC 
From: Lijun Ou <oulijun@huawei.com>

The capbilities of hardware should be got at first and then used in
hns_roce_alloc_vf_resource(). Also removes an unnecessary if ... else
condition in it.

Signed-off-by: Lijun Ou <oulijun@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 143 +++++++++++++----------------
 1 file changed, 62 insertions(+), 81 deletions(-)

Comments

Jason Gunthorpe March 26, 2020, 7:51 p.m. UTC | #1 
On Fri, Mar 20, 2020 at 11:23:36AM +0800, Weihang Li wrote:

> @@ -2028,6 +2002,13 @@ static int hns_roce_v2_profile(struct hns_roce_dev *hr_dev)
>  	if (ret)
>  		set_default_caps(hr_dev);
>  
> +	ret = hns_roce_alloc_vf_resource(hr_dev);
> +	if (ret) {
> +		dev_err(hr_dev->dev, "Allocate vf resource fail, ret = %d.\n",
> +			ret);
> +		return ret;
> +	}

It is unfortunate these have to remain as dev_err()

I've thought about setting the name during ib_alloc_dev, which would
avoid this, what do you think?

Jason
Weihang Li March 27, 2020, 7:09 a.m. UTC | #2 
On 2020/3/27 3:51, Jason Gunthorpe wrote:
> On Fri, Mar 20, 2020 at 11:23:36AM +0800, Weihang Li wrote:
> 
>> @@ -2028,6 +2002,13 @@ static int hns_roce_v2_profile(struct hns_roce_dev *hr_dev)
>>  	if (ret)
>>  		set_default_caps(hr_dev);
>>  
>> +	ret = hns_roce_alloc_vf_resource(hr_dev);
>> +	if (ret) {
>> +		dev_err(hr_dev->dev, "Allocate vf resource fail, ret = %d.\n",
>> +			ret);
>> +		return ret;
>> +	}
> 
> It is unfortunate these have to remain as dev_err()
> 
> I've thought about setting the name during ib_alloc_dev, which would
> avoid this, what do you think?
> 
> Jason
> 

Hi Jason,

Thanks for your comments. I agree with you and make a simple test by just
moving assign_name() into _ib_alloc_device(), and ibdev_*() works fine
anywhere in hns. But I'm not sure if there are any side effects.

I noticed that the code about name assignment is implemented by you, could
you please explain why put assign_name() in ib_register_device() rather than
ib_alloc_device()?

Thanks
Weihang
Jason Gunthorpe March 27, 2020, 12:36 p.m. UTC | #3 
On Fri, Mar 27, 2020 at 07:09:02AM +0000, liweihang wrote:
> On 2020/3/27 3:51, Jason Gunthorpe wrote:
> > On Fri, Mar 20, 2020 at 11:23:36AM +0800, Weihang Li wrote:
> > 
> >> @@ -2028,6 +2002,13 @@ static int hns_roce_v2_profile(struct hns_roce_dev *hr_dev)
> >>  	if (ret)
> >>  		set_default_caps(hr_dev);
> >>  
> >> +	ret = hns_roce_alloc_vf_resource(hr_dev);
> >> +	if (ret) {
> >> +		dev_err(hr_dev->dev, "Allocate vf resource fail, ret = %d.\n",
> >> +			ret);
> >> +		return ret;
> >> +	}
> > 
> > It is unfortunate these have to remain as dev_err()
> > 
> > I've thought about setting the name during ib_alloc_dev, which would
> > avoid this, what do you think?
> > 
> > Jason
> > 
> 
> Hi Jason,
> 
> Thanks for your comments. I agree with you and make a simple test by just
> moving assign_name() into _ib_alloc_device(), and ibdev_*() works fine
> anywhere in hns. But I'm not sure if there are any side effects.

Hmm. It actually looks like it should work now, older versions may
have had problems, but this looks OK.

Jason
Weihang Li March 28, 2020, 1:55 a.m. UTC | #4 
On 2020/3/27 20:36, Jason Gunthorpe wrote:
> On Fri, Mar 27, 2020 at 07:09:02AM +0000, liweihang wrote:
>> On 2020/3/27 3:51, Jason Gunthorpe wrote:
>>> On Fri, Mar 20, 2020 at 11:23:36AM +0800, Weihang Li wrote:
>>>
>>>> @@ -2028,6 +2002,13 @@ static int hns_roce_v2_profile(struct hns_roce_dev *hr_dev)
>>>>  	if (ret)
>>>>  		set_default_caps(hr_dev);
>>>>  
>>>> +	ret = hns_roce_alloc_vf_resource(hr_dev);
>>>> +	if (ret) {
>>>> +		dev_err(hr_dev->dev, "Allocate vf resource fail, ret = %d.\n",
>>>> +			ret);
>>>> +		return ret;
>>>> +	}
>>>
>>> It is unfortunate these have to remain as dev_err()
>>>
>>> I've thought about setting the name during ib_alloc_dev, which would
>>> avoid this, what do you think?
>>>
>>> Jason
>>>
>>
>> Hi Jason,
>>
>> Thanks for your comments. I agree with you and make a simple test by just
>> moving assign_name() into _ib_alloc_device(), and ibdev_*() works fine
>> anywhere in hns. But I'm not sure if there are any side effects.
> 
> Hmm. It actually looks like it should work now, older versions may
> have had problems, but this looks OK.
> 
> Jason
> 
OK, I will make a series to modify.

Thank you
Weihang
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 518a649..aff7c5d 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -1432,82 +1432,63 @@  static int hns_roce_alloc_vf_resource(struct hns_roce_dev *hr_dev)
 			desc[i].flag |= cpu_to_le16(HNS_ROCE_CMD_FLAG_NEXT);
 		else
 			desc[i].flag &= ~cpu_to_le16(HNS_ROCE_CMD_FLAG_NEXT);
-
-		if (i == 0) {
-			roce_set_field(req_a->vf_qpc_bt_idx_num,
-				       VF_RES_A_DATA_1_VF_QPC_BT_IDX_M,
-				       VF_RES_A_DATA_1_VF_QPC_BT_IDX_S, 0);
-			roce_set_field(req_a->vf_qpc_bt_idx_num,
-				       VF_RES_A_DATA_1_VF_QPC_BT_NUM_M,
-				       VF_RES_A_DATA_1_VF_QPC_BT_NUM_S,
-				       HNS_ROCE_VF_QPC_BT_NUM);
-
-			roce_set_field(req_a->vf_srqc_bt_idx_num,
-				       VF_RES_A_DATA_2_VF_SRQC_BT_IDX_M,
-				       VF_RES_A_DATA_2_VF_SRQC_BT_IDX_S, 0);
-			roce_set_field(req_a->vf_srqc_bt_idx_num,
-				       VF_RES_A_DATA_2_VF_SRQC_BT_NUM_M,
-				       VF_RES_A_DATA_2_VF_SRQC_BT_NUM_S,
-				       HNS_ROCE_VF_SRQC_BT_NUM);
-
-			roce_set_field(req_a->vf_cqc_bt_idx_num,
-				       VF_RES_A_DATA_3_VF_CQC_BT_IDX_M,
-				       VF_RES_A_DATA_3_VF_CQC_BT_IDX_S, 0);
-			roce_set_field(req_a->vf_cqc_bt_idx_num,
-				       VF_RES_A_DATA_3_VF_CQC_BT_NUM_M,
-				       VF_RES_A_DATA_3_VF_CQC_BT_NUM_S,
-				       HNS_ROCE_VF_CQC_BT_NUM);
-
-			roce_set_field(req_a->vf_mpt_bt_idx_num,
-				       VF_RES_A_DATA_4_VF_MPT_BT_IDX_M,
-				       VF_RES_A_DATA_4_VF_MPT_BT_IDX_S, 0);
-			roce_set_field(req_a->vf_mpt_bt_idx_num,
-				       VF_RES_A_DATA_4_VF_MPT_BT_NUM_M,
-				       VF_RES_A_DATA_4_VF_MPT_BT_NUM_S,
-				       HNS_ROCE_VF_MPT_BT_NUM);
-
-			roce_set_field(req_a->vf_eqc_bt_idx_num,
-				       VF_RES_A_DATA_5_VF_EQC_IDX_M,
-				       VF_RES_A_DATA_5_VF_EQC_IDX_S, 0);
-			roce_set_field(req_a->vf_eqc_bt_idx_num,
-				       VF_RES_A_DATA_5_VF_EQC_NUM_M,
-				       VF_RES_A_DATA_5_VF_EQC_NUM_S,
-				       HNS_ROCE_VF_EQC_NUM);
-		} else {
-			roce_set_field(req_b->vf_smac_idx_num,
-				       VF_RES_B_DATA_1_VF_SMAC_IDX_M,
-				       VF_RES_B_DATA_1_VF_SMAC_IDX_S, 0);
-			roce_set_field(req_b->vf_smac_idx_num,
-				       VF_RES_B_DATA_1_VF_SMAC_NUM_M,
-				       VF_RES_B_DATA_1_VF_SMAC_NUM_S,
-				       HNS_ROCE_VF_SMAC_NUM);
-
-			roce_set_field(req_b->vf_sgid_idx_num,
-				       VF_RES_B_DATA_2_VF_SGID_IDX_M,
-				       VF_RES_B_DATA_2_VF_SGID_IDX_S, 0);
-			roce_set_field(req_b->vf_sgid_idx_num,
-				       VF_RES_B_DATA_2_VF_SGID_NUM_M,
-				       VF_RES_B_DATA_2_VF_SGID_NUM_S,
-				       HNS_ROCE_VF_SGID_NUM);
-
-			roce_set_field(req_b->vf_qid_idx_sl_num,
-				       VF_RES_B_DATA_3_VF_QID_IDX_M,
-				       VF_RES_B_DATA_3_VF_QID_IDX_S, 0);
-			roce_set_field(req_b->vf_qid_idx_sl_num,
-				       VF_RES_B_DATA_3_VF_SL_NUM_M,
-				       VF_RES_B_DATA_3_VF_SL_NUM_S,
-				       HNS_ROCE_VF_SL_NUM);
-
-			roce_set_field(req_b->vf_sccc_idx_num,
-				       VF_RES_B_DATA_4_VF_SCCC_BT_IDX_M,
-				       VF_RES_B_DATA_4_VF_SCCC_BT_IDX_S, 0);
-			roce_set_field(req_b->vf_sccc_idx_num,
-				       VF_RES_B_DATA_4_VF_SCCC_BT_NUM_M,
-				       VF_RES_B_DATA_4_VF_SCCC_BT_NUM_S,
-				       HNS_ROCE_VF_SCCC_BT_NUM);
-		}
 	}
 
+	roce_set_field(req_a->vf_qpc_bt_idx_num,
+		       VF_RES_A_DATA_1_VF_QPC_BT_IDX_M,
+		       VF_RES_A_DATA_1_VF_QPC_BT_IDX_S, 0);
+	roce_set_field(req_a->vf_qpc_bt_idx_num,
+		       VF_RES_A_DATA_1_VF_QPC_BT_NUM_M,
+		       VF_RES_A_DATA_1_VF_QPC_BT_NUM_S, HNS_ROCE_VF_QPC_BT_NUM);
+
+	roce_set_field(req_a->vf_srqc_bt_idx_num,
+		       VF_RES_A_DATA_2_VF_SRQC_BT_IDX_M,
+		       VF_RES_A_DATA_2_VF_SRQC_BT_IDX_S, 0);
+	roce_set_field(req_a->vf_srqc_bt_idx_num,
+		       VF_RES_A_DATA_2_VF_SRQC_BT_NUM_M,
+		       VF_RES_A_DATA_2_VF_SRQC_BT_NUM_S,
+		       HNS_ROCE_VF_SRQC_BT_NUM);
+
+	roce_set_field(req_a->vf_cqc_bt_idx_num,
+		       VF_RES_A_DATA_3_VF_CQC_BT_IDX_M,
+		       VF_RES_A_DATA_3_VF_CQC_BT_IDX_S, 0);
+	roce_set_field(req_a->vf_cqc_bt_idx_num,
+		       VF_RES_A_DATA_3_VF_CQC_BT_NUM_M,
+		       VF_RES_A_DATA_3_VF_CQC_BT_NUM_S, HNS_ROCE_VF_CQC_BT_NUM);
+
+	roce_set_field(req_a->vf_mpt_bt_idx_num,
+		       VF_RES_A_DATA_4_VF_MPT_BT_IDX_M,
+		       VF_RES_A_DATA_4_VF_MPT_BT_IDX_S, 0);
+	roce_set_field(req_a->vf_mpt_bt_idx_num,
+		       VF_RES_A_DATA_4_VF_MPT_BT_NUM_M,
+		       VF_RES_A_DATA_4_VF_MPT_BT_NUM_S, HNS_ROCE_VF_MPT_BT_NUM);
+
+	roce_set_field(req_a->vf_eqc_bt_idx_num, VF_RES_A_DATA_5_VF_EQC_IDX_M,
+		       VF_RES_A_DATA_5_VF_EQC_IDX_S, 0);
+	roce_set_field(req_a->vf_eqc_bt_idx_num, VF_RES_A_DATA_5_VF_EQC_NUM_M,
+		       VF_RES_A_DATA_5_VF_EQC_NUM_S, HNS_ROCE_VF_EQC_NUM);
+
+	roce_set_field(req_b->vf_smac_idx_num, VF_RES_B_DATA_1_VF_SMAC_IDX_M,
+		       VF_RES_B_DATA_1_VF_SMAC_IDX_S, 0);
+	roce_set_field(req_b->vf_smac_idx_num, VF_RES_B_DATA_1_VF_SMAC_NUM_M,
+		       VF_RES_B_DATA_1_VF_SMAC_NUM_S, HNS_ROCE_VF_SMAC_NUM);
+
+	roce_set_field(req_b->vf_sgid_idx_num, VF_RES_B_DATA_2_VF_SGID_IDX_M,
+		       VF_RES_B_DATA_2_VF_SGID_IDX_S, 0);
+	roce_set_field(req_b->vf_sgid_idx_num, VF_RES_B_DATA_2_VF_SGID_NUM_M,
+		       VF_RES_B_DATA_2_VF_SGID_NUM_S, HNS_ROCE_VF_SGID_NUM);
+
+	roce_set_field(req_b->vf_qid_idx_sl_num, VF_RES_B_DATA_3_VF_QID_IDX_M,
+		       VF_RES_B_DATA_3_VF_QID_IDX_S, 0);
+	roce_set_field(req_b->vf_qid_idx_sl_num, VF_RES_B_DATA_3_VF_SL_NUM_M,
+		       VF_RES_B_DATA_3_VF_SL_NUM_S, HNS_ROCE_VF_SL_NUM);
+
+	roce_set_field(req_b->vf_sccc_idx_num, VF_RES_B_DATA_4_VF_SCCC_BT_IDX_M,
+		       VF_RES_B_DATA_4_VF_SCCC_BT_IDX_S, 0);
+	roce_set_field(req_b->vf_sccc_idx_num, VF_RES_B_DATA_4_VF_SCCC_BT_NUM_M,
+		       VF_RES_B_DATA_4_VF_SCCC_BT_NUM_S,
+		       HNS_ROCE_VF_SCCC_BT_NUM);
+
 	return hns_roce_cmq_send(hr_dev, desc, 2);
 }
 
@@ -2001,13 +1982,6 @@  static int hns_roce_v2_profile(struct hns_roce_dev *hr_dev)
 		}
 	}
 
-	ret = hns_roce_alloc_vf_resource(hr_dev);
-	if (ret) {
-		dev_err(hr_dev->dev, "Allocate vf resource fail, ret = %d.\n",
-			ret);
-		return ret;
-	}
-
 	hr_dev->vendor_part_id = hr_dev->pci_dev->device;
 	hr_dev->sys_image_guid = be64_to_cpu(hr_dev->ib_dev.node_guid);
 
@@ -2028,6 +2002,13 @@  static int hns_roce_v2_profile(struct hns_roce_dev *hr_dev)
 	if (ret)
 		set_default_caps(hr_dev);
 
+	ret = hns_roce_alloc_vf_resource(hr_dev);
+	if (ret) {
+		dev_err(hr_dev->dev, "Allocate vf resource fail, ret = %d.\n",
+			ret);
+		return ret;
+	}
+
 	ret = hns_roce_v2_set_bt(hr_dev);
 	if (ret)
 		dev_err(hr_dev->dev, "Configure bt attribute fail, ret = %d.\n",