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

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

[Dan Carpenter]

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 <dan.carpenter@oracle.com>
---
 drivers/infiniband/hw/hns/hns_roce_srq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/hw/hns/hns_roce_srq.c b/drivers/infiniband/hw/hns/hns_roce_srq.c
index 463df60094e8..6377e734e28e 100644
--- a/drivers/infiniband/hw/hns/hns_roce_srq.c
+++ b/drivers/infiniband/hw/hns/hns_roce_srq.c
@@ -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;
 		}
 
-- 
2.17.1

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

[oulijun]

ÔÚ 2018/12/17 15:08, Dan Carpenter дµÀ:
> 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 <dan.carpenter@oracle.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_srq.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/infiniband/hw/hns/hns_roce_srq.c b/drivers/infiniband/hw/hns/hns_roce_srq.c
> index 463df60094e8..6377e734e28e 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_srq.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_srq.c
> @@ -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;
>  		}
>  

Ok£¬thanks

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

[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 <dan.carpenter@oracle.com>
---
Static analysis.  Not tested.

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

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index ac017c24b200..018ff302ab9e 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -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;
-- 
2.20.1

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

[Leon Romanovsky]

On Sat, Jun 08, 2019 at 12:27:14PM +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> ---
> Static analysis.  Not tested.
>
>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> index ac017c24b200..018ff302ab9e 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> @@ -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

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

[Dan Carpenter]

On Wed, Jun 12, 2019 at 08:23:16PM +0300, Leon Romanovsky wrote:
> On Sat, Jun 08, 2019 at 12:27:14PM +0300, Dan Carpenter wrote:
> > 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 <dan.carpenter@oracle.com>
> > ---
> > Static analysis.  Not tested.
> >
> >  drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> > index ac017c24b200..018ff302ab9e 100644
> > --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> > +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> > @@ -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

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

[oulijun]

ÔÚ 2019/6/13 1:23, Leon Romanovsky дµÀ:
> On Sat, Jun 08, 2019 at 12:27:14PM +0300, Dan Carpenter wrote:
>> 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 <dan.carpenter@oracle.com>
>> ---
>> Static analysis.  Not tested.
>>
>>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> index ac017c24b200..018ff302ab9e 100644
>> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>> @@ -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
>
> .
>
Sorry, Our guys are analyzing his influence. So the response is a bit late.

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

[Leon Romanovsky]

On Wed, Jun 12, 2019 at 11:05:17PM -0700, Dan Carpenter wrote:
> On Wed, Jun 12, 2019 at 08:23:16PM +0300, Leon Romanovsky wrote:
> > On Sat, Jun 08, 2019 at 12:27:14PM +0300, Dan Carpenter wrote:
> > > 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 <dan.carpenter@oracle.com>
> > > ---
> > > Static analysis.  Not tested.
> > >
> > >  drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> > > index ac017c24b200..018ff302ab9e 100644
> > > --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> > > +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> > > @@ -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...

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

thanks

>
> regards,
> dan carpenter

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

[oulijun]

在 2019/6/13 14:05, Dan Carpenter 写道:
> On Wed, Jun 12, 2019 at 08:23:16PM +0300, Leon Romanovsky wrote:
>> On Sat, Jun 08, 2019 at 12:27:14PM +0300, Dan Carpenter wrote:
>>> 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 <dan.carpenter@oracle.com>
>>> ---
>>> Static analysis.  Not tested.
>>>
>>>  drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>>> index ac017c24b200..018ff302ab9e 100644
>>> --- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
>>> @@ -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
>
> .
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
   

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

[Jason Gunthorpe]

On Sat, Jun 08, 2019 at 12:27:14PM +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> ---
> Static analysis.  Not tested.

Applied to for-next, thanks

Jason