diff mbox series

[v2,for-next,7/7] RDMA/hns: Optimize qp doorbell allocation flow

Message ID 1581325720-12975-8-git-send-email-liweihang@huawei.com (mailing list archive)
State Superseded
Delegated to: Jason Gunthorpe
Headers show
Series RDMA/hns: Refactor qp related code | expand

Commit Message

Weihang Li Feb. 10, 2020, 9:08 a.m. UTC 
From: Xi Wang <wangxi11@huawei.com>

Encapsulate the kernel qp doorbell allocation related code into 2
functions: alloc_qp_db() and free_qp_db().

Signed-off-by: Xi Wang <wangxi11@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_qp.c | 214 +++++++++++++++++---------------
 1 file changed, 113 insertions(+), 101 deletions(-)

Comments

Jason Gunthorpe Feb. 19, 2020, 12:52 a.m. UTC | #1 
On Mon, Feb 10, 2020 at 05:08:40PM +0800, Weihang Li wrote:
> From: Xi Wang <wangxi11@huawei.com>
> 
> Encapsulate the kernel qp doorbell allocation related code into 2
> functions: alloc_qp_db() and free_qp_db().
> 
> Signed-off-by: Xi Wang <wangxi11@huawei.com>
> Signed-off-by: Weihang Li <liweihang@huawei.com>
>  drivers/infiniband/hw/hns/hns_roce_qp.c | 214 +++++++++++++++++---------------
>  1 file changed, 113 insertions(+), 101 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
> index ad34187..46785f1 100644
> +++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
> @@ -844,6 +844,96 @@ static void free_qp_buf(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp)
>  		free_rq_inline_buf(hr_qp);
>  }
>  
> +#define user_qp_has_sdb(hr_dev, init_attr, udata, resp, ucmd) \
> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_SQ_RECORD_DB) && \
> +		udata->outlen >= sizeof(*resp) && \
> +		hns_roce_qp_has_sq(init_attr) && udata->inlen >= sizeof(*ucmd))
> +
> +#define user_qp_has_rdb(hr_dev, init_attr, udata, resp) \
> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
> +		udata->outlen >= sizeof(*resp) && \
> +		hns_roce_qp_has_rq(init_attr))
> +
> +#define kernel_qp_has_rdb(hr_dev, init_attr) \
> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
> +		hns_roce_qp_has_rq(init_attr))

static inline functions not defines please

Also, these tests against inline and outlen look very strange. What
are they doing?

Jason
Weihang Li Feb. 19, 2020, 8:14 a.m. UTC | #2 
On 2020/2/19 8:52, Jason Gunthorpe wrote:
> On Mon, Feb 10, 2020 at 05:08:40PM +0800, Weihang Li wrote:
>> From: Xi Wang <wangxi11@huawei.com>
>>
>> Encapsulate the kernel qp doorbell allocation related code into 2
>> functions: alloc_qp_db() and free_qp_db().
>>
>> Signed-off-by: Xi Wang <wangxi11@huawei.com>
>> Signed-off-by: Weihang Li <liweihang@huawei.com>
>>  drivers/infiniband/hw/hns/hns_roce_qp.c | 214 +++++++++++++++++---------------
>>  1 file changed, 113 insertions(+), 101 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
>> index ad34187..46785f1 100644
>> +++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
>> @@ -844,6 +844,96 @@ static void free_qp_buf(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp)
>>  		free_rq_inline_buf(hr_qp);
>>  }
>>  
>> +#define user_qp_has_sdb(hr_dev, init_attr, udata, resp, ucmd) \
>> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_SQ_RECORD_DB) && \
>> +		udata->outlen >= sizeof(*resp) && \
>> +		hns_roce_qp_has_sq(init_attr) && udata->inlen >= sizeof(*ucmd))
>> +
>> +#define user_qp_has_rdb(hr_dev, init_attr, udata, resp) \
>> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
>> +		udata->outlen >= sizeof(*resp) && \
>> +		hns_roce_qp_has_rq(init_attr))
>> +
>> +#define kernel_qp_has_rdb(hr_dev, init_attr) \
>> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
>> +		hns_roce_qp_has_rq(init_attr))
> 
> static inline functions not defines please
> 

OK, I will change them into inline functions.

> Also, these tests against inline and outlen look very strange. What
> are they doing?
> 
> Jason
>

These judgement about inlen and outlen is for compatibility reasons,
previous discussions can be found at:

https://patchwork.kernel.org/patch/10172233/

Thanks,
Weihang

>
Jason Gunthorpe Feb. 19, 2020, 1:19 p.m. UTC | #3 
On Wed, Feb 19, 2020 at 04:14:36PM +0800, Weihang Li wrote:
> 
> 
> On 2020/2/19 8:52, Jason Gunthorpe wrote:
> > On Mon, Feb 10, 2020 at 05:08:40PM +0800, Weihang Li wrote:
> >> From: Xi Wang <wangxi11@huawei.com>
> >>
> >> Encapsulate the kernel qp doorbell allocation related code into 2
> >> functions: alloc_qp_db() and free_qp_db().
> >>
> >> Signed-off-by: Xi Wang <wangxi11@huawei.com>
> >> Signed-off-by: Weihang Li <liweihang@huawei.com>
> >>  drivers/infiniband/hw/hns/hns_roce_qp.c | 214 +++++++++++++++++---------------
> >>  1 file changed, 113 insertions(+), 101 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
> >> index ad34187..46785f1 100644
> >> +++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
> >> @@ -844,6 +844,96 @@ static void free_qp_buf(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp)
> >>  		free_rq_inline_buf(hr_qp);
> >>  }
> >>  
> >> +#define user_qp_has_sdb(hr_dev, init_attr, udata, resp, ucmd) \
> >> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_SQ_RECORD_DB) && \
> >> +		udata->outlen >= sizeof(*resp) && \
> >> +		hns_roce_qp_has_sq(init_attr) && udata->inlen >= sizeof(*ucmd))
> >> +
> >> +#define user_qp_has_rdb(hr_dev, init_attr, udata, resp) \
> >> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
> >> +		udata->outlen >= sizeof(*resp) && \
> >> +		hns_roce_qp_has_rq(init_attr))
> >> +
> >> +#define kernel_qp_has_rdb(hr_dev, init_attr) \
> >> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
> >> +		hns_roce_qp_has_rq(init_attr))
> > 
> > static inline functions not defines please
> > 
> 
> OK, I will change them into inline functions.
> 
> > Also, these tests against inline and outlen look very strange. What
> > are they doing?
> > 
> > Jason
> >
> 
> These judgement about inlen and outlen is for compatibility reasons,
> previous discussions can be found at:
> 
> https://patchwork.kernel.org/patch/10172233/

Something is wrong, it should be testing the legnth using a
field_offset_off kind of scheme, not sizeof(*resp)

Jason
Weihang Li Feb. 22, 2020, 7:22 a.m. UTC | #4 
On 2020/2/19 21:19, Jason Gunthorpe wrote:
> On Wed, Feb 19, 2020 at 04:14:36PM +0800, Weihang Li wrote:
>>
>>
>> On 2020/2/19 8:52, Jason Gunthorpe wrote:
>>> On Mon, Feb 10, 2020 at 05:08:40PM +0800, Weihang Li wrote:
>>>> From: Xi Wang <wangxi11@huawei.com>
>>>>
>>>> Encapsulate the kernel qp doorbell allocation related code into 2
>>>> functions: alloc_qp_db() and free_qp_db().
>>>>
>>>> Signed-off-by: Xi Wang <wangxi11@huawei.com>
>>>> Signed-off-by: Weihang Li <liweihang@huawei.com>
>>>>  drivers/infiniband/hw/hns/hns_roce_qp.c | 214 +++++++++++++++++---------------
>>>>  1 file changed, 113 insertions(+), 101 deletions(-)
>>>>
>>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
>>>> index ad34187..46785f1 100644
>>>> +++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
>>>> @@ -844,6 +844,96 @@ static void free_qp_buf(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp)
>>>>  		free_rq_inline_buf(hr_qp);
>>>>  }
>>>>  
>>>> +#define user_qp_has_sdb(hr_dev, init_attr, udata, resp, ucmd) \
>>>> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_SQ_RECORD_DB) && \
>>>> +		udata->outlen >= sizeof(*resp) && \
>>>> +		hns_roce_qp_has_sq(init_attr) && udata->inlen >= sizeof(*ucmd))
>>>> +
>>>> +#define user_qp_has_rdb(hr_dev, init_attr, udata, resp) \
>>>> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
>>>> +		udata->outlen >= sizeof(*resp) && \
>>>> +		hns_roce_qp_has_rq(init_attr))
>>>> +
>>>> +#define kernel_qp_has_rdb(hr_dev, init_attr) \
>>>> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
>>>> +		hns_roce_qp_has_rq(init_attr))
>>>
>>> static inline functions not defines please
>>>
>>
>> OK, I will change them into inline functions.
>>
>>> Also, these tests against inline and outlen look very strange. What
>>> are they doing?
>>>
>>> Jason
>>>
>>
>> These judgement about inlen and outlen is for compatibility reasons,
>> previous discussions can be found at:
>>
>> https://patchwork.kernel.org/patch/10172233/
> 
> Something is wrong, it should be testing the legnth using a
> field_offset_off kind of scheme, not sizeof(*resp)
> 
> Jason
> 
Hi Jason,

Do you means

	udata->outlen >= sizeof(*resp)

should be changed into:

	udata->out_len >= offsetof(typeof(*resp), cap_flags)

If yes, I will fix other similar codes with this issue in hns drivers.

Thanks
Weihang
Jason Gunthorpe Feb. 22, 2020, 11:38 p.m. UTC | #5 
On Sat, Feb 22, 2020 at 03:22:18PM +0800, Weihang Li wrote:
> 
> 
> On 2020/2/19 21:19, Jason Gunthorpe wrote:
> > On Wed, Feb 19, 2020 at 04:14:36PM +0800, Weihang Li wrote:
> >>
> >>
> >> On 2020/2/19 8:52, Jason Gunthorpe wrote:
> >>> On Mon, Feb 10, 2020 at 05:08:40PM +0800, Weihang Li wrote:
> >>>> From: Xi Wang <wangxi11@huawei.com>
> >>>>
> >>>> Encapsulate the kernel qp doorbell allocation related code into 2
> >>>> functions: alloc_qp_db() and free_qp_db().
> >>>>
> >>>> Signed-off-by: Xi Wang <wangxi11@huawei.com>
> >>>> Signed-off-by: Weihang Li <liweihang@huawei.com>
> >>>>  drivers/infiniband/hw/hns/hns_roce_qp.c | 214 +++++++++++++++++---------------
> >>>>  1 file changed, 113 insertions(+), 101 deletions(-)
> >>>>
> >>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
> >>>> index ad34187..46785f1 100644
> >>>> +++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
> >>>> @@ -844,6 +844,96 @@ static void free_qp_buf(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp)
> >>>>  		free_rq_inline_buf(hr_qp);
> >>>>  }
> >>>>  
> >>>> +#define user_qp_has_sdb(hr_dev, init_attr, udata, resp, ucmd) \
> >>>> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_SQ_RECORD_DB) && \
> >>>> +		udata->outlen >= sizeof(*resp) && \
> >>>> +		hns_roce_qp_has_sq(init_attr) && udata->inlen >= sizeof(*ucmd))
> >>>> +
> >>>> +#define user_qp_has_rdb(hr_dev, init_attr, udata, resp) \
> >>>> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
> >>>> +		udata->outlen >= sizeof(*resp) && \
> >>>> +		hns_roce_qp_has_rq(init_attr))
> >>>> +
> >>>> +#define kernel_qp_has_rdb(hr_dev, init_attr) \
> >>>> +		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
> >>>> +		hns_roce_qp_has_rq(init_attr))
> >>>
> >>> static inline functions not defines please
> >>>
> >>
> >> OK, I will change them into inline functions.
> >>
> >>> Also, these tests against inline and outlen look very strange. What
> >>> are they doing?
> >>>
> >>> Jason
> >>>
> >>
> >> These judgement about inlen and outlen is for compatibility reasons,
> >> previous discussions can be found at:
> >>
> >> https://patchwork.kernel.org/patch/10172233/
> > 
> > Something is wrong, it should be testing the legnth using a
> > field_offset_off kind of scheme, not sizeof(*resp)
> > 
> > Jason
> > 
> Hi Jason,
> 
> Do you means
> 
> 	udata->outlen >= sizeof(*resp)
> 
> should be changed into:
> 
> 	udata->out_len >= offsetof(typeof(*resp), cap_flags)
> 
> If yes, I will fix other similar codes with this issue in hns drivers.

Probably offsetofend though, right?

But yes, that is how the general 'feature test for old userspace with
old kernel ABI' should look

Jason
Weihang Li Feb. 24, 2020, 1:29 a.m. UTC | #6 
On 2020/2/23 7:38, Jason Gunthorpe wrote:
>> Hi Jason,
>>
>> Do you means
>>
>> 	udata->outlen >= sizeof(*resp)
>>
>> should be changed into:
>>
>> 	udata->out_len >= offsetof(typeof(*resp), cap_flags)
>>
>> If yes, I will fix other similar codes with this issue in hns drivers.
> Probably offsetofend though, right?
> 
> But yes, that is how the general 'feature test for old userspace with
> old kernel ABI' should look
> 
> Jason
> 

Yes, you are right, I made a mistake :)
Thanks for your comments.

Weihang
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
index ad34187..46785f1 100644
--- a/drivers/infiniband/hw/hns/hns_roce_qp.c
+++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
@@ -844,6 +844,96 @@  static void free_qp_buf(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp)
 		free_rq_inline_buf(hr_qp);
 }
 
+#define user_qp_has_sdb(hr_dev, init_attr, udata, resp, ucmd) \
+		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_SQ_RECORD_DB) && \
+		udata->outlen >= sizeof(*resp) && \
+		hns_roce_qp_has_sq(init_attr) && udata->inlen >= sizeof(*ucmd))
+
+#define user_qp_has_rdb(hr_dev, init_attr, udata, resp) \
+		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
+		udata->outlen >= sizeof(*resp) && \
+		hns_roce_qp_has_rq(init_attr))
+
+#define kernel_qp_has_rdb(hr_dev, init_attr) \
+		((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) && \
+		hns_roce_qp_has_rq(init_attr))
+
+static int alloc_qp_db(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp,
+		       struct ib_qp_init_attr *init_attr,
+		       struct ib_udata *udata,
+		       struct hns_roce_ib_create_qp *ucmd,
+		       struct hns_roce_ib_create_qp_resp *resp)
+{
+	struct hns_roce_ucontext *uctx = rdma_udata_to_drv_context(
+		udata, struct hns_roce_ucontext, ibucontext);
+	struct ib_device *ibdev = &hr_dev->ib_dev;
+	int ret;
+
+	if (udata) {
+		if (user_qp_has_sdb(hr_dev, init_attr, udata, resp, ucmd)) {
+			ret = hns_roce_db_map_user(uctx, udata, ucmd->sdb_addr,
+						   &hr_qp->sdb);
+			if (ret) {
+				ibdev_err(ibdev, "Failed to map user SQ doorbell\n");
+				goto err_out;
+			}
+			hr_qp->sdb_en = 1;
+			resp->cap_flags |= HNS_ROCE_SUPPORT_SQ_RECORD_DB;
+		}
+
+		if (user_qp_has_rdb(hr_dev, init_attr, udata, resp)) {
+			ret = hns_roce_db_map_user(uctx, udata, ucmd->db_addr,
+						   &hr_qp->rdb);
+			if (ret) {
+				ibdev_err(ibdev, "Failed to map user RQ doorbell\n");
+				goto err_sdb;
+			}
+			hr_qp->rdb_en = 1;
+			resp->cap_flags |= HNS_ROCE_SUPPORT_RQ_RECORD_DB;
+		}
+	} else {
+		/* QP doorbell register address */
+		hr_qp->sq.db_reg_l = hr_dev->reg_base + hr_dev->sdb_offset +
+				     DB_REG_OFFSET * hr_dev->priv_uar.index;
+		hr_qp->rq.db_reg_l = hr_dev->reg_base + hr_dev->odb_offset +
+				     DB_REG_OFFSET * hr_dev->priv_uar.index;
+
+		if (kernel_qp_has_rdb(hr_dev, init_attr)) {
+			ret = hns_roce_alloc_db(hr_dev, &hr_qp->rdb, 0);
+			if (ret) {
+				ibdev_err(ibdev, "Failed to alloc kernel RQ doorbell\n");
+				goto err_out;
+			}
+			*hr_qp->rdb.db_record = 0;
+			hr_qp->rdb_en = 1;
+		}
+	}
+
+	return 0;
+err_sdb:
+	if (udata && hr_qp->sdb_en)
+		hns_roce_db_unmap_user(uctx, &hr_qp->sdb);
+err_out:
+	return ret;
+}
+
+static void free_qp_db(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp,
+		       struct ib_udata *udata)
+{
+	struct hns_roce_ucontext *uctx = rdma_udata_to_drv_context(
+		udata, struct hns_roce_ucontext, ibucontext);
+
+	if (udata) {
+		if  (hr_qp->rdb_en)
+			hns_roce_db_unmap_user(uctx, &hr_qp->rdb);
+		if  (hr_qp->sdb_en)
+			hns_roce_db_unmap_user(uctx, &hr_qp->sdb);
+	} else {
+		if  (hr_qp->rdb_en)
+			hns_roce_free_db(hr_dev, &hr_qp->rdb);
+	}
+}
+
 static int alloc_kernel_wrid(struct hns_roce_dev *hr_dev,
 			     struct hns_roce_qp *hr_qp)
 {
@@ -940,11 +1030,9 @@  static int hns_roce_create_qp_common(struct hns_roce_dev *hr_dev,
 				     struct ib_udata *udata,
 				     struct hns_roce_qp *hr_qp)
 {
-	struct device *dev = hr_dev->dev;
-	struct hns_roce_ib_create_qp ucmd;
 	struct hns_roce_ib_create_qp_resp resp = {};
-	struct hns_roce_ucontext *uctx = rdma_udata_to_drv_context(
-		udata, struct hns_roce_ucontext, ibucontext);
+	struct ib_device *ibdev = &hr_dev->ib_dev;
+	struct hns_roce_ib_create_qp ucmd;
 	int ret;
 
 	mutex_init(&hr_qp->mutex);
@@ -955,95 +1043,55 @@  static int hns_roce_create_qp_common(struct hns_roce_dev *hr_dev,
 
 	ret = set_qp_param(hr_dev, hr_qp, init_attr, udata, &ucmd);
 	if (ret) {
-		ibdev_err(&hr_dev->ib_dev, "Failed to set QP param\n");
+		ibdev_err(ibdev, "Failed to set QP param\n");
 		return ret;
 	}
 
-	if (udata) {
-		if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_SQ_RECORD_DB) &&
-		    (udata->inlen >= sizeof(ucmd)) &&
-		    (udata->outlen >= sizeof(resp)) &&
-		    hns_roce_qp_has_sq(init_attr)) {
-			ret = hns_roce_db_map_user(uctx, udata, ucmd.sdb_addr,
-						   &hr_qp->sdb);
-			if (ret) {
-				dev_err(dev, "sq record doorbell map failed!\n");
-				goto err_out;
-			}
-
-			/* indicate kernel supports sq record db */
-			resp.cap_flags |= HNS_ROCE_SUPPORT_SQ_RECORD_DB;
-			hr_qp->sdb_en = 1;
-		}
-
-		if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) &&
-		    (udata->outlen >= sizeof(resp)) &&
-		    hns_roce_qp_has_rq(init_attr)) {
-			ret = hns_roce_db_map_user(uctx, udata, ucmd.db_addr,
-						   &hr_qp->rdb);
-			if (ret) {
-				dev_err(dev, "rq record doorbell map failed!\n");
-				goto err_sq_dbmap;
-			}
-
-			/* indicate kernel supports rq record db */
-			resp.cap_flags |= HNS_ROCE_SUPPORT_RQ_RECORD_DB;
-			hr_qp->rdb_en = 1;
-		}
-	} else {
-		/* QP doorbell register address */
-		hr_qp->sq.db_reg_l = hr_dev->reg_base + hr_dev->sdb_offset +
-				     DB_REG_OFFSET * hr_dev->priv_uar.index;
-		hr_qp->rq.db_reg_l = hr_dev->reg_base + hr_dev->odb_offset +
-				     DB_REG_OFFSET * hr_dev->priv_uar.index;
-
-		if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) &&
-		    hns_roce_qp_has_rq(init_attr)) {
-			ret = hns_roce_alloc_db(hr_dev, &hr_qp->rdb, 0);
-			if (ret) {
-				dev_err(dev, "rq record doorbell alloc failed!\n");
-				goto err_out;
-			}
-			*hr_qp->rdb.db_record = 0;
-			hr_qp->rdb_en = 1;
-		}
-
+	if (!udata) {
 		ret = alloc_kernel_wrid(hr_dev, hr_qp);
 		if (ret) {
-			ibdev_err(&hr_dev->ib_dev, "Failed to alloc wrid\n");
-			goto err_db;
+			ibdev_err(ibdev, "Failed to alloc wrid\n");
+			return ret;
 		}
 	}
 
+	ret = alloc_qp_db(hr_dev, hr_qp, init_attr, udata, &ucmd, &resp);
+	if (ret) {
+		ibdev_err(ibdev, "Failed to alloc QP doorbell\n");
+		goto err_wrid;
+	}
+
 	ret = alloc_qp_buf(hr_dev, hr_qp, init_attr, udata, ucmd.buf_addr);
 	if (ret) {
-		ibdev_err(&hr_dev->ib_dev, "Failed to alloc QP buffer\n");
+		ibdev_err(ibdev, "Failed to alloc QP buffer\n");
 		goto err_db;
 	}
 
 	ret = alloc_qpn(hr_dev, hr_qp);
 	if (ret) {
-		ibdev_err(&hr_dev->ib_dev, "Failed to alloc QPN\n");
+		ibdev_err(ibdev, "Failed to alloc QPN\n");
 		goto err_buf;
 	}
 
 	ret = alloc_qpc(hr_dev, hr_qp);
 	if (ret) {
-		ibdev_err(&hr_dev->ib_dev, "Failed to alloc QP context\n");
+		ibdev_err(ibdev, "Failed to alloc QP context\n");
 		goto err_qpn;
 	}
 
 	ret = hns_roce_qp_store(hr_dev, hr_qp, init_attr);
 	if (ret) {
-		ibdev_err(&hr_dev->ib_dev, "Failed to store QP\n");
+		ibdev_err(ibdev, "Failed to store QP\n");
 		goto err_qpc;
 	}
 
 	if (udata) {
 		ret = ib_copy_to_udata(udata, &resp,
 				       min(udata->outlen, sizeof(resp)));
-		if (ret)
+		if (ret) {
+			ibdev_err(ibdev, "copy qp resp failed!\n");
 			goto err_store;
+		}
 	}
 
 	if (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_QP_FLOW_CTRL) {
@@ -1067,30 +1115,10 @@  static int hns_roce_create_qp_common(struct hns_roce_dev *hr_dev,
 	free_qpn(hr_dev, hr_qp);
 err_buf:
 	free_qp_buf(hr_dev, hr_qp);
-
-	free_kernel_wrid(hr_dev, hr_qp);
-
-	if (udata) {
-		if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB) &&
-		    (udata->outlen >= sizeof(resp)) &&
-		    hns_roce_qp_has_rq(init_attr))
-			hns_roce_db_unmap_user(uctx, &hr_qp->rdb);
-	}
-
-err_sq_dbmap:
-	if (udata)
-		if ((hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_SQ_RECORD_DB) &&
-		    (udata->inlen >= sizeof(ucmd)) &&
-		    (udata->outlen >= sizeof(resp)) &&
-		    hns_roce_qp_has_sq(init_attr))
-			hns_roce_db_unmap_user(uctx, &hr_qp->sdb);
-
 err_db:
-	if (!udata && hns_roce_qp_has_rq(init_attr) &&
-	    (hr_dev->caps.flags & HNS_ROCE_CAP_FLAG_RECORD_DB))
-		hns_roce_free_db(hr_dev, &hr_qp->rdb);
-
-err_out:
+	free_qp_db(hr_dev, hr_qp, udata);
+err_wrid:
+	free_kernel_wrid(hr_dev, hr_qp);
 	return ret;
 }
 
@@ -1105,23 +1133,7 @@  void hns_roce_qp_destroy(struct hns_roce_dev *hr_dev, struct hns_roce_qp *hr_qp,
 	free_qpn(hr_dev, hr_qp);
 	free_qp_buf(hr_dev, hr_qp);
 	free_kernel_wrid(hr_dev, hr_qp);
-
-	if (udata) {
-		struct hns_roce_ucontext *context =
-			rdma_udata_to_drv_context(
-				udata,
-				struct hns_roce_ucontext,
-				ibucontext);
-
-		if (hr_qp->sq.wqe_cnt && (hr_qp->sdb_en == 1))
-			hns_roce_db_unmap_user(context, &hr_qp->sdb);
-
-		if (hr_qp->rq.wqe_cnt && (hr_qp->rdb_en == 1))
-			hns_roce_db_unmap_user(context, &hr_qp->rdb);
-	} else {
-		if (hr_qp->rq.wqe_cnt)
-			hns_roce_free_db(hr_dev, &hr_qp->rdb);
-	}
+	free_qp_db(hr_dev, hr_qp, udata);
 
 	kfree(hr_qp);
 }