On 2018/12/6 17:14, Leon Romanovsky wrote:
> On Thu, Dec 06, 2018 at 04:53:53PM +0800, liyangyang (M) wrote:
>> Hi, Leon:
>>
>> Thanks a lot for your reply.
>>
>> On 2018/12/5 22:43, Leon Romanovsky wrote:
>>> On Wed, Dec 05, 2018 at 05:41:42PM +0800, Lijun Ou wrote:
>>>> From: Yangyang Li <liyangyang20@xxxxxxxxxx>
>>>>
>>>> This patch adds SCC context allocation and initialization support
>>>> for DCQCN in kernel space driver.
>>>>
>>>> Signed-off-by: Yangyang Li <liyangyang20@xxxxxxxxxx>
>>>> ---
>>>> drivers/infiniband/hw/hns/hns_roce_cmd.h | 4 ++++
>>>> drivers/infiniband/hw/hns/hns_roce_device.h | 6 +++++
>>>> drivers/infiniband/hw/hns/hns_roce_hem.c | 26 ++++++++++++++++++---
>>>> drivers/infiniband/hw/hns/hns_roce_hem.h | 1 +
>>>> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 35 ++++++++++++++++++++++++++++-
>>>> drivers/infiniband/hw/hns/hns_roce_hw_v2.h | 23 +++++++++++++++++--
>>>> drivers/infiniband/hw/hns/hns_roce_main.c | 18 +++++++++++++++
>>>> drivers/infiniband/hw/hns/hns_roce_qp.c | 20 ++++++++++++++++-
>>>> 8 files changed, 126 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_cmd.h b/drivers/infiniband/hw/hns/hns_roce_cmd.h
>>>> index 927701d..5348282 100644
>>>> --- a/drivers/infiniband/hw/hns/hns_roce_cmd.h
>>>> +++ b/drivers/infiniband/hw/hns/hns_roce_cmd.h
>>>> @@ -89,6 +89,10 @@ enum {
>>>> HNS_ROCE_CMD_DESTROY_SRQC_BT1 = 0x39,
>>>> HNS_ROCE_CMD_DESTROY_SRQC_BT2 = 0x3a,
>>>>
>>>> + /* SCC CTX BT commands */
>>>> + HNS_ROCE_CMD_READ_SCC_CTX_BT0 = 0xa4,
>>>> + HNS_ROCE_CMD_WRITE_SCC_CTX_BT0 = 0xa5,
>>>> +
>>>> /* EQC commands */
>>>> HNS_ROCE_CMD_CREATE_AEQC = 0x80,
>>>> HNS_ROCE_CMD_MODIFY_AEQC = 0x81,
>>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
>>>> index 779dd4c4..718b415 100644
>>>> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
>>>> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
>>>> @@ -482,6 +482,7 @@ struct hns_roce_qp_table {
>>>> struct hns_roce_hem_table qp_table;
>>>> struct hns_roce_hem_table irrl_table;
>>>> struct hns_roce_hem_table trrl_table;
>>>> + struct hns_roce_hem_table scc_ctx_table;
>>>> };
>>>>
>>>> struct hns_roce_cq_table {
>>>> @@ -768,6 +769,7 @@ struct hns_roce_caps {
>>>> int irrl_entry_sz;
>>>> int trrl_entry_sz;
>>>> int cqc_entry_sz;
>>>> + int scc_ctx_entry_sz;
>>>> int srqc_entry_sz;
>>>> int idx_entry_sz;
>>>> u32 pbl_ba_pg_sz;
>>>> @@ -780,6 +782,7 @@ struct hns_roce_caps {
>>>> u32 srqc_bt_num;
>>>> u32 cqc_bt_num;
>>>> u32 mpt_bt_num;
>>>> + u32 scc_ctx_bt_num;
>>>> u32 qpc_ba_pg_sz;
>>>> u32 qpc_buf_pg_sz;
>>>> u32 qpc_hop_num;
>>>> @@ -795,6 +798,9 @@ struct hns_roce_caps {
>>>> u32 mtt_ba_pg_sz;
>>>> u32 mtt_buf_pg_sz;
>>>> u32 mtt_hop_num;
>>>> + u32 scc_ctx_ba_pg_sz;
>>>> + u32 scc_ctx_buf_pg_sz;
>>>> + u32 scc_ctx_hop_num;
>>>> u32 cqe_ba_pg_sz;
>>>> u32 cqe_buf_pg_sz;
>>>> u32 cqe_hop_num;
>>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hem.c b/drivers/infiniband/hw/hns/hns_roce_hem.c
>>>> index 4cdbcaf..9e4a2ba 100644
>>>> --- a/drivers/infiniband/hw/hns/hns_roce_hem.c
>>>> +++ b/drivers/infiniband/hw/hns/hns_roce_hem.c
>>>> @@ -45,6 +45,7 @@ bool hns_roce_check_whether_mhop(struct hns_roce_dev *hr_dev, u32 type)
>>>> (hr_dev->caps.mpt_hop_num && type == HEM_TYPE_MTPT) ||
>>>> (hr_dev->caps.cqc_hop_num && type == HEM_TYPE_CQC) ||
>>>> (hr_dev->caps.srqc_hop_num && type == HEM_TYPE_SRQC) ||
>>>> + (hr_dev->caps.scc_ctx_hop_num && type == HEM_TYPE_SCC_CTX) ||
>>>> (hr_dev->caps.cqe_hop_num && type == HEM_TYPE_CQE) ||
>>>> (hr_dev->caps.mtt_hop_num && type == HEM_TYPE_MTT) ||
>>>> (hr_dev->caps.srqwqe_hop_num && type == HEM_TYPE_SRQWQE) ||
>>>> @@ -125,6 +126,14 @@ int hns_roce_calc_hem_mhop(struct hns_roce_dev *hr_dev,
>>>> mhop->ba_l0_num = hr_dev->caps.cqc_bt_num;
>>>> mhop->hop_num = hr_dev->caps.cqc_hop_num;
>>>> break;
>>>> + case HEM_TYPE_SCC_CTX:
>>>> + mhop->buf_chunk_size = 1 << (hr_dev->caps.scc_ctx_buf_pg_sz
>>>> + + PAGE_SHIFT);
>>>> + mhop->bt_chunk_size = 1 << (hr_dev->caps.scc_ctx_ba_pg_sz
>>>> + + PAGE_SHIFT);
>>>> + mhop->ba_l0_num = hr_dev->caps.scc_ctx_bt_num;
>>>> + mhop->hop_num = hr_dev->caps.scc_ctx_hop_num;
>>>> + break;
>>>> case HEM_TYPE_SRQC:
>>>> mhop->buf_chunk_size = 1 << (hr_dev->caps.srqc_buf_pg_sz
>>>> + PAGE_SHIFT);
>>>> @@ -175,7 +184,7 @@ int hns_roce_calc_hem_mhop(struct hns_roce_dev *hr_dev,
>>>> return 0;
>>>>
>>>> /*
>>>> - * QPC/MTPT/CQC/SRQC alloc hem for buffer pages.
>>>> + * QPC/MTPT/CQC/SRQC/SCC_CTX alloc hem for buffer pages.
>>>> * MTT/CQE alloc hem for bt pages.
>>>> */
>>>> bt_num = hns_roce_get_bt_num(table->type, mhop->hop_num);
>>>> @@ -486,7 +495,7 @@ static int hns_roce_table_mhop_get(struct hns_roce_dev *hr_dev,
>>>> }
>>>>
>>>> /*
>>>> - * alloc buffer space chunk for QPC/MTPT/CQC/SRQC.
>>>> + * alloc buffer space chunk for QPC/MTPT/CQC/SRQC/SCC_CTX.
>>>> * alloc bt space chunk for MTT/CQE.
>>>> */
>>>> size = table->type < HEM_TYPE_MTT ? buf_chunk_size : bt_chunk_size;
>>>> @@ -658,7 +667,7 @@ static void hns_roce_table_mhop_put(struct hns_roce_dev *hr_dev,
>>>> }
>>>>
>>>> /*
>>>> - * free buffer space chunk for QPC/MTPT/CQC/SRQC.
>>>> + * free buffer space chunk for QPC/MTPT/CQC/SRQC/SCC_CTX.
>>>> * free bt space chunk for MTT/CQE.
>>>> */
>>>> hns_roce_free_hem(hr_dev, table->hem[hem_idx]);
>>>> @@ -904,6 +913,14 @@ int hns_roce_init_hem_table(struct hns_roce_dev *hr_dev,
>>>> num_bt_l0 = hr_dev->caps.cqc_bt_num;
>>>> hop_num = hr_dev->caps.cqc_hop_num;
>>>> break;
>>>> + case HEM_TYPE_SCC_CTX:
>>>> + buf_chunk_size = 1 << (hr_dev->caps.scc_ctx_buf_pg_sz
>>>> + + PAGE_SHIFT);
>>>> + bt_chunk_size = 1 << (hr_dev->caps.scc_ctx_ba_pg_sz
>>>> + + PAGE_SHIFT);
>>>> + num_bt_l0 = hr_dev->caps.scc_ctx_bt_num;
>>>> + hop_num = hr_dev->caps.scc_ctx_hop_num;
>>>> + break;
>>>> case HEM_TYPE_SRQC:
>>>> buf_chunk_size = 1 << (hr_dev->caps.srqc_buf_pg_sz
>>>> + PAGE_SHIFT);
>>>> @@ -1081,6 +1098,9 @@ void hns_roce_cleanup_hem(struct hns_roce_dev *hr_dev)
>>>> hns_roce_cleanup_hem_table(hr_dev,
>>>> &hr_dev->srq_table.table);
>>>> hns_roce_cleanup_hem_table(hr_dev, &hr_dev->cq_table.table);
>>>> + if (hr_dev->caps.scc_ctx_entry_sz)
>>>> + hns_roce_cleanup_hem_table(hr_dev,
>>>> + &hr_dev->qp_table.scc_ctx_table);
>>>> if (hr_dev->caps.trrl_entry_sz)
>>>> hns_roce_cleanup_hem_table(hr_dev,
>>>> &hr_dev->qp_table.trrl_table);
>>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hem.h b/drivers/infiniband/hw/hns/hns_roce_hem.h
>>>> index a650278..6e4fe97 100644
>>>> --- a/drivers/infiniband/hw/hns/hns_roce_hem.h
>>>> +++ b/drivers/infiniband/hw/hns/hns_roce_hem.h
>>>> @@ -44,6 +44,7 @@ enum {
>>>> HEM_TYPE_MTPT,
>>>> HEM_TYPE_CQC,
>>>> HEM_TYPE_SRQC,
>>>> + HEM_TYPE_SCC_CTX,
>>>>
>>>> /* UNMAP HEM */
>>>> HEM_TYPE_MTT,
>>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>>>> index 835b783..77cfd9b 100644
>>>> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>>>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>>>> @@ -1078,6 +1078,9 @@ static int hns_roce_query_pf_resource(struct hns_roce_dev *hr_dev)
>>>> hr_dev->caps.sl_num = roce_get_field(req_b->qid_idx_sl_num,
>>>> PF_RES_DATA_3_PF_SL_NUM_M,
>>>> PF_RES_DATA_3_PF_SL_NUM_S);
>>>> + hr_dev->caps.scc_ctx_bt_num = roce_get_field(req_b->scc_ctx_bt_idx_num,
>>>> + PF_RES_DATA_4_PF_SCC_CTX_BT_NUM_M,
>>>> + PF_RES_DATA_4_PF_SCC_CTX_BT_NUM_S);
>>>>
>>>> return 0;
>>>> }
>>>> @@ -1178,6 +1181,7 @@ static int hns_roce_v2_set_bt(struct hns_roce_dev *hr_dev)
>>>> u8 qpc_hop_num = hr_dev->caps.qpc_hop_num;
>>>> u8 cqc_hop_num = hr_dev->caps.cqc_hop_num;
>>>> u8 mpt_hop_num = hr_dev->caps.mpt_hop_num;
>>>> + u8 scc_ctx_hop_num = hr_dev->caps.scc_ctx_hop_num;
>>>> struct hns_roce_cfg_bt_attr *req;
>>>> struct hns_roce_cmq_desc desc;
>>>>
>>>> @@ -1225,6 +1229,20 @@ static int hns_roce_v2_set_bt(struct hns_roce_dev *hr_dev)
>>>> CFG_BT_ATTR_DATA_3_VF_MPT_HOPNUM_S,
>>>> mpt_hop_num == HNS_ROCE_HOP_NUM_0 ? 0 : mpt_hop_num);
>>>>
>>>> + roce_set_field(req->vf_scc_ctx_cfg,
>>>> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_BA_PGSZ_M,
>>>> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_BA_PGSZ_S,
>>>> + hr_dev->caps.scc_ctx_ba_pg_sz + PG_SHIFT_OFFSET);
>>>> + roce_set_field(req->vf_scc_ctx_cfg,
>>>> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_BUF_PGSZ_M,
>>>> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_BUF_PGSZ_S,
>>>> + hr_dev->caps.scc_ctx_buf_pg_sz + PG_SHIFT_OFFSET);
>>>> + roce_set_field(req->vf_scc_ctx_cfg,
>>>> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_HOPNUM_M,
>>>> + CFG_BT_ATTR_DATA_4_VF_SCC_CTX_HOPNUM_S,
>>>> + scc_ctx_hop_num ==
>>>> + HNS_ROCE_HOP_NUM_0 ? 0 : scc_ctx_hop_num);
>>>> +
>>>> return hns_roce_cmq_send(hr_dev, &desc, 1);
>>>> }
>>>>
>>>> @@ -1372,9 +1390,14 @@ static int hns_roce_v2_profile(struct hns_roce_dev *hr_dev)
>>>> caps->max_srq_wrs = HNS_ROCE_V2_MAX_SRQ_WR;
>>>> caps->max_srq_sges = HNS_ROCE_V2_MAX_SRQ_SGE;
>>>>
>>>> - if (hr_dev->pci_dev->revision == 0x21)
>>>> + if (hr_dev->pci_dev->revision == 0x21) {
>>>> caps->flags |= HNS_ROCE_CAP_FLAG_ATOMIC |
>>>> HNS_ROCE_CAP_FLAG_SRQ;
>>>> + caps->scc_ctx_entry_sz = HNS_ROCE_V2_SCC_CTX_ENTRY_SZ;
>>>> + caps->scc_ctx_ba_pg_sz = 0;
>>>> + caps->scc_ctx_buf_pg_sz = 0;
>>>> + caps->scc_ctx_hop_num = HNS_ROCE_SCC_CTX_HOP_NUM;
>>>> + }
>>>>
>>>> ret = hns_roce_v2_set_bt(hr_dev);
>>>> if (ret)
>>>> @@ -2618,6 +2641,13 @@ static int hns_roce_v2_set_hem(struct hns_roce_dev *hr_dev,
>>>> case HEM_TYPE_SRQC:
>>>> op = HNS_ROCE_CMD_WRITE_SRQC_BT0;
>>>> break;
>>>> + case HEM_TYPE_SCC_CTX:
>>>> + if (step_idx) {
>>>> + /* No need to notify Hardware when step_idx is 1 or 2*/
>>>> + return 0;
>>>> + }
>>>> + op = HNS_ROCE_CMD_WRITE_SCC_CTX_BT0;
>>>> + break;
>>>> default:
>>>> dev_warn(dev, "Table %d not to be written by mailbox!\n",
>>>> table->type);
>>>> @@ -2677,6 +2707,9 @@ static int hns_roce_v2_clear_hem(struct hns_roce_dev *hr_dev,
>>>> case HEM_TYPE_CQC:
>>>> op = HNS_ROCE_CMD_DESTROY_CQC_BT0;
>>>> break;
>>>> + case HEM_TYPE_SCC_CTX:
>>>> + /* there is no need to destroy scc ctx */
>>>> + return 0;
>>>
>>> Combination of "break" and "return" constructions in the same
>>> switch<->case is a great way to shot yourself into the head.
>>>
>>> Please don't mix them.
>> Good suggestion.
>> "return" will be replaced by "goto" in function hns_roce_v2_set_hem and
>> function hns_roce_v2_clear_hem.
>> The modified function is as follows, please help me to see if this is appropriate?
>>
>> @@ -2618,6 +2641,14 @@ static int hns_roce_v2_set_hem(struct hns_roce_dev *hr_dev,
>> case HEM_TYPE_SRQC:
>> op = HNS_ROCE_CMD_WRITE_SRQC_BT0;
>> break;
>> + case HEM_TYPE_SCC_CTX:
>> + if (step_idx) {
>> + /* No need to notify Hardware when step_idx is 1 or 2*/
>> + ret = 0;
>> + goto done;
>
> I have the same feeling about "goto" as for the "return".
> Let's do this, prior to executing switch():
>
> if ( ... == HEM_TYPE_SCC_CTX && !step_idx)
> return 0;
As your suggestion, I adjusted the position, and it seems clean and readable.
The adjusted code is as follows:
@@ -2618,11 +2641,18 @@ static int hns_roce_v2_set_hem(struct hns_roce_dev *hr_dev,
case HEM_TYPE_SRQC:
op = HNS_ROCE_CMD_WRITE_SRQC_BT0;
break;
+ case HEM_TYPE_SCCC:
+ op = HNS_ROCE_CMD_WRITE_SCCC_BT0;
+ break;
default:
dev_warn(dev, "Table %d not to be written by mailbox!\n",
table->type);
return 0;
}
+
+ if (table->type == HEM_TYPE_SCCC && step_idx)
+ return 0;
+
@@ -2677,6 +2707,8 @@ static int hns_roce_v2_clear_hem(struct hns_roce_dev *hr_dev,
case HEM_TYPE_CQC:
op = HNS_ROCE_CMD_DESTROY_CQC_BT0;
break;
+ case HEM_TYPE_SCCC:
+ break;
case HEM_TYPE_SRQC:
op = HNS_ROCE_CMD_DESTROY_SRQC_BT0;
break;
@@ -2685,6 +2717,10 @@ static int hns_roce_v2_clear_hem(struct hns_roce_dev *hr_dev,
table->type);
return 0;
}
+
+ if (table->type == HEM_TYPE_SCCC)
+ return 0;
+
op += step_idx;
Thanks
>
>
> It will make switch<->case clean and very readable, without jumps in the middle.
>
>> + }
>> + op = HNS_ROCE_CMD_WRITE_SCC_CTX_BT0;
>> + break;
>> default:
>> dev_warn(dev, "Table %d not to be written by mailbox!\n",
>> table->type);
>> @@ -2652,6 +2683,8 @@ static int hns_roce_v2_set_hem(struct hns_roce_dev *hr_dev,
>> }
>>
>> hns_roce_free_cmd_mailbox(hr_dev, mailbox);
>> +
>> +done:
>> return ret;
>> }
>> @@ -2661,8 +2694,8 @@ static int hns_roce_v2_clear_hem(struct hns_roce_dev *hr_dev,
>> {
>> struct device *dev = hr_dev->dev;
>> struct hns_roce_cmd_mailbox *mailbox;
>> - int ret = 0;
>> u16 op = 0xff;
>> + int ret;
>>
>> if (!hns_roce_check_whether_mhop(hr_dev, table->type))
>> return 0;
>> @@ -2677,6 +2710,10 @@ static int hns_roce_v2_clear_hem(struct hns_roce_dev *hr_dev,
>> case HEM_TYPE_CQC:
>> op = HNS_ROCE_CMD_DESTROY_CQC_BT0;
>> break;
>> + case HEM_TYPE_SCC_CTX:
>> + /* no need to destroy scc ctx */
>> + ret = 0;
>> + goto done;
>
> The same commend as above.
>
>> case HEM_TYPE_SRQC:
>> op = HNS_ROCE_CMD_DESTROY_SRQC_BT0;
>> break;
>> @@ -2696,6 +2733,8 @@ static int hns_roce_v2_clear_hem(struct hns_roce_dev *hr_dev,
>> HNS_ROCE_CMD_TIMEOUT_MSECS);
>>
>> hns_roce_free_cmd_mailbox(hr_dev, mailbox);
>> +
>> +done:
>> return ret;
>> }
>>
>> Thanks
>>>
>>> Thanks
>>>
>>
