Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp
From: Wei Hu (Xavier)
Date: Fri Sep 29 2017 - 09:15:54 EST
On 2017/9/29 18:23, Leon Romanovsky wrote:
On Fri, Sep 29, 2017 at 02:07:22PM +0800, Wei Hu (Xavier) wrote:
On 2017/9/28 20:59, Leon Romanovsky wrote:
On Thu, Sep 28, 2017 at 07:56:59PM +0800, Wei Hu (Xavier) wrote:
On 2017/9/28 17:13, Leon Romanovsky wrote:
On Thu, Sep 28, 2017 at 12:57:28PM +0800, Wei Hu (Xavier) wrote:
From: Lijun Ou <oulijun@xxxxxxxxxx>
When lp_qp_work is NULL, it should be returned ENOMEM. This patch
mainly fixes it.
Ihis patch fixes the smatch error as below:
drivers/infiniband/hw/hns/hns_roce_hw_v1.c:918 hns_roce_v1_recreate_lp_qp()
error: potential null dereference 'lp_qp_work'. (kzalloc returns null)
Signed-off-by: Lijun Ou <oulijun@xxxxxxxxxx>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@xxxxxxxxxx>
Signed-off-by: Shaobo Xu <xushaobo2@xxxxxxxxxx>
---
drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
index 95f5c88..1071fa2 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
@@ -912,6 +912,8 @@ static int hns_roce_v1_recreate_lp_qp(struct hns_roce_dev *hr_dev)
lp_qp_work = kzalloc(sizeof(struct hns_roce_recreate_lp_qp_work),
GFP_KERNEL);
+ if (!lp_qp_work)
+ return -ENOMEM;
You will treat this error in the same was as you will treat timeout,
which is wrong.
Thanks, Leon
We will send v2 to fix the compatible warn info.
No, you missed the point.
From the code flow below the behavior of hns_roce_v1_recreate_lp_qp
for ENOMEM and ETIMEOUT returns will be the same and it is wrong.
For the ETIMEOUT, you can continue, for ENOMEM, you should properly
unfold the whole flow.
Thanks
Hi, Leon
We prepare to modify the warn info as bleow:
if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
dev_warn(&hr_dev->pdev->dev, "recreate lp qp failed!\n");
for -ETIMEDOUT, there is a warn info as blow, but there isn't this one
for -ENOMEM.
dev_warn(dev, "recreate lp qp failed 20s timeout and return
failed!\n");
static int hns_roce_v1_recreate_lp_qp(struct hns_roce_dev *hr_dev)
{
<snip>
lp_qp_work = kzalloc(sizeof(struct
hns_roce_recreate_lp_qp_work),
GFP_KERNEL);
if (!lp_qp_work)
return -ENOMEM;
<snip>
dev_warn(dev, "recreate lp qp failed 20s timeout and return
failed!\n");
return -ETIMEDOUT;
}
Regards
Wei Hu
Hi Wei,
It will be helpful, if you post your suggestions in git diff format.
My expectation is to see the following code:
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
index 747efd1ae5a6..0b9ec7c24f2d 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
@@ -1648,14 +1648,18 @@ void hns_roce_v1_set_mac(struct hns_roce_dev *hr_dev, u8 phy_port, u8 *addr)
u16 *p_h;
u32 *p;
u32 val;
/*
* When mac changed, loopback may fail
* because of smac not equal to dmac.
* We Need to release and create reserved qp again.
*/
- if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
- dev_warn(&hr_dev->pdev->dev, "recreate lp qp timeout!\n");
+ if (hr_dev->hw->dereg_mr) {
+ int ret;
+ ret = hns_roce_v1_recreate_lp_qp(hr_dev);
+ if (ret && ret != -ETIMEDOUT)
+ return ret;
+ }
p = (u32 *)(&addr[0]);
reg_smac_l = *p;
Thanks
Hi, Leon
Thanks for your suggestion. We will send patch v2 to fix it.
Best Regard
Wei Hu