On 2020/1/23 22:31, Leon Romanovsky wrote:
> On Mon, Jan 20, 2020 at 04:19:34PM +0800, Weihang Li wrote:
>> From: Xi Wang <wangxi11@xxxxxxxxxx>
>>
>> Encapsulate qp buffer allocation related code into 3 functions:
>> alloc_qp_buf(), map_qp_buf() and free_qp_buf().
>>
>> Signed-off-by: Xi Wang <wangxi11@xxxxxxxxxx>
>> Signed-off-by: Weihang Li <liweihang@xxxxxxxxxx>
>> ---
>> drivers/infiniband/hw/hns/hns_roce_device.h | 1 -
>> drivers/infiniband/hw/hns/hns_roce_qp.c | 268 +++++++++++++++-------------
>> 2 files changed, 147 insertions(+), 122 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
>> index 1f361e6..9ddeb2b 100644
>> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
>> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
>> @@ -660,7 +660,6 @@ struct hns_roce_qp {
>> /* this define must less than HNS_ROCE_MAX_BT_REGION */
>> #define HNS_ROCE_WQE_REGION_MAX 3
>> struct hns_roce_buf_region regions[HNS_ROCE_WQE_REGION_MAX];
>> - int region_cnt;
>> int wqe_bt_pg_shift;
>>
>> u32 buff_size;
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
>> index 3bd5809..5184cb4 100644
>> --- a/drivers/infiniband/hw/hns/hns_roce_qp.c
>> +++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
>> @@ -716,23 +716,150 @@ static void free_rq_inline_buf(struct hns_roce_qp *hr_qp)
>> kfree(hr_qp->rq_inl_buf.wqe_list);
>> }
>>
>> +static int map_qp_buf(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp,
>> + u32 page_shift, bool is_user)
>> +{
>> + dma_addr_t *buf_list[ARRAY_SIZE(hr_qp->regions)] = { NULL };
>> + struct ib_device *ibdev = &hr_dev->ib_dev;
>> + struct hns_roce_buf_region *r;
>> + int region_count;
>> + int buf_count;
>> + int ret;
>> + int i;
>> +
>> + region_count = split_wqe_buf_region(hr_dev, hr_qp, hr_qp->regions,
>> + ARRAY_SIZE(hr_qp->regions), page_shift);
>> +
>> + /* alloc a tmp list for storing wqe buf address */
>> + ret = hns_roce_alloc_buf_list(hr_qp->regions, buf_list, region_count);
>> + if (ret) {
>> + ibdev_err(ibdev, "alloc buf_list error for create qp\n");
>> + return ret;
>> + }
>> +
>> + for (i = 0; i < region_count; i++) {
>> + r = &hr_qp->regions[i];
>> + if (is_user)
>> + buf_count = hns_roce_get_umem_bufs(hr_dev, buf_list[i],
>> + r->count, r->offset, hr_qp->umem,
>> + page_shift);
>> + else
>> + buf_count = hns_roce_get_kmem_bufs(hr_dev, buf_list[i],
>> + r->count, r->offset, &hr_qp->hr_buf);
>> +
>> + if (buf_count != r->count) {
>> + ibdev_err(ibdev, "get %s qp buf err,expect %d,ret %d.\n",
>> + is_user ? "user" : "kernel",
>> + r->count, buf_count);
>> + ret = -ENOBUFS;
>> + goto done;
>> + }
>> + }
>> +
>> + hr_qp->wqe_bt_pg_shift = calc_wqe_bt_page_shift(hr_dev, hr_qp->regions,
>> + region_count);
>> + hns_roce_mtr_init(&hr_qp->mtr, PAGE_SHIFT + hr_qp->wqe_bt_pg_shift,
>> + page_shift);
>> + ret = hns_roce_mtr_attach(hr_dev, &hr_qp->mtr, buf_list, hr_qp->regions,
>> + region_count);
>> + if (ret)
>> + ibdev_err(ibdev, "mtr attatch error for create qp\n");
>> +
>> + goto done;
>> +
>> + hns_roce_mtr_cleanup(hr_dev, &hr_qp->mtr);
>> +done:
>> + hns_roce_free_buf_list(buf_list, region_count);
>> +
>> + return ret;
>> +}
>> +
>> +static int alloc_qp_buf(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp,
>> + struct ib_qp_init_attr *init_attr,
>> + struct ib_udata *udata, unsigned long addr)
>> +{
>> + u32 page_shift = PAGE_SHIFT + hr_dev->caps.mtt_buf_pg_sz;
>> + struct ib_device *ibdev = &hr_dev->ib_dev;
>> + bool is_rq_buf_inline;
>> + int ret;
>> +
>> + is_rq_buf_inline = (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RQ_INLINE) &&
>> + hns_roce_qp_has_rq(init_attr);
>> + if (is_rq_buf_inline) {
>> + ret = alloc_rq_inline_buf(hr_qp, init_attr);
>> + if (ret) {
>> + ibdev_err(ibdev, "alloc recv inline buffer error\n");
>> + return ret;
>> + }
>> + }
>> +
>> + if (udata) {
>> + hr_qp->umem = ib_umem_get(udata, addr, hr_qp->buff_size, 0);
>> + if (IS_ERR(hr_qp->umem)) {
>> + ibdev_err(ibdev, "get umem error for qp buf\n");
>> + ret = PTR_ERR(hr_qp->umem);
>> + goto err_inline;
>> + }
>> + } else {
>> + ret = hns_roce_buf_alloc(hr_dev, hr_qp->buff_size,
>> + (1 << page_shift) * 2,
>> + &hr_qp->hr_buf, page_shift);
>> + if (ret) {
>> + ibdev_err(ibdev, "alloc roce buf error\n");
>> + goto err_inline;
>> + }
>> + }
>> +
>> + ret = map_qp_buf(hr_dev, hr_qp, page_shift, udata);
>
>
> I don't remember what was the resolution if it is ok to rely on "udata"
> as an indicator of user/kernel flow.
Hi Leon,
Sorry for that I don't know much about previous discussions, will check for
that. Thanks for your reminder.
>
>> + if (ret) {
>> + ibdev_err(ibdev, "map roce buf error\n");
>
> You put ibdev_err() on almost every line in map_qp_buf(), please leave
> only one place.
>
> Thanks
>
OK, thank you.
Weihang
> .
>
