RDMA/hns: Fix an error code in hns_roce_create_srq()

PATCH Applied Dan Carpenter 4 participants 9 messages Dec 17, 2018 – Jun 25, 2019

Subsystems (3)

Kernel janitors InfiniBand HiSilicon RoCE driver

lore

Share & tools

lore

Dan Carpenter

author

2018-12-17 07:08 UTC #0 lore raw

PATCH 1/1 RDMA/hns: Fix an error code in hns_roce_create_srq()

The function accidentally returns success on this error path.

Fixes c7bcb13442e1 ("RDMA/hns: Add SRQ support for hip08 kernel mode")

Signed-off-by Dan Carpenter <[email protected]>

---
 drivers/infiniband/hw/hns/hns_roce_srq.c | 1 +
 1 file changed, 1 insertion(+)

drivers/infiniband/hw/hns/hns_roce_srq.c +1 -0

index 463df60094e8..6377e734e28e 100644
@@ -286,6 +286,7 @@ struct ib_srq *hns_roce_create_srq(struct ib_pd *pd,
 		if (IS_ERR(srq->idx_que.umem)) {
 			dev_err(hr_dev->dev,
 				"ib_umem_get error for index queue\n");
+			ret = PTR_ERR(srq->idx_que.umem);
 			goto err_srq_mtt;
 		}

Signature · 1 line

2.17.1

Dan Carpenter

author

2019-06-08 09:27 UTC #2 lore raw

PATCH 1/1 RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()

In reply to Dan Carpenter

This function is supposed to return negative kernel error codes but here it returns CMD_RST_PRC_EBUSY (2). The error code eventually gets passed to IS_ERR() and since it's not an error pointer it leads to an Oops in hns_roce_v1_rsv_lp_qp()

Signed-off-by Dan Carpenter <[email protected]>

---
Static analysis. Not tested.

 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/infiniband/hw/hns/hns_roce_hw_v2.c +2 -2

index ac017c24b200..018ff302ab9e 100644
@@ -1098,7 +1098,7 @@ static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 	if (ret = CMD_RST_PRC_SUCCESS)
 		return 0;
 	if (ret = CMD_RST_PRC_EBUSY)
-		return ret;
+		return -EBUSY;
 
 	ret = __hns_roce_cmq_send(hr_dev, desc, num);
 	if (ret) {

@@ -1106,7 +1106,7 @@ static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 		if (retval = CMD_RST_PRC_SUCCESS)
 			return 0;
 		else if (retval = CMD_RST_PRC_EBUSY)
-			return retval;
+			return -EBUSY;
 	}
 
 	return ret;

Signature · 1 line

2.20.1

Leon Romanovsky

2019-06-12 17:23 UTC #3 lore raw

Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()

In reply to Dan Carpenter

Quoted 17 lines · Dan Carpenter

This function is supposed to return negative kernel error codes but here it returns CMD_RST_PRC_EBUSY (2). The error code eventually gets passed to IS_ERR() and since it's not an error pointer it leads to an Oops in hns_roce_v1_rsv_lp_qp()
Signed-off-by Dan Carpenter <[email protected]>
---
Static analysis. Not tested.
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/infiniband/hw/hns/hns_roce_hw_v2.c +0 -0
index ac017c24b200..018ff302ab9e 100644

drivers/infiniband/hw/hns/hns_roce_hw_v2.c static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev, · line 1100

Quoted 3 lines

drivers/infiniband/hw/hns/hns_roce_hw_v2.c +0 -0

@@ -1098,7 +1098,7 @@ static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 	if (ret = CMD_RST_PRC_SUCCESS)
 		return 0;
 	if (ret = CMD_RST_PRC_EBUSY)

The better fix will be to remove CMD_RST_PRC_* definitions in favor of normal errno.

Thanks

Dan Carpenter

author

2019-06-13 06:05 UTC #4 lore raw

Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()

In reply to Dan Carpenter

Quoted 13 lines · Leon Romanovsky

Quoted 12 lines · Dan Carpenter
This function is supposed to return negative kernel error codes but here it returns CMD_RST_PRC_EBUSY (2). The error code eventually gets passed to IS_ERR() and since it's not an error pointer it leads to an Oops in hns_roce_v1_rsv_lp_qp()
Signed-off-by Dan Carpenter <[email protected]>
---
Static analysis. Not tested.
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Quoted 12 lines

drivers/infiniband/hw/hns/hns_roce_hw_v2.c static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev, · line 1100
Quoted 8 lines
drivers/infiniband/hw/hns/hns_roce_hw_v2.c +0 -0
index ac017c24b200..018ff302ab9e 100644
@@ -1098,7 +1098,7 @@ static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 	if (ret = CMD_RST_PRC_SUCCESS)
 		return 0;
 	if (ret = CMD_RST_PRC_EBUSY)
The better fix will be to remove CMD_RST_PRC_* definitions in favor of normal errno.

Yes.

I've looked at that idea and I would almost feel like it's easy enough to send a patch like that without testing it at all. But it would be better if the people with the hardware sent it. I reported this bug months ago...

regards,
dan carpenter

oulijun

2019-06-13 06:14 UTC #5 lore raw

Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()

In reply to Dan Carpenter

ÔÚ 2019/6/13 1:23, Leon Romanovsky Ð´µÀ:

Quoted 24 lines · Dan Carpenter

Quoted 17 lines
This function is supposed to return negative kernel error codes but here it returns CMD_RST_PRC_EBUSY (2). The error code eventually gets passed to IS_ERR() and since it's not an error pointer it leads to an Oops in hns_roce_v1_rsv_lp_qp()
Signed-off-by Dan Carpenter <[email protected]>
---
Static analysis. Not tested.
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/infiniband/hw/hns/hns_roce_hw_v2.c +0 -0
index ac017c24b200..018ff302ab9e 100644
drivers/infiniband/hw/hns/hns_roce_hw_v2.c static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev, · line 1100
Quoted 3 lines
drivers/infiniband/hw/hns/hns_roce_hw_v2.c +0 -0
@@ -1098,7 +1098,7 @@ static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 	if (ret = CMD_RST_PRC_SUCCESS)
 		return 0;
 	if (ret = CMD_RST_PRC_EBUSY)
The better fix will be to remove CMD_RST_PRC_* definitions in favor of normal errno.

Quoted 4 lines

Thanks
.

Sorry, Our guys are analyzing his influence. So the response is a bit late.

Leon Romanovsky

2019-06-13 06:33 UTC #6 lore raw

Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()

In reply to Dan Carpenter

Quoted 27 lines · Dan Carpenter

Quoted 13 lines · Leon Romanovsky
Quoted 12 lines · Dan Carpenter
This function is supposed to return negative kernel error codes but here it returns CMD_RST_PRC_EBUSY (2). The error code eventually gets passed to IS_ERR() and since it's not an error pointer it leads to an Oops in hns_roce_v1_rsv_lp_qp()
Signed-off-by Dan Carpenter <[email protected]>
---
Static analysis. Not tested.
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 12 lines
drivers/infiniband/hw/hns/hns_roce_hw_v2.c static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev, · line 1100
Quoted 8 lines
drivers/infiniband/hw/hns/hns_roce_hw_v2.c +0 -0
index ac017c24b200..018ff302ab9e 100644
@@ -1098,7 +1098,7 @@ static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 	if (ret = CMD_RST_PRC_SUCCESS)
 		return 0;
 	if (ret = CMD_RST_PRC_EBUSY)
The better fix will be to remove CMD_RST_PRC_* definitions in favor of normal errno.

Quoted 6 lines

Yes.
I've looked at that idea and I would almost feel like it's easy enough to send a patch like that without testing it at all. But it would be better if the people with the hardware sent it. I reported this bug months ago...

Feel free to send, we will give time to respond.

thanks

Quoted 3 lines

regards,
dan carpenter

oulijun

2019-06-13 14:05 UTC #7 lore raw

Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()

In reply to Dan Carpenter

Quoted 32 lines · Dan Carpenter

Quoted 13 lines · Leon Romanovsky
Quoted 12 lines · Dan Carpenter
This function is supposed to return negative kernel error codes but here it returns CMD_RST_PRC_EBUSY (2). The error code eventually gets passed to IS_ERR() and since it's not an error pointer it leads to an Oops in hns_roce_v1_rsv_lp_qp()
Signed-off-by Dan Carpenter <[email protected]>
---
Static analysis. Not tested.
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 11 lines
drivers/infiniband/hw/hns/hns_roce_hw_v2.c static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev, · line 1100
Quoted 8 lines
drivers/infiniband/hw/hns/hns_roce_hw_v2.c +0 -0
index ac017c24b200..018ff302ab9e 100644
@@ -1098,7 +1098,7 @@ static int hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 	if (ret = CMD_RST_PRC_SUCCESS)
 		return 0;
 	if (ret = CMD_RST_PRC_EBUSY)
The better fix will be to remove CMD_RST_PRC_* definitions in favor of normal errno.
Yes.
I've looked at that idea and I would almost feel like it's easy enough to send a patch like that without testing it at all. But it would be better if the people with the hardware sent it. I reported this bug months ago...

Quoted 4 lines

regards,
dan carpenter
.

Hi, dan carpenter
Sorry to reply slowly. I have noticed before months ago when I see your email.
But we are sorting through our entire process and haven't had time to deal with it yet.

We are agree with your modifications after analysis. For leon's advice, We are
considering. If consider to use normal errno, it is not easy to review the entire reset for others.
We will give a patch for testing and send it.

Thanks.
Lijun Ou

Jason Gunthorpe

Applied

2019-06-25 13:22 UTC #8 lore raw

Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()

In reply to Dan Carpenter

Quoted 8 lines · Dan Carpenter

This function is supposed to return negative kernel error codes but here it returns CMD_RST_PRC_EBUSY (2). The error code eventually gets passed to IS_ERR() and since it's not an error pointer it leads to an Oops in hns_roce_v1_rsv_lp_qp()
Signed-off-by Dan Carpenter <[email protected]>
---
Static analysis. Not tested.

Applied to for-next, thanks

Jason