Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-29 Thread Wei Hu (Xavier) 



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 

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 
Signed-off-by: Wei Hu (Xavier) 
Signed-off-by: Shaobo Xu 
---
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(_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)
 {
 
 lp_qp_work = kzalloc(sizeof(struct
hns_roce_recreate_lp_qp_work),
  GFP_KERNEL);
 if (!lp_qp_work)
 return -ENOMEM;

 
 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(_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 *)([0]);
reg_smac_l = *p;

Thanks

Hi, Leon
    Thanks for your suggestion. We will send patch v2 to fix it.

    Best Regard
Wei Hu

Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-29 Thread Wei Hu (Xavier) 



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 

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 
Signed-off-by: Wei Hu (Xavier) 
Signed-off-by: Shaobo Xu 
---
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(_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)
 {
 
 lp_qp_work = kzalloc(sizeof(struct
hns_roce_recreate_lp_qp_work),
  GFP_KERNEL);
 if (!lp_qp_work)
 return -ENOMEM;

 
 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(_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 *)([0]);
reg_smac_l = *p;

Thanks

Hi, Leon
    Thanks for your suggestion. We will send patch v2 to fix it.

    Best Regard
Wei Hu

Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-29 Thread Leon Romanovsky 
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 
> > > > >
> > > > > 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 
> > > > > Signed-off-by: Wei Hu (Xavier) 
> > > > > Signed-off-by: Shaobo Xu 
> > > > > ---
> > > > >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(_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)
> {
> 
> lp_qp_work = kzalloc(sizeof(struct
> hns_roce_recreate_lp_qp_work),
>  GFP_KERNEL);
> if (!lp_qp_work)
> return -ENOMEM;
>
> 
> 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(_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 *)([0]);
reg_smac_l = *p;

Thanks


signature.asc
Description: PGP signature

Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-29 Thread Leon Romanovsky 
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 
> > > > >
> > > > > 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 
> > > > > Signed-off-by: Wei Hu (Xavier) 
> > > > > Signed-off-by: Shaobo Xu 
> > > > > ---
> > > > >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(_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)
> {
> 
> lp_qp_work = kzalloc(sizeof(struct
> hns_roce_recreate_lp_qp_work),
>  GFP_KERNEL);
> if (!lp_qp_work)
> return -ENOMEM;
>
> 
> 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(_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 *)([0]);
reg_smac_l = *p;

Thanks


signature.asc
Description: PGP signature

Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-29 Thread Wei Hu (Xavier) 



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 

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 
Signed-off-by: Wei Hu (Xavier) 
Signed-off-by: Shaobo Xu 
---
   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(_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)
{

lp_qp_work = kzalloc(sizeof(struct 
hns_roce_recreate_lp_qp_work),

 GFP_KERNEL);
if (!lp_qp_work)
return -ENOMEM;


dev_warn(dev, "recreate lp qp failed 20s timeout and return 
failed!\n");

return -ETIMEDOUT;
}

Regards
Wei Hu

1656  */
1657 if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
1658 dev_warn(_dev->pdev->dev, "recreate lp qp timeout!\n");
1659
1660 p = (u32 *)([0]);



INIT_WORK(&(lp_qp_work->work), hns_roce_v1_recreate_lp_qp_work_fn);

--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-29 Thread Wei Hu (Xavier) 



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 

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 
Signed-off-by: Wei Hu (Xavier) 
Signed-off-by: Shaobo Xu 
---
   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(_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)
{

lp_qp_work = kzalloc(sizeof(struct 
hns_roce_recreate_lp_qp_work),

 GFP_KERNEL);
if (!lp_qp_work)
return -ENOMEM;


dev_warn(dev, "recreate lp qp failed 20s timeout and return 
failed!\n");

return -ETIMEDOUT;
}

Regards
Wei Hu

1656  */
1657 if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
1658 dev_warn(_dev->pdev->dev, "recreate lp qp timeout!\n");
1659
1660 p = (u32 *)([0]);



INIT_WORK(&(lp_qp_work->work), hns_roce_v1_recreate_lp_qp_work_fn);

--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-28 Thread Leon Romanovsky 
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 
> > >
> > > 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 
> > > Signed-off-by: Wei Hu (Xavier) 
> > > Signed-off-by: Shaobo Xu 
> > > ---
> > >   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


> > 1656  */
> > 1657 if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
> > 1658 dev_warn(_dev->pdev->dev, "recreate lp qp 
> > timeout!\n");
> > 1659
> > 1660 p = (u32 *)([0]);
> >
> >
> > >   INIT_WORK(&(lp_qp_work->work), 
> > > hns_roce_v1_recreate_lp_qp_work_fn);
> > >
> > > --
> > > 1.9.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > > the body of a message to majord...@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-28 Thread Leon Romanovsky 
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 
> > >
> > > 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 
> > > Signed-off-by: Wei Hu (Xavier) 
> > > Signed-off-by: Shaobo Xu 
> > > ---
> > >   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


> > 1656  */
> > 1657 if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
> > 1658 dev_warn(_dev->pdev->dev, "recreate lp qp 
> > timeout!\n");
> > 1659
> > 1660 p = (u32 *)([0]);
> >
> >
> > >   INIT_WORK(&(lp_qp_work->work), 
> > > hns_roce_v1_recreate_lp_qp_work_fn);
> > >
> > > --
> > > 1.9.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> > > the body of a message to majord...@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-28 Thread Wei Hu (Xavier) 



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 

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 
Signed-off-by: Wei Hu (Xavier) 
Signed-off-by: Shaobo Xu 
---
  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.

1656  */
1657 if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
1658 dev_warn(_dev->pdev->dev, "recreate lp qp timeout!\n");
1659
1660 p = (u32 *)([0]);



INIT_WORK(&(lp_qp_work->work), hns_roce_v1_recreate_lp_qp_work_fn);

--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-28 Thread Wei Hu (Xavier) 



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 

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 
Signed-off-by: Wei Hu (Xavier) 
Signed-off-by: Shaobo Xu 
---
  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.

1656  */
1657 if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
1658 dev_warn(_dev->pdev->dev, "recreate lp qp timeout!\n");
1659
1660 p = (u32 *)([0]);



INIT_WORK(&(lp_qp_work->work), hns_roce_v1_recreate_lp_qp_work_fn);

--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-28 Thread Leon Romanovsky 
On Thu, Sep 28, 2017 at 12:57:28PM +0800, Wei Hu (Xavier) wrote:
> From: Lijun Ou 
>
> 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 
> Signed-off-by: Wei Hu (Xavier) 
> Signed-off-by: Shaobo Xu 
> ---
>  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.

1656  */
1657 if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
1658 dev_warn(_dev->pdev->dev, "recreate lp qp timeout!\n");
1659
1660 p = (u32 *)([0]);


>   INIT_WORK(&(lp_qp_work->work), hns_roce_v1_recreate_lp_qp_work_fn);
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

Re: [PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-28 Thread Leon Romanovsky 
On Thu, Sep 28, 2017 at 12:57:28PM +0800, Wei Hu (Xavier) wrote:
> From: Lijun Ou 
>
> 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 
> Signed-off-by: Wei Hu (Xavier) 
> Signed-off-by: Shaobo Xu 
> ---
>  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.

1656  */
1657 if (hr_dev->hw->dereg_mr && hns_roce_v1_recreate_lp_qp(hr_dev))
1658 dev_warn(_dev->pdev->dev, "recreate lp qp timeout!\n");
1659
1660 p = (u32 *)([0]);


>   INIT_WORK(&(lp_qp_work->work), hns_roce_v1_recreate_lp_qp_work_fn);
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

[PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-27 Thread Wei Hu (Xavier) 
From: Lijun Ou 

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 
Signed-off-by: Wei Hu (Xavier) 
Signed-off-by: Shaobo Xu 
---
 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;
 
INIT_WORK(&(lp_qp_work->work), hns_roce_v1_recreate_lp_qp_work_fn);
 
-- 
1.9.1

[PATCH for-next 3/9] RDMA/hns: Add return statement when kzalloc return NULL in hns_roce_v1_recreate_lp_qp

2017-09-27 Thread Wei Hu (Xavier) 
From: Lijun Ou 

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 
Signed-off-by: Wei Hu (Xavier) 
Signed-off-by: Shaobo Xu 
---
 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;
 
INIT_WORK(&(lp_qp_work->work), hns_roce_v1_recreate_lp_qp_work_fn);
 
-- 
1.9.1