1. Patchew
  2. linux
  3. View series

[PATCH] drivers: fix the exception was not returned

YanLong Dai posted 1 patch 2 weeks, 2 days ago 
drivers/infiniband/hw/bnxt_re/ib_verbs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
[PATCH] drivers: fix the exception was not returned
Posted by YanLong Dai 2 weeks, 2 days ago 
From: daiyanlong <daiyanlong@kylinos.cn>

The return value rc of bnxt_qplib_destroy_qp is abnormal and no return

Signed-off-by: daiyanlong <daiyanlong@kylinos.cn>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 260dc67b8b87..d52cfb50b1b8 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -971,8 +971,10 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp, struct ib_udata *udata)
 	bnxt_qplib_flush_cqn_wq(&qp->qplib_qp);
 
 	rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &qp->qplib_qp);
-	if (rc)
+	if (rc) {
 		ibdev_err(&rdev->ibdev, "Failed to destroy HW QP");
+		return rc;
+	}
 
 	if (rdma_is_kernel_res(&qp->ib_qp.res)) {
 		flags = bnxt_re_lock_cqs(qp);
-- 
2.43.0
Re: [PATCH] drivers: fix the exception was not returned
Posted by Leon Romanovsky 2 weeks, 1 day ago 
On Tue, Sep 16, 2025 at 05:11:54PM +0800, YanLong Dai wrote:
> From: daiyanlong <daiyanlong@kylinos.cn>
> 
> The return value rc of bnxt_qplib_destroy_qp is abnormal and no return

And what is wrong with that? This is expected behavior - do not fail if
destroy fails.

Thanks

> 
> Signed-off-by: daiyanlong <daiyanlong@kylinos.cn>
> ---
>  drivers/infiniband/hw/bnxt_re/ib_verbs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index 260dc67b8b87..d52cfb50b1b8 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -971,8 +971,10 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp, struct ib_udata *udata)
>  	bnxt_qplib_flush_cqn_wq(&qp->qplib_qp);
>  
>  	rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &qp->qplib_qp);
> -	if (rc)
> +	if (rc) {
>  		ibdev_err(&rdev->ibdev, "Failed to destroy HW QP");
> +		return rc;
> +	}
>  
>  	if (rdma_is_kernel_res(&qp->ib_qp.res)) {
>  		flags = bnxt_re_lock_cqs(qp);
> -- 
> 2.43.0
>
Re: [PATCH] drivers: fix the exception was not returned
Posted by YanLong Dai 2 weeks, 1 day ago 
On Tue, Sep 16, 2025 at 04:49:15PM +0300, Leon Romanovsky wrote:
> On Tue, Sep 16, 2025 at 05:11:54PM +0800, YanLong Dai wrote:
> > From: daiyanlong <daiyanlong@kylinos.cn>
> > 
> > The return value rc of bnxt_qplib_destroy_qp is abnormal and no return
> 
> And what is wrong with that? This is expected behavior - do not fail if
> destroy fails.

Thank you for the feedback! I understand your perspective that destroy operations should ideally complete cleanup whenever possible.

However, while reviewing the related code, I noticed a consistency detail:
In the bnxt_re_destroy_gsi_sqp() function within the same file (drivers/infiniband/hw/bnxt_re/ib_verbs.c), the error handling for bnxt_qplib_destroy_qp() is different:

static void bnxt_re_destroy_gsi_sqp(struct bnxt_re_qp *qp)
{
    ...
    rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &qp->qplib_qp);
    if (rc) {
        ibdev_err(...);  // <- logs a warning here
        goto fail;       // <- returns immediately without further cleanup
    }
    bnxt_qplib_free_qp_res(&rdev->qplib_res, &qp->qplib_qp); // <- cleans up resources
	
fail:
	return rc;
}

Whereas in the function addressed by the current patch:

static int bnxt_re_destroy_qp(...)
{
    ...
    rc = bnxt_qplib_destroy_qp(&qp->qplib_qp);
	if (rc)
		ibdev_err(&rdev->ibdev, "Failed to destroy HW QP");

    // no check of rc, proceeds directly to cleanup
    bnxt_qplib_free_qp_res(&qp->qplib_qp);
}

My consideration is:
If bnxt_qplib_free_qp_res() is called after bnxt_qplib_destroy_qp() fails, could this potentially lead to 

This might be a minor robustness concern in the code. Of course, you as the maintainer have the best understanding of the code context. If this is an intentional design decision, please feel free to disregard my suggestion.

Thank you for your time!
YanLong
[PATCH] drivers: fix the potential memory leak in bnxt_re_destroy_gsi_sqp()
Posted by YanLong Dai 2 weeks, 1 day ago 
From: daiyanlong <daiyanlong@kylinos.cn>

As suggested by Kalesh Anakkur Purayil, fix the potential memory leak in bnxt_re_destroy_gsi_sqp() by continuing the teardown even if bnxt_qplib_destroy_qp() fails, we should not fail the resource destroy operations

Signed-off-by: daiyanlong <daiyanlong@kylinos.cn>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 260dc67b8b87..adee44aa0583 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -931,10 +931,9 @@ static int bnxt_re_destroy_gsi_sqp(struct bnxt_re_qp *qp)
 
 	ibdev_dbg(&rdev->ibdev, "Destroy the shadow QP\n");
 	rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &gsi_sqp->qplib_qp);
-	if (rc) {
+	if (rc)
 		ibdev_err(&rdev->ibdev, "Destroy Shadow QP failed");
-		goto fail;
-	}
+
 	bnxt_qplib_free_qp_res(&rdev->qplib_res, &gsi_sqp->qplib_qp);
 
 	/* remove from active qp list */
-- 
2.43.0
Re: [PATCH] drivers: fix the potential memory leak in bnxt_re_destroy_gsi_sqp()
Posted by Leon Romanovsky 2 weeks ago 
On Wed, Sep 17, 2025 at 07:35:39PM +0800, YanLong Dai wrote:
> From: daiyanlong <daiyanlong@kylinos.cn>
> 
> As suggested by Kalesh Anakkur Purayil, fix the potential memory leak in bnxt_re_destroy_gsi_sqp() by continuing the teardown even if bnxt_qplib_destroy_qp() fails, we should not fail the resource destroy operations
> 
> Signed-off-by: daiyanlong <daiyanlong@kylinos.cn>
> ---
>  drivers/infiniband/hw/bnxt_re/ib_verbs.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Please resend this patch as standalone and not as Reply-to to other conversion.
While you are doing that, make sure that your patch pass checkpatch.pl, break lines
in commit message, use real name in Signed-off-by and From fields, and
remove "rc" variable too, which is not used.

Thanks

> 
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index 260dc67b8b87..adee44aa0583 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -931,10 +931,9 @@ static int bnxt_re_destroy_gsi_sqp(struct bnxt_re_qp *qp)
>  
>  	ibdev_dbg(&rdev->ibdev, "Destroy the shadow QP\n");
>  	rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &gsi_sqp->qplib_qp);
> -	if (rc) {
> +	if (rc)
>  		ibdev_err(&rdev->ibdev, "Destroy Shadow QP failed");
> -		goto fail;
> -	}
> +
>  	bnxt_qplib_free_qp_res(&rdev->qplib_res, &gsi_sqp->qplib_qp);
>  
>  	/* remove from active qp list */
> -- 
> 2.43.0
> 
>
[PATCH v3] drivers: fix the potential memory leak in bnxt_re_destroy_gsi_sqp()
Posted by YanLong Dai 2 weeks, 1 day ago 
From: daiyanlong <daiyanlong@kylinos.cn>

As suggested by Kalesh Anakkur Purayil, fix the potential memory leak in
bnxt_re_destroy_gsi_sqp() by continuing the teardown even if
bnxt_qplib_destroy_qp() fails, weshould not fail the resource
destroy operations

Eliminates the 'fail:' label and associated return statement

Modify commit information exceeding 75 bytes

Signed-off-by: daiyanlong <daiyanlong@kylinos.cn>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 260dc67b8b87..15d3f5d5c0ee 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -931,10 +931,9 @@ static int bnxt_re_destroy_gsi_sqp(struct bnxt_re_qp *qp)
 
 	ibdev_dbg(&rdev->ibdev, "Destroy the shadow QP\n");
 	rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &gsi_sqp->qplib_qp);
-	if (rc) {
+	if (rc)
 		ibdev_err(&rdev->ibdev, "Destroy Shadow QP failed");
-		goto fail;
-	}
+
 	bnxt_qplib_free_qp_res(&rdev->qplib_res, &gsi_sqp->qplib_qp);
 
 	/* remove from active qp list */
@@ -951,8 +950,6 @@ static int bnxt_re_destroy_gsi_sqp(struct bnxt_re_qp *qp)
 	rdev->gsi_ctx.sqp_tbl = NULL;
 
 	return 0;
-fail:
-	return rc;
 }
 
 /* Queue Pairs */
-- 
2.43.0
[PATCH v3] drivers: fix the potential memory leak in bnxt_re_destroy_gsi_sqp()
Posted by YanLong Dai 2 weeks, 1 day ago 
From: daiyanlong <daiyanlong@kylinos.cn>

As suggested by Kalesh Anakkur Purayil, fix the potential memory leak in
bnxt_re_destroy_gsi_sqp() by continuing the teardown even if
bnxt_qplib_destroy_qp() fails, weshould not fail the resource
destroy operations

Eliminates the 'fail:' label and associated return statement

Modify commit information exceeding 75 bytes

Signed-off-by: daiyanlong <daiyanlong@kylinos.cn>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 260dc67b8b87..15d3f5d5c0ee 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -931,10 +931,9 @@ static int bnxt_re_destroy_gsi_sqp(struct bnxt_re_qp *qp)
 
 	ibdev_dbg(&rdev->ibdev, "Destroy the shadow QP\n");
 	rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &gsi_sqp->qplib_qp);
-	if (rc) {
+	if (rc)
 		ibdev_err(&rdev->ibdev, "Destroy Shadow QP failed");
-		goto fail;
-	}
+
 	bnxt_qplib_free_qp_res(&rdev->qplib_res, &gsi_sqp->qplib_qp);
 
 	/* remove from active qp list */
@@ -951,8 +950,6 @@ static int bnxt_re_destroy_gsi_sqp(struct bnxt_re_qp *qp)
 	rdev->gsi_ctx.sqp_tbl = NULL;
 
 	return 0;
-fail:
-	return rc;
 }
 
 /* Queue Pairs */
-- 
2.43.0
[PATCH v2] drivers: fix the potential memory leak in bnxt_re_destroy_gsi_sqp()
Posted by YanLong Dai 2 weeks, 1 day ago 
From: daiyanlong <daiyanlong@kylinos.cn>

As suggested by Kalesh Anakkur Purayil, fix the potential memory leak in bnxt_re_destroy_gsi_sqp() by continuing the teardown even if bnxt_qplib_destroy_qp() fails, we should not fail the resource destroy operations

Eliminates the 'fail:' label and associated return statement

Signed-off-by: daiyanlong <daiyanlong@kylinos.cn>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 260dc67b8b87..15d3f5d5c0ee 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -931,10 +931,9 @@ static int bnxt_re_destroy_gsi_sqp(struct bnxt_re_qp *qp)
 
 	ibdev_dbg(&rdev->ibdev, "Destroy the shadow QP\n");
 	rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &gsi_sqp->qplib_qp);
-	if (rc) {
+	if (rc)
 		ibdev_err(&rdev->ibdev, "Destroy Shadow QP failed");
-		goto fail;
-	}
+
 	bnxt_qplib_free_qp_res(&rdev->qplib_res, &gsi_sqp->qplib_qp);
 
 	/* remove from active qp list */
@@ -951,8 +950,6 @@ static int bnxt_re_destroy_gsi_sqp(struct bnxt_re_qp *qp)
 	rdev->gsi_ctx.sqp_tbl = NULL;
 
 	return 0;
-fail:
-	return rc;
 }
 
 /* Queue Pairs */
-- 
2.43.0

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.