kernel-janitors.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] RDMA/hns: Fix an error code in hns_roce_create_srq()
@ 2018-12-17  7:08 Dan Carpenter
  2018-12-17 11:18 ` oulijun
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Dan Carpenter @ 2018-12-17  7:08 UTC (permalink / raw)
  To: kernel-janitors

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_create_srq()
  2018-12-17  7:08 [PATCH] RDMA/hns: Fix an error code in hns_roce_create_srq() Dan Carpenter
@ 2018-12-17 11:18 ` oulijun
  2019-06-08  9:27 ` [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size() Dan Carpenter
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: oulijun @ 2018-12-17 11:18 UTC (permalink / raw)
  To: kernel-janitors

ÔÚ 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()
  2018-12-17  7:08 [PATCH] RDMA/hns: Fix an error code in hns_roce_create_srq() Dan Carpenter
  2018-12-17 11:18 ` oulijun
@ 2019-06-08  9:27 ` Dan Carpenter
  2019-06-12 17:23 ` Leon Romanovsky
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2019-06-08  9:27 UTC (permalink / raw)
  To: kernel-janitors

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()
  2018-12-17  7:08 [PATCH] RDMA/hns: Fix an error code in hns_roce_create_srq() Dan Carpenter
  2018-12-17 11:18 ` oulijun
  2019-06-08  9:27 ` [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size() Dan Carpenter
@ 2019-06-12 17:23 ` Leon Romanovsky
  2019-06-13  6:05 ` Dan Carpenter
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2019-06-12 17:23 UTC (permalink / raw)
  To: kernel-janitors

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()
  2018-12-17  7:08 [PATCH] RDMA/hns: Fix an error code in hns_roce_create_srq() Dan Carpenter
                   ` (2 preceding siblings ...)
  2019-06-12 17:23 ` Leon Romanovsky
@ 2019-06-13  6:05 ` Dan Carpenter
  2019-06-13  6:14 ` oulijun
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2019-06-13  6:05 UTC (permalink / raw)
  To: kernel-janitors

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()
  2018-12-17  7:08 [PATCH] RDMA/hns: Fix an error code in hns_roce_create_srq() Dan Carpenter
                   ` (3 preceding siblings ...)
  2019-06-13  6:05 ` Dan Carpenter
@ 2019-06-13  6:14 ` oulijun
  2019-06-13  6:33 ` Leon Romanovsky
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: oulijun @ 2019-06-13  6:14 UTC (permalink / raw)
  To: kernel-janitors

ÔÚ 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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()
  2018-12-17  7:08 [PATCH] RDMA/hns: Fix an error code in hns_roce_create_srq() Dan Carpenter
                   ` (4 preceding siblings ...)
  2019-06-13  6:14 ` oulijun
@ 2019-06-13  6:33 ` Leon Romanovsky
  2019-06-13 14:05 ` oulijun
  2019-06-25 13:22 ` Jason Gunthorpe
  7 siblings, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2019-06-13  6:33 UTC (permalink / raw)
  To: kernel-janitors

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()
  2018-12-17  7:08 [PATCH] RDMA/hns: Fix an error code in hns_roce_create_srq() Dan Carpenter
                   ` (5 preceding siblings ...)
  2019-06-13  6:33 ` Leon Romanovsky
@ 2019-06-13 14:05 ` oulijun
  2019-06-25 13:22 ` Jason Gunthorpe
  7 siblings, 0 replies; 9+ messages in thread
From: oulijun @ 2019-06-13 14:05 UTC (permalink / raw)
  To: kernel-janitors

在 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
   

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size()
  2018-12-17  7:08 [PATCH] RDMA/hns: Fix an error code in hns_roce_create_srq() Dan Carpenter
                   ` (6 preceding siblings ...)
  2019-06-13 14:05 ` oulijun
@ 2019-06-25 13:22 ` Jason Gunthorpe
  7 siblings, 0 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2019-06-25 13:22 UTC (permalink / raw)
  To: kernel-janitors

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-06-25 13:22 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-17  7:08 [PATCH] RDMA/hns: Fix an error code in hns_roce_create_srq() Dan Carpenter
2018-12-17 11:18 ` oulijun
2019-06-08  9:27 ` [PATCH] RDMA/hns: Fix an error code in hns_roce_set_user_sq_size() Dan Carpenter
2019-06-12 17:23 ` Leon Romanovsky
2019-06-13  6:05 ` Dan Carpenter
2019-06-13  6:14 ` oulijun
2019-06-13  6:33 ` Leon Romanovsky
2019-06-13 14:05 ` oulijun
2019-06-25 13:22 ` Jason Gunthorpe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).