[PATCH] Staging: rtl8723bs: Remove functions and replace return types

[PATCH] Staging: rtl8723bs: Remove functions and replace return types

[Madhumitha Prabakaran]

- Remove functions rtw_init_cmd_priv and rtw_init_evt_priv, as its only
purpose is to call the other functions _rtw_init_cmd_priv and
_rtw_init_evt_priv, respectively.
- Rename functions from _rtw_init_cmd_priv and _rtw_init_evt_priv, to
rtw_init_cmd_priv and rtw_init_evt_priv.
- Replace return types from sint to u32
- Replace local variable 'res' data type from sint to u32 in both the
functions.
Issue suggested by Coccinelle using ret.cocci.

Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_cmd.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index 91520ca3bbad..d8c362947dd4 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -162,9 +162,9 @@ Caller and the rtw_cmd_thread can protect cmd_q by spin_lock.
 No irqsave is necessary.
 */
 
-sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
+u32 rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
 {
-	sint res = _SUCCESS;
+	u32 res = _SUCCESS;
 
 	init_completion(&pcmdpriv->cmd_queue_comp);
 	init_completion(&pcmdpriv->terminate_cmdthread_comp);
@@ -201,9 +201,9 @@ sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
 }
 
 static void c2h_wk_callback(_workitem *work);
-sint _rtw_init_evt_priv(struct evt_priv *pevtpriv)
+u32 rtw_init_evt_priv(struct evt_priv *pevtpriv)
 {
-	sint res = _SUCCESS;
+	u32 res = _SUCCESS;
 
 	/* allocate DMA-able/Non-Page memory for cmd_buf and rsp_buf */
 	atomic_set(&pevtpriv->event_seq, 0);
@@ -295,22 +295,6 @@ struct	cmd_obj	*_rtw_dequeue_cmd(struct __queue *queue)
 	return obj;
 }
 
-u32 rtw_init_cmd_priv(struct cmd_priv *pcmdpriv)
-{
-	u32 res;
-
-	res = _rtw_init_cmd_priv(pcmdpriv);
-	return res;
-}
-
-u32 rtw_init_evt_priv(struct	evt_priv *pevtpriv)
-{
-	int	res;
-
-	res = _rtw_init_evt_priv(pevtpriv);
-	return res;
-}
-
 void rtw_free_evt_priv(struct	evt_priv *pevtpriv)
 {
 	RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, ("rtw_free_evt_priv\n"));
-- 
2.17.1

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: Remove functions and replace return types

[Julia Lawall]

On Thu, 14 Mar 2019, Madhumitha Prabakaran wrote:

> - Remove functions rtw_init_cmd_priv and rtw_init_evt_priv, as its only
> purpose is to call the other functions _rtw_init_cmd_priv and
> _rtw_init_evt_priv, respectively.
> - Rename functions from _rtw_init_cmd_priv and _rtw_init_evt_priv, to
> rtw_init_cmd_priv and rtw_init_evt_priv.
> - Replace return types from sint to u32
> - Replace local variable 'res' data type from sint to u32 in both the
> functions.
> Issue suggested by Coccinelle using ret.cocci.

OK, this is a really good first step, with a nice log message as well.
However, looking a bit further, there is more to do.

We can observe that you changed the return type from signed to unsigned,
and it is a little bit surprising that this is ok.

Looking at the definitions of these functions it seems that they return
error codes, _SUCCESS and _FAIL.  Often in the Linux kernel, success is
indicated by 0 and failure is indicated by a negative number, so one may
still be surprised by the change of type.

But this driver actually returns 1 for success and 0 for failure, so the
code remains correctly functioning with the change of type.

Still, these are not really ideal error values.  To be compatible with the
rest of the kernel, success should be 0 and failure should be -ENOMEM,
because these are memory allocating functions.  In that case the return
type of these functions should be int, rather than u32.

Finally, _rtw_init_evt_priv actually never returns _FAIL.  The memory
allocation in this function bottoms out at kmalloc, which can fail, so
there needs to be a check for this, and to return an -ENOMEM value.

Checking on all of that leads to:

void *_rtw_malloc(u32 sz)
{
	return kmalloc(sz, in_interrupt() ? GFP_ATOMIC : GFP_KERNEL);
}

which is incorrect.  Each call site needs to be checked to determine
whether it is in atomic context (accessible from an interrupt handler or
with a lock held) and to use GFP_ATOMIC if that is the case and GFP_KERNEL
otherwise.

I would suggest to clean up everything up to the paragraph starting with
"Finally" in a single series.  Then, if you want to work more on this
driver, you can look for other uses of _SUCCESS and _FAIL and convert them
to more appropriate return values, and convert calls to rtw_malloc to
calls to kmalloc with the correct flag argument.  There is also a function
rtw_zalloc that should be kzalloc with the same issues.  Actually, the
file osdep_service.c hasa good set of unsuitable definitions that should
be cleaned up.

julia

>
> Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_cmd.c | 24 ++++--------------------
>  1 file changed, 4 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index 91520ca3bbad..d8c362947dd4 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -162,9 +162,9 @@ Caller and the rtw_cmd_thread can protect cmd_q by spin_lock.
>  No irqsave is necessary.
>  */
>
> -sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> +u32 rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
>  {
> -	sint res = _SUCCESS;
> +	u32 res = _SUCCESS;
>
>  	init_completion(&pcmdpriv->cmd_queue_comp);
>  	init_completion(&pcmdpriv->terminate_cmdthread_comp);
> @@ -201,9 +201,9 @@ sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
>  }
>
>  static void c2h_wk_callback(_workitem *work);
> -sint _rtw_init_evt_priv(struct evt_priv *pevtpriv)
> +u32 rtw_init_evt_priv(struct evt_priv *pevtpriv)
>  {
> -	sint res = _SUCCESS;
> +	u32 res = _SUCCESS;
>
>  	/* allocate DMA-able/Non-Page memory for cmd_buf and rsp_buf */
>  	atomic_set(&pevtpriv->event_seq, 0);
> @@ -295,22 +295,6 @@ struct	cmd_obj	*_rtw_dequeue_cmd(struct __queue *queue)
>  	return obj;
>  }
>
> -u32 rtw_init_cmd_priv(struct cmd_priv *pcmdpriv)
> -{
> -	u32 res;
> -
> -	res = _rtw_init_cmd_priv(pcmdpriv);
> -	return res;
> -}
> -
> -u32 rtw_init_evt_priv(struct	evt_priv *pevtpriv)
> -{
> -	int	res;
> -
> -	res = _rtw_init_evt_priv(pevtpriv);
> -	return res;
> -}
> -
>  void rtw_free_evt_priv(struct	evt_priv *pevtpriv)
>  {
>  	RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, ("rtw_free_evt_priv\n"));
> --
> 2.17.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20190314223416.3994-1-madhumithabiw%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: Remove functions and replace return types

[Madhumthia Prabakaran]

On Fri, Mar 15, 2019 at 07:50:57AM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 14 Mar 2019, Madhumitha Prabakaran wrote:
> 
> > - Remove functions rtw_init_cmd_priv and rtw_init_evt_priv, as its only
> > purpose is to call the other functions _rtw_init_cmd_priv and
> > _rtw_init_evt_priv, respectively.
> > - Rename functions from _rtw_init_cmd_priv and _rtw_init_evt_priv, to
> > rtw_init_cmd_priv and rtw_init_evt_priv.
> > - Replace return types from sint to u32
> > - Replace local variable 'res' data type from sint to u32 in both the
> > functions.
> > Issue suggested by Coccinelle using ret.cocci.
> 
> OK, this is a really good first step, with a nice log message as well.
> However, looking a bit further, there is more to do.
> 
> We can observe that you changed the return type from signed to unsigned,
> and it is a little bit surprising that this is ok.
> 
> Looking at the definitions of these functions it seems that they return
> error codes, _SUCCESS and _FAIL.  Often in the Linux kernel, success is
> indicated by 0 and failure is indicated by a negative number, so one may
> still be surprised by the change of type.

I didn't  changed _SUCCESS value to 0 and _FAIL value to -ENOMEM. yet
again it complied. I want to confirm whether I have to change it
directly or there is function RTW_STATUS_CODE in os_dep/osdep_service.c,
which can be modified to make _SUCCESS and _FAIL compatible with rest of
the kernel
> 
> But this driver actually returns 1 for success and 0 for failure, so the
> code remains correctly functioning with the change of type.
> 
> Still, these are not really ideal error values.  To be compatible with the
> rest of the kernel, success should be 0 and failure should be -ENOMEM,
> because these are memory allocating functions.  In that case the return
> type of these functions should be int, rather than u32.
> 
> Finally, _rtw_init_evt_priv actually never returns _FAIL.  The memory
> allocation in this function bottoms out at kmalloc, which can fail, so
> there needs to be a check for this, and to return an -ENOMEM value.
>
> Checking on all of that leads to:
> 
> void *_rtw_malloc(u32 sz)
> {
> 	return kmalloc(sz, in_interrupt() ? GFP_ATOMIC : GFP_KERNEL);
> }
> 
> which is incorrect.  Each call site needs to be checked to determine
> whether it is in atomic context (accessible from an interrupt handler or
> with a lock held) and to use GFP_ATOMIC if that is the case and GFP_KERNEL
> otherwise.
> 
> I would suggest to clean up everything up to the paragraph starting with
> "Finally" in a single series.  Then, if you want to work more on this
> driver, you can look for other uses of _SUCCESS and _FAIL and convert them
> to more appropriate return values, and convert calls to rtw_malloc to
> calls to kmalloc with the correct flag argument.  There is also a function
> rtw_zalloc that should be kzalloc with the same issues.  Actually, the
> file osdep_service.c hasa good set of unsuitable definitions that should
> be cleaned up.

I sent it as a patch series. I will work on all unsuitable definitions
in osdep_service.c.

> 
> julia
> 
> >
> > Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> > ---
> >  drivers/staging/rtl8723bs/core/rtw_cmd.c | 24 ++++--------------------
> >  1 file changed, 4 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > index 91520ca3bbad..d8c362947dd4 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > @@ -162,9 +162,9 @@ Caller and the rtw_cmd_thread can protect cmd_q by spin_lock.
> >  No irqsave is necessary.
> >  */
> >
> > -sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> > +u32 rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> >  {
> > -	sint res = _SUCCESS;
> > +	u32 res = _SUCCESS;
> >
> >  	init_completion(&pcmdpriv->cmd_queue_comp);
> >  	init_completion(&pcmdpriv->terminate_cmdthread_comp);
> > @@ -201,9 +201,9 @@ sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> >  }
> >
> >  static void c2h_wk_callback(_workitem *work);
> > -sint _rtw_init_evt_priv(struct evt_priv *pevtpriv)
> > +u32 rtw_init_evt_priv(struct evt_priv *pevtpriv)
> >  {
> > -	sint res = _SUCCESS;
> > +	u32 res = _SUCCESS;
> >
> >  	/* allocate DMA-able/Non-Page memory for cmd_buf and rsp_buf */
> >  	atomic_set(&pevtpriv->event_seq, 0);
> > @@ -295,22 +295,6 @@ struct	cmd_obj	*_rtw_dequeue_cmd(struct __queue *queue)
> >  	return obj;
> >  }
> >
> > -u32 rtw_init_cmd_priv(struct cmd_priv *pcmdpriv)
> > -{
> > -	u32 res;
> > -
> > -	res = _rtw_init_cmd_priv(pcmdpriv);
> > -	return res;
> > -}
> > -
> > -u32 rtw_init_evt_priv(struct	evt_priv *pevtpriv)
> > -{
> > -	int	res;
> > -
> > -	res = _rtw_init_evt_priv(pevtpriv);
> > -	return res;
> > -}
> > -
> >  void rtw_free_evt_priv(struct	evt_priv *pevtpriv)
> >  {
> >  	RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, ("rtw_free_evt_priv\n"));
> > --
> > 2.17.1
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20190314223416.3994-1-madhumithabiw%40gmail.com.
> > For more options, visit https://groups.google.com/d/optout.
> >

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: Remove functions and replace return types

[Julia Lawall]

On Sat, 16 Mar 2019, Madhumthia Prabakaran wrote:

> On Fri, Mar 15, 2019 at 07:50:57AM +0100, Julia Lawall wrote:
> >
> >
> > On Thu, 14 Mar 2019, Madhumitha Prabakaran wrote:
> >
> > > - Remove functions rtw_init_cmd_priv and rtw_init_evt_priv, as its only
> > > purpose is to call the other functions _rtw_init_cmd_priv and
> > > _rtw_init_evt_priv, respectively.
> > > - Rename functions from _rtw_init_cmd_priv and _rtw_init_evt_priv, to
> > > rtw_init_cmd_priv and rtw_init_evt_priv.
> > > - Replace return types from sint to u32
> > > - Replace local variable 'res' data type from sint to u32 in both the
> > > functions.
> > > Issue suggested by Coccinelle using ret.cocci.
> >
> > OK, this is a really good first step, with a nice log message as well.
> > However, looking a bit further, there is more to do.
> >
> > We can observe that you changed the return type from signed to unsigned,
> > and it is a little bit surprising that this is ok.
> >
> > Looking at the definitions of these functions it seems that they return
> > error codes, _SUCCESS and _FAIL.  Often in the Linux kernel, success is
> > indicated by 0 and failure is indicated by a negative number, so one may
> > still be surprised by the change of type.
>
> I didn't  changed _SUCCESS value to 0 and _FAIL value to -ENOMEM. yet
> again it complied. I want to confirm whether I have to change it
> directly or there is function RTW_STATUS_CODE in os_dep/osdep_service.c,
> which can be modified to make _SUCCESS and _FAIL compatible with rest of
> the kernel

Please don't use _SUCCESS and _FAIL.  It doesn't really matter what these
symbols are bound to.  Using them won't cause a compiler error.  The
problem is that they are the wrong symbols.  Success should be indicated
with 0 and failure, in the case of a memory allocation, should be
indicated with -ENOMEM.  Other kinds of failures have other error codes
(-EINVAL, -ENODEV, etc).

> >
> > But this driver actually returns 1 for success and 0 for failure, so the
> > code remains correctly functioning with the change of type.
> >
> > Still, these are not really ideal error values.  To be compatible with the
> > rest of the kernel, success should be 0 and failure should be -ENOMEM,
> > because these are memory allocating functions.  In that case the return
> > type of these functions should be int, rather than u32.
> >
> > Finally, _rtw_init_evt_priv actually never returns _FAIL.  The memory
> > allocation in this function bottoms out at kmalloc, which can fail, so
> > there needs to be a check for this, and to return an -ENOMEM value.
> >
> > Checking on all of that leads to:
> >
> > void *_rtw_malloc(u32 sz)
> > {
> > 	return kmalloc(sz, in_interrupt() ? GFP_ATOMIC : GFP_KERNEL);
> > }
> >
> > which is incorrect.  Each call site needs to be checked to determine
> > whether it is in atomic context (accessible from an interrupt handler or
> > with a lock held) and to use GFP_ATOMIC if that is the case and GFP_KERNEL
> > otherwise.
> >
> > I would suggest to clean up everything up to the paragraph starting with
> > "Finally" in a single series.  Then, if you want to work more on this
> > driver, you can look for other uses of _SUCCESS and _FAIL and convert them
> > to more appropriate return values, and convert calls to rtw_malloc to
> > calls to kmalloc with the correct flag argument.  There is also a function
> > rtw_zalloc that should be kzalloc with the same issues.  Actually, the
> > file osdep_service.c hasa good set of unsuitable definitions that should
> > be cleaned up.
>
> I sent it as a patch series. I will work on all unsuitable definitions
> in osdep_service.c.

Actually, it is not really the definitions that need to be cleaned up.
The uses of these functions need to be replaced by direct calls to the
standard kernel functions, and then the functions in osdep_service.c
should be dropped.

julia

>
> >
> > julia
> >
> > >
> > > Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> > > ---
> > >  drivers/staging/rtl8723bs/core/rtw_cmd.c | 24 ++++--------------------
> > >  1 file changed, 4 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > index 91520ca3bbad..d8c362947dd4 100644
> > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > @@ -162,9 +162,9 @@ Caller and the rtw_cmd_thread can protect cmd_q by spin_lock.
> > >  No irqsave is necessary.
> > >  */
> > >
> > > -sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> > > +u32 rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> > >  {
> > > -	sint res = _SUCCESS;
> > > +	u32 res = _SUCCESS;
> > >
> > >  	init_completion(&pcmdpriv->cmd_queue_comp);
> > >  	init_completion(&pcmdpriv->terminate_cmdthread_comp);
> > > @@ -201,9 +201,9 @@ sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> > >  }
> > >
> > >  static void c2h_wk_callback(_workitem *work);
> > > -sint _rtw_init_evt_priv(struct evt_priv *pevtpriv)
> > > +u32 rtw_init_evt_priv(struct evt_priv *pevtpriv)
> > >  {
> > > -	sint res = _SUCCESS;
> > > +	u32 res = _SUCCESS;
> > >
> > >  	/* allocate DMA-able/Non-Page memory for cmd_buf and rsp_buf */
> > >  	atomic_set(&pevtpriv->event_seq, 0);
> > > @@ -295,22 +295,6 @@ struct	cmd_obj	*_rtw_dequeue_cmd(struct __queue *queue)
> > >  	return obj;
> > >  }
> > >
> > > -u32 rtw_init_cmd_priv(struct cmd_priv *pcmdpriv)
> > > -{
> > > -	u32 res;
> > > -
> > > -	res = _rtw_init_cmd_priv(pcmdpriv);
> > > -	return res;
> > > -}
> > > -
> > > -u32 rtw_init_evt_priv(struct	evt_priv *pevtpriv)
> > > -{
> > > -	int	res;
> > > -
> > > -	res = _rtw_init_evt_priv(pevtpriv);
> > > -	return res;
> > > -}
> > > -
> > >  void rtw_free_evt_priv(struct	evt_priv *pevtpriv)
> > >  {
> > >  	RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, ("rtw_free_evt_priv\n"));
> > > --
> > > 2.17.1
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20190314223416.3994-1-madhumithabiw%40gmail.com.
> > > For more options, visit https://groups.google.com/d/optout.
> > >
>

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: Remove functions and replace return types

[Madhumthia Prabakaran]

On Sat, Mar 16, 2019 at 02:23:03PM +0100, Julia Lawall wrote:
> 
> 
> On Sat, 16 Mar 2019, Madhumthia Prabakaran wrote:
> 
> > On Fri, Mar 15, 2019 at 07:50:57AM +0100, Julia Lawall wrote:
> > >
> > >
> > > On Thu, 14 Mar 2019, Madhumitha Prabakaran wrote:
> > >
> > > > - Remove functions rtw_init_cmd_priv and rtw_init_evt_priv, as its only
> > > > purpose is to call the other functions _rtw_init_cmd_priv and
> > > > _rtw_init_evt_priv, respectively.
> > > > - Rename functions from _rtw_init_cmd_priv and _rtw_init_evt_priv, to
> > > > rtw_init_cmd_priv and rtw_init_evt_priv.
> > > > - Replace return types from sint to u32
> > > > - Replace local variable 'res' data type from sint to u32 in both the
> > > > functions.
> > > > Issue suggested by Coccinelle using ret.cocci.
> > >
> > > OK, this is a really good first step, with a nice log message as well.
> > > However, looking a bit further, there is more to do.
> > >
> > > We can observe that you changed the return type from signed to unsigned,
> > > and it is a little bit surprising that this is ok.
> > >
> > > Looking at the definitions of these functions it seems that they return
> > > error codes, _SUCCESS and _FAIL.  Often in the Linux kernel, success is
> > > indicated by 0 and failure is indicated by a negative number, so one may
> > > still be surprised by the change of type.
> >
> > I didn't  changed _SUCCESS value to 0 and _FAIL value to -ENOMEM. yet
> > again it complied. I want to confirm whether I have to change it
> > directly or there is function RTW_STATUS_CODE in os_dep/osdep_service.c,
> > which can be modified to make _SUCCESS and _FAIL compatible with rest of
> > the kernel
> 
> Please don't use _SUCCESS and _FAIL.  It doesn't really matter what these
> symbols are bound to.  Using them won't cause a compiler error.  The
> problem is that they are the wrong symbols.  Success should be indicated
> with 0 and failure, in the case of a memory allocation, should be
> indicated with -ENOMEM.  Other kinds of failures have other error codes
> (-EINVAL, -ENODEV, etc).
> 
> > >
> > > But this driver actually returns 1 for success and 0 for failure, so the
> > > code remains correctly functioning with the change of type.
> > >
> > > Still, these are not really ideal error values.  To be compatible with the
> > > rest of the kernel, success should be 0 and failure should be -ENOMEM,
> > > because these are memory allocating functions.  In that case the return
> > > type of these functions should be int, rather than u32.
> > >
> > > Finally, _rtw_init_evt_priv actually never returns _FAIL.  The memory
> > > allocation in this function bottoms out at kmalloc, which can fail, so
> > > there needs to be a check for this, and to return an -ENOMEM value.
> > >
> > > Checking on all of that leads to:
> > >
> > > void *_rtw_malloc(u32 sz)
> > > {
> > > 	return kmalloc(sz, in_interrupt() ? GFP_ATOMIC : GFP_KERNEL);
> > > }
> > >
> > > which is incorrect.  Each call site needs to be checked to determine
> > > whether it is in atomic context (accessible from an interrupt handler or
> > > with a lock held) and to use GFP_ATOMIC if that is the case and GFP_KERNEL
> > > otherwise.
> > >
> > > I would suggest to clean up everything up to the paragraph starting with
> > > "Finally" in a single series.  Then, if you want to work more on this
> > > driver, you can look for other uses of _SUCCESS and _FAIL and convert them
> > > to more appropriate return values, and convert calls to rtw_malloc to
> > > calls to kmalloc with the correct flag argument.  There is also a function
> > > rtw_zalloc that should be kzalloc with the same issues.  Actually, the
> > > file osdep_service.c hasa good set of unsuitable definitions that should
> > > be cleaned up.
> >
> > I sent it as a patch series. I will work on all unsuitable definitions
> > in osdep_service.c.
> 
> Actually, it is not really the definitions that need to be cleaned up.
> The uses of these functions need to be replaced by direct calls to the
> standard kernel functions, and then the functions in osdep_service.c
> should be dropped.

I will fix  all the functions backtracing from _rtw_init_evt_priv to
_rtw_malloc in another patch. Along with that, I wil fix rtw_zalloc,
which have same issues. 

Thanks

> 
> julia
> 
> >
> > >
> > > julia
> > >
> > > >
> > > > Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> > > > ---
> > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c | 24 ++++--------------------
> > > >  1 file changed, 4 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > index 91520ca3bbad..d8c362947dd4 100644
> > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > @@ -162,9 +162,9 @@ Caller and the rtw_cmd_thread can protect cmd_q by spin_lock.
> > > >  No irqsave is necessary.
> > > >  */
> > > >
> > > > -sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> > > > +u32 rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> > > >  {
> > > > -	sint res = _SUCCESS;
> > > > +	u32 res = _SUCCESS;
> > > >
> > > >  	init_completion(&pcmdpriv->cmd_queue_comp);
> > > >  	init_completion(&pcmdpriv->terminate_cmdthread_comp);
> > > > @@ -201,9 +201,9 @@ sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> > > >  }
> > > >
> > > >  static void c2h_wk_callback(_workitem *work);
> > > > -sint _rtw_init_evt_priv(struct evt_priv *pevtpriv)
> > > > +u32 rtw_init_evt_priv(struct evt_priv *pevtpriv)
> > > >  {
> > > > -	sint res = _SUCCESS;
> > > > +	u32 res = _SUCCESS;
> > > >
> > > >  	/* allocate DMA-able/Non-Page memory for cmd_buf and rsp_buf */
> > > >  	atomic_set(&pevtpriv->event_seq, 0);
> > > > @@ -295,22 +295,6 @@ struct	cmd_obj	*_rtw_dequeue_cmd(struct __queue *queue)
> > > >  	return obj;
> > > >  }
> > > >
> > > > -u32 rtw_init_cmd_priv(struct cmd_priv *pcmdpriv)
> > > > -{
> > > > -	u32 res;
> > > > -
> > > > -	res = _rtw_init_cmd_priv(pcmdpriv);
> > > > -	return res;
> > > > -}
> > > > -
> > > > -u32 rtw_init_evt_priv(struct	evt_priv *pevtpriv)
> > > > -{
> > > > -	int	res;
> > > > -
> > > > -	res = _rtw_init_evt_priv(pevtpriv);
> > > > -	return res;
> > > > -}
> > > > -
> > > >  void rtw_free_evt_priv(struct	evt_priv *pevtpriv)
> > > >  {
> > > >  	RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, ("rtw_free_evt_priv\n"));
> > > > --
> > > > 2.17.1
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > > > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > > > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > > > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20190314223416.3994-1-madhumithabiw%40gmail.com.
> > > > For more options, visit https://groups.google.com/d/optout.
> > > >
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.1903161419430.2487%40hadrien.
> For more options, visit https://groups.google.com/d/optout.

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: Remove functions and replace return types

[Julia Lawall]

On Sat, 16 Mar 2019, Madhumthia Prabakaran wrote:

> On Sat, Mar 16, 2019 at 02:23:03PM +0100, Julia Lawall wrote:
> >
> >
> > On Sat, 16 Mar 2019, Madhumthia Prabakaran wrote:
> >
> > > On Fri, Mar 15, 2019 at 07:50:57AM +0100, Julia Lawall wrote:
> > > >
> > > >
> > > > On Thu, 14 Mar 2019, Madhumitha Prabakaran wrote:
> > > >
> > > > > - Remove functions rtw_init_cmd_priv and rtw_init_evt_priv, as its only
> > > > > purpose is to call the other functions _rtw_init_cmd_priv and
> > > > > _rtw_init_evt_priv, respectively.
> > > > > - Rename functions from _rtw_init_cmd_priv and _rtw_init_evt_priv, to
> > > > > rtw_init_cmd_priv and rtw_init_evt_priv.
> > > > > - Replace return types from sint to u32
> > > > > - Replace local variable 'res' data type from sint to u32 in both the
> > > > > functions.
> > > > > Issue suggested by Coccinelle using ret.cocci.
> > > >
> > > > OK, this is a really good first step, with a nice log message as well.
> > > > However, looking a bit further, there is more to do.
> > > >
> > > > We can observe that you changed the return type from signed to unsigned,
> > > > and it is a little bit surprising that this is ok.
> > > >
> > > > Looking at the definitions of these functions it seems that they return
> > > > error codes, _SUCCESS and _FAIL.  Often in the Linux kernel, success is
> > > > indicated by 0 and failure is indicated by a negative number, so one may
> > > > still be surprised by the change of type.
> > >
> > > I didn't  changed _SUCCESS value to 0 and _FAIL value to -ENOMEM. yet
> > > again it complied. I want to confirm whether I have to change it
> > > directly or there is function RTW_STATUS_CODE in os_dep/osdep_service.c,
> > > which can be modified to make _SUCCESS and _FAIL compatible with rest of
> > > the kernel
> >
> > Please don't use _SUCCESS and _FAIL.  It doesn't really matter what these
> > symbols are bound to.  Using them won't cause a compiler error.  The
> > problem is that they are the wrong symbols.  Success should be indicated
> > with 0 and failure, in the case of a memory allocation, should be
> > indicated with -ENOMEM.  Other kinds of failures have other error codes
> > (-EINVAL, -ENODEV, etc).
> >
> > > >
> > > > But this driver actually returns 1 for success and 0 for failure, so the
> > > > code remains correctly functioning with the change of type.
> > > >
> > > > Still, these are not really ideal error values.  To be compatible with the
> > > > rest of the kernel, success should be 0 and failure should be -ENOMEM,
> > > > because these are memory allocating functions.  In that case the return
> > > > type of these functions should be int, rather than u32.
> > > >
> > > > Finally, _rtw_init_evt_priv actually never returns _FAIL.  The memory
> > > > allocation in this function bottoms out at kmalloc, which can fail, so
> > > > there needs to be a check for this, and to return an -ENOMEM value.
> > > >
> > > > Checking on all of that leads to:
> > > >
> > > > void *_rtw_malloc(u32 sz)
> > > > {
> > > > 	return kmalloc(sz, in_interrupt() ? GFP_ATOMIC : GFP_KERNEL);
> > > > }
> > > >
> > > > which is incorrect.  Each call site needs to be checked to determine
> > > > whether it is in atomic context (accessible from an interrupt handler or
> > > > with a lock held) and to use GFP_ATOMIC if that is the case and GFP_KERNEL
> > > > otherwise.
> > > >
> > > > I would suggest to clean up everything up to the paragraph starting with
> > > > "Finally" in a single series.  Then, if you want to work more on this
> > > > driver, you can look for other uses of _SUCCESS and _FAIL and convert them
> > > > to more appropriate return values, and convert calls to rtw_malloc to
> > > > calls to kmalloc with the correct flag argument.  There is also a function
> > > > rtw_zalloc that should be kzalloc with the same issues.  Actually, the
> > > > file osdep_service.c hasa good set of unsuitable definitions that should
> > > > be cleaned up.
> > >
> > > I sent it as a patch series. I will work on all unsuitable definitions
> > > in osdep_service.c.
> >
> > Actually, it is not really the definitions that need to be cleaned up.
> > The uses of these functions need to be replaced by direct calls to the
> > standard kernel functions, and then the functions in osdep_service.c
> > should be dropped.
>
> I will fix  all the functions backtracing from _rtw_init_evt_priv to
> _rtw_malloc in another patch. Along with that, I wil fix rtw_zalloc,
> which have same issues.

Don't go overobard.  I would suggest to fix each call site one at a time.
Or perhaps all call sites in a single file, if there aren't too many of
them.  But don't aim to send a single patch with 50 changes.

julia

>
> Thanks
>
> >
> > julia
> >
> > >
> > > >
> > > > julia
> > > >
> > > > >
> > > > > Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> > > > > ---
> > > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c | 24 ++++--------------------
> > > > >  1 file changed, 4 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > index 91520ca3bbad..d8c362947dd4 100644
> > > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > @@ -162,9 +162,9 @@ Caller and the rtw_cmd_thread can protect cmd_q by spin_lock.
> > > > >  No irqsave is necessary.
> > > > >  */
> > > > >
> > > > > -sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> > > > > +u32 rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> > > > >  {
> > > > > -	sint res = _SUCCESS;
> > > > > +	u32 res = _SUCCESS;
> > > > >
> > > > >  	init_completion(&pcmdpriv->cmd_queue_comp);
> > > > >  	init_completion(&pcmdpriv->terminate_cmdthread_comp);
> > > > > @@ -201,9 +201,9 @@ sint	_rtw_init_cmd_priv(struct	cmd_priv *pcmdpriv)
> > > > >  }
> > > > >
> > > > >  static void c2h_wk_callback(_workitem *work);
> > > > > -sint _rtw_init_evt_priv(struct evt_priv *pevtpriv)
> > > > > +u32 rtw_init_evt_priv(struct evt_priv *pevtpriv)
> > > > >  {
> > > > > -	sint res = _SUCCESS;
> > > > > +	u32 res = _SUCCESS;
> > > > >
> > > > >  	/* allocate DMA-able/Non-Page memory for cmd_buf and rsp_buf */
> > > > >  	atomic_set(&pevtpriv->event_seq, 0);
> > > > > @@ -295,22 +295,6 @@ struct	cmd_obj	*_rtw_dequeue_cmd(struct __queue *queue)
> > > > >  	return obj;
> > > > >  }
> > > > >
> > > > > -u32 rtw_init_cmd_priv(struct cmd_priv *pcmdpriv)
> > > > > -{
> > > > > -	u32 res;
> > > > > -
> > > > > -	res = _rtw_init_cmd_priv(pcmdpriv);
> > > > > -	return res;
> > > > > -}
> > > > > -
> > > > > -u32 rtw_init_evt_priv(struct	evt_priv *pevtpriv)
> > > > > -{
> > > > > -	int	res;
> > > > > -
> > > > > -	res = _rtw_init_evt_priv(pevtpriv);
> > > > > -	return res;
> > > > > -}
> > > > > -
> > > > >  void rtw_free_evt_priv(struct	evt_priv *pevtpriv)
> > > > >  {
> > > > >  	RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, ("rtw_free_evt_priv\n"));
> > > > > --
> > > > > 2.17.1
> > > > >
> > > > > --
> > > > > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > > > > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > > > > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > > > > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20190314223416.3994-1-madhumithabiw%40gmail.com.
> > > > > For more options, visit https://groups.google.com/d/optout.
> > > > >
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/alpine.DEB.2.21.1903161419430.2487%40hadrien.
> > For more options, visit https://groups.google.com/d/optout.
>