[Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

Removed the led_blink_hdl() function (declaration, definition, and
caller code) because it's useless. It only seems to check whether or not a
given pointer is NULL. There are other (simpler) means for that purpose.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
 drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
 3 files changed, 11 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index 0297fbad7bce..4c44dfd21514 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
 
 	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
 	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param), set_chplan_hdl) /*59*/
-	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), led_blink_hdl) /*60*/
 
 	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelSwitch_param), set_csa_hdl) /*61*/
 	GEN_MLME_EXT_HANDLER(sizeof(struct TDLSoption_param), tdls_hdl) /*62*/
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 440e22922106..6f2a0584f977 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -6189,15 +6189,6 @@ u8 set_chplan_hdl(struct adapter *padapter, unsigned char *pbuf)
 	return	H2C_SUCCESS;
 }
 
-u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf)
-{
-
-	if (!pbuf)
-		return H2C_PARAMETERS_ERROR;
-
-	return	H2C_SUCCESS;
-}
-
 u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf)
 {
 	return	H2C_REJECTED;
diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
index 5e6cf63956b8..472818c5fd83 100644
--- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
+++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
@@ -745,7 +745,6 @@ u8 chk_bmc_sleepq_hdl(struct adapter *padapter, unsigned char *pbuf);
 u8 tx_beacon_hdl(struct adapter *padapter, unsigned char *pbuf);
 u8 set_ch_hdl(struct adapter *padapter, u8 *pbuf);
 u8 set_chplan_hdl(struct adapter *padapter, unsigned char *pbuf);
-u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf);
 u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf);	/* Kurt: Handling DFS channel switch announcement ie. */
 u8 tdls_hdl(struct adapter *padapter, unsigned char *pbuf);
 u8 run_in_thread_hdl(struct adapter *padapter, u8 *pbuf);
-- 
2.31.1

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Julia Lawall]

On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:

> Removed the led_blink_hdl() function (declaration, definition, and
> caller code) because it's useless. It only seems to check whether or not a
> given pointer is NULL. There are other (simpler) means for that purpose.
>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
>  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
>  3 files changed, 11 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index 0297fbad7bce..4c44dfd21514 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
>
>  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
>  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param), set_chplan_hdl) /*59*/
> -	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), led_blink_hdl) /*60*/

This is worrisome.  Doyou fully understand the impact of this?  If not,
the change is probably not a good idea.

julia

>
>  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelSwitch_param), set_csa_hdl) /*61*/
>  	GEN_MLME_EXT_HANDLER(sizeof(struct TDLSoption_param), tdls_hdl) /*62*/
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 440e22922106..6f2a0584f977 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -6189,15 +6189,6 @@ u8 set_chplan_hdl(struct adapter *padapter, unsigned char *pbuf)
>  	return	H2C_SUCCESS;
>  }
>
> -u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf)
> -{
> -
> -	if (!pbuf)
> -		return H2C_PARAMETERS_ERROR;
> -
> -	return	H2C_SUCCESS;
> -}
> -
>  u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf)
>  {
>  	return	H2C_REJECTED;
> diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> index 5e6cf63956b8..472818c5fd83 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> @@ -745,7 +745,6 @@ u8 chk_bmc_sleepq_hdl(struct adapter *padapter, unsigned char *pbuf);
>  u8 tx_beacon_hdl(struct adapter *padapter, unsigned char *pbuf);
>  u8 set_ch_hdl(struct adapter *padapter, u8 *pbuf);
>  u8 set_chplan_hdl(struct adapter *padapter, unsigned char *pbuf);
> -u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf);
>  u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf);	/* Kurt: Handling DFS channel switch announcement ie. */
>  u8 tdls_hdl(struct adapter *padapter, unsigned char *pbuf);
>  u8 run_in_thread_hdl(struct adapter *padapter, u8 *pbuf);
> --
> 2.31.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 view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20210413155908.8691-1-fmdefrancesco%40gmail.com.
>

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall wrote:
> On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > Removed the led_blink_hdl() function (declaration, definition, and
> > caller code) because it's useless. It only seems to check whether or
> > not a given pointer is NULL. There are other (simpler) means for that
> > purpose.
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> >  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
> >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
> >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> >  3 files changed, 11 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > 0297fbad7bce..4c44dfd21514 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
> > 
> >  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> >  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param),
> >  	set_chplan_hdl) /*59*/> 
> > -	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), 
led_blink_hdl)
> > /*60*/
> This is worrisome.  Doyou fully understand the impact of this?  If not,
> the change is probably not a good idea.
>
This is that macro definition:

#define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},

struct C2HEvent_Header {

#ifdef __LITTLE_ENDIAN

        unsigned int len:16;
        unsigned int ID:8;
        unsigned int seq:8;
#else
        unsigned int seq:8;
        unsigned int ID:8;
        unsigned int len:16;
#endif
        unsigned int rsvd;
};

It's a bit convoluted with regard to my experience. Probably I don't 
understand it fully, but it seems to me to not having effects to the code 
where I removed its use within core/rtw_cmd.c.

What am I missing?

Thanks for your kind help,

Fabio
> 
> julia
> 
> >  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelSwitch_param),
> >  	set_csa_hdl) /*61*/ GEN_MLME_EXT_HANDLER(sizeof(struct
> >  	TDLSoption_param), tdls_hdl) /*62*/> 
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c index
> > 440e22922106..6f2a0584f977 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> > @@ -6189,15 +6189,6 @@ u8 set_chplan_hdl(struct adapter *padapter,
> > unsigned char *pbuf)> 
> >  	return	H2C_SUCCESS;
> >  
> >  }
> > 
> > -u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf)
> > -{
> > -
> > -	if (!pbuf)
> > -		return H2C_PARAMETERS_ERROR;
> > -
> > -	return	H2C_SUCCESS;
> > -}
> > -
> > 
> >  u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf)
> >  {
> >  
> >  	return	H2C_REJECTED;
> > 
> > diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> > b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h index
> > 5e6cf63956b8..472818c5fd83 100644
> > --- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> > +++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> > @@ -745,7 +745,6 @@ u8 chk_bmc_sleepq_hdl(struct adapter *padapter,
> > unsigned char *pbuf);> 
> >  u8 tx_beacon_hdl(struct adapter *padapter, unsigned char *pbuf);
> >  u8 set_ch_hdl(struct adapter *padapter, u8 *pbuf);
> >  u8 set_chplan_hdl(struct adapter *padapter, unsigned char *pbuf);
> > 
> > -u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf);
> > 
> >  u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf);	
/*
> >  Kurt: Handling DFS channel switch announcement ie. */ u8
> >  tdls_hdl(struct adapter *padapter, unsigned char *pbuf);
> >  u8 run_in_thread_hdl(struct adapter *padapter, u8 *pbuf);
> > 
> > --
> > 2.31.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 view this discussion
> > on the web visit
> > https://groups.google.com/d/msgid/outreachy-kernel/20210413155908.8691
> > -1-fmdefrancesco%40gmail.com.

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Julia Lawall]

On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:

> On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall wrote:
> > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > Removed the led_blink_hdl() function (declaration, definition, and
> > > caller code) because it's useless. It only seems to check whether or
> > > not a given pointer is NULL. There are other (simpler) means for that
> > > purpose.
> > >
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > >
> > >  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
> > >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
> > >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > >  3 files changed, 11 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > 0297fbad7bce..4c44dfd21514 100644
> > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
> > >
> > >  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> > >  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param),
> > >  	set_chplan_hdl) /*59*/>
> > > -	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param),
> led_blink_hdl)
> > > /*60*/
> > This is worrisome.  Doyou fully understand the impact of this?  If not,
> > the change is probably not a good idea.
> >
> This is that macro definition:
>
> #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
>
> struct C2HEvent_Header {
>
> #ifdef __LITTLE_ENDIAN
>
>         unsigned int len:16;
>         unsigned int ID:8;
>         unsigned int seq:8;
> #else
>         unsigned int seq:8;
>         unsigned int ID:8;
>         unsigned int len:16;
> #endif
>         unsigned int rsvd;
> };
>
> It's a bit convoluted with regard to my experience. Probably I don't
> understand it fully, but it seems to me to not having effects to the code
> where I removed its use within core/rtw_cmd.c.
>
> What am I missing?

It seems that the function is being put into an array.  Probably someone
expects to find it there.  Probably you have shifted all of the functions
that come afterwards back one slot so that they are all in the wrong
places.

julia

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

On Tuesday, April 13, 2021 6:27:17 PM CEST Julia Lawall wrote:
> On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall wrote:
> > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > Removed the led_blink_hdl() function (declaration, definition, and
> > > > caller code) because it's useless. It only seems to check whether
> > > > or
> > > > not a given pointer is NULL. There are other (simpler) means for
> > > > that
> > > > purpose.
> > > > 
> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
> > > >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
> > > >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > >  3 files changed, 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > 0297fbad7bce..4c44dfd21514 100644
> > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
> > > > 
> > > >  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> > > >  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param),
> > > >  	set_chplan_hdl) /*59*/>
> > > > 
> > > > -	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param),
> > 
> > led_blink_hdl)
> > 
> > > > /*60*/
> > > 
> > > This is worrisome.  Doyou fully understand the impact of this?  If
> > > not,
> > > the change is probably not a good idea.
> > 
> > This is that macro definition:
> > 
> > #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
> > 
> > struct C2HEvent_Header {
> > 
> > #ifdef __LITTLE_ENDIAN
> > 
> >         unsigned int len:16;
> >         unsigned int ID:8;
> >         unsigned int seq:8;
> > 
> > #else
> > 
> >         unsigned int seq:8;
> >         unsigned int ID:8;
> >         unsigned int len:16;
> > 
> > #endif
> > 
> >         unsigned int rsvd;
> > 
> > };
> > 
> > It's a bit convoluted with regard to my experience. Probably I don't
> > understand it fully, but it seems to me to not having effects to the
> > code where I removed its use within core/rtw_cmd.c.
> > 
> > What am I missing?
> 
> It seems that the function is being put into an array.  Probably someone
> expects to find it there.  Probably you have shifted all of the functions
> that come afterwards back one slot so that they are all in the wrong
> places.
> 
> julia
>
Thanks for your explanation. Obviously this implies that the function 
cannot be removed, unless one fill the slot that is deleted by to not 
calling this macro at the right moment. 

I also suppose that providing a function pointer with a NULL value wouldn't 
work either.

OK, h2c_msg_hdl() cannot be deleted.

Thanks,

Fabio
  

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Dan Carpenter]

On Tue, Apr 13, 2021 at 06:47:06PM +0200, Fabio M. De Francesco wrote:
> On Tuesday, April 13, 2021 6:27:17 PM CEST Julia Lawall wrote:
> > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall wrote:
> > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > Removed the led_blink_hdl() function (declaration, definition, and
> > > > > caller code) because it's useless. It only seems to check whether
> > > > > or
> > > > > not a given pointer is NULL. There are other (simpler) means for
> > > > > that
> > > > > purpose.
> > > > > 
> > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > ---
> > > > > 
> > > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
> > > > >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
> > > > >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > > >  3 files changed, 11 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > > 0297fbad7bce..4c44dfd21514 100644
> > > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
> > > > > 
> > > > >  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> > > > >  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param),
> > > > >  	set_chplan_hdl) /*59*/>
> > > > > 
> > > > > -	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param),
> > > 
> > > led_blink_hdl)
> > > 
> > > > > /*60*/
> > > > 
> > > > This is worrisome.  Doyou fully understand the impact of this?  If
> > > > not,
> > > > the change is probably not a good idea.
> > > 
> > > This is that macro definition:
> > > 
> > > #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
> > > 
> > > struct C2HEvent_Header {
> > > 
> > > #ifdef __LITTLE_ENDIAN
> > > 
> > >         unsigned int len:16;
> > >         unsigned int ID:8;
> > >         unsigned int seq:8;
> > > 
> > > #else
> > > 
> > >         unsigned int seq:8;
> > >         unsigned int ID:8;
> > >         unsigned int len:16;
> > > 
> > > #endif
> > > 
> > >         unsigned int rsvd;
> > > 
> > > };
> > > 
> > > It's a bit convoluted with regard to my experience. Probably I don't
> > > understand it fully, but it seems to me to not having effects to the
> > > code where I removed its use within core/rtw_cmd.c.
> > > 
> > > What am I missing?
> > 
> > It seems that the function is being put into an array.  Probably someone
> > expects to find it there.  Probably you have shifted all of the functions
> > that come afterwards back one slot so that they are all in the wrong
> > places.
> > 
> > julia
> >
> Thanks for your explanation. Obviously this implies that the function 
> cannot be removed, unless one fill the slot that is deleted by to not 
> calling this macro at the right moment. 
> 
> I also suppose that providing a function pointer with a NULL value wouldn't 
> work either.

It would work.  That array is full of NULL function pointers.

regards,
dan carpenter

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

On Tuesday, April 13, 2021 8:20:50 PM CEST Dan Carpenter wrote:
> On Tue, Apr 13, 2021 at 06:47:06PM +0200, Fabio M. De Francesco wrote:
> > On Tuesday, April 13, 2021 6:27:17 PM CEST Julia Lawall wrote:
> > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall wrote:
> > > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > > Removed the led_blink_hdl() function (declaration, definition,
> > > > > > and
> > > > > > caller code) because it's useless. It only seems to check
> > > > > > whether
> > > > > > or
> > > > > > not a given pointer is NULL. There are other (simpler) means
> > > > > > for
> > > > > > that
> > > > > > purpose.
> > > > > > 
> > > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
> > > > > >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
> > > > > >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > > > >  3 files changed, 11 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > > > 0297fbad7bce..4c44dfd21514 100644
> > > > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
> > > > > > 
> > > > > >  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> > > > > >  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param),
> > > > > >  	set_chplan_hdl) /*59*/>
> > > > > > 
> > > > > > -	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param),
> > > > 
> > > > led_blink_hdl)
> > > > 
> > > > > > /*60*/
> > > > > 
> > > > > This is worrisome.  Doyou fully understand the impact of this? 
> > > > > If
> > > > > not,
> > > > > the change is probably not a good idea.
> > > > 
> > > > This is that macro definition:
> > > > 
> > > > #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
> > > > 
> > > > struct C2HEvent_Header {
> > > > 
> > > > #ifdef __LITTLE_ENDIAN
> > > > 
> > > >         unsigned int len:16;
> > > >         unsigned int ID:8;
> > > >         unsigned int seq:8;
> > > > 
> > > > #else
> > > > 
> > > >         unsigned int seq:8;
> > > >         unsigned int ID:8;
> > > >         unsigned int len:16;
> > > > 
> > > > #endif
> > > > 
> > > >         unsigned int rsvd;
> > > > 
> > > > };
> > > > 
> > > > It's a bit convoluted with regard to my experience. Probably I
> > > > don't
> > > > understand it fully, but it seems to me to not having effects to
> > > > the
> > > > code where I removed its use within core/rtw_cmd.c.
> > > > 
> > > > What am I missing?
> > > 
> > > It seems that the function is being put into an array.  Probably
> > > someone
> > > expects to find it there.  Probably you have shifted all of the
> > > functions that come afterwards back one slot so that they are all in
> > > the wrong places.
> > > 
> > > julia
> > 
> > Thanks for your explanation. Obviously this implies that the function
> > cannot be removed, unless one fill the slot that is deleted by to not
> > calling this macro at the right moment.
> > 
> > I also suppose that providing a function pointer with a NULL value
> > wouldn't work either.
> 
> It would work.  That array is full of NULL function pointers.
> 
Interesting, thanks.

I'm going to remove that function and replace its name in the macro with a 
NULL function pointer.

I couldn't believe it would work when I wrote about that.

Thanks a lot,

Fabio
>
> regards,
> dan carpenter

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Julia Lawall]

On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:

> On Tuesday, April 13, 2021 8:20:50 PM CEST Dan Carpenter wrote:
> > On Tue, Apr 13, 2021 at 06:47:06PM +0200, Fabio M. De Francesco wrote:
> > > On Tuesday, April 13, 2021 6:27:17 PM CEST Julia Lawall wrote:
> > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall wrote:
> > > > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > > > Removed the led_blink_hdl() function (declaration, definition,
> > > > > > > and
> > > > > > > caller code) because it's useless. It only seems to check
> > > > > > > whether
> > > > > > > or
> > > > > > > not a given pointer is NULL. There are other (simpler) means
> > > > > > > for
> > > > > > > that
> > > > > > > purpose.
> > > > > > >
> > > > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > > > > ---
> > > > > > >
> > > > > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
> > > > > > >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
> > > > > > >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > > > > >  3 files changed, 11 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > > > > 0297fbad7bce..4c44dfd21514 100644
> > > > > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
> > > > > > >
> > > > > > >  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> > > > > > >  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param),
> > > > > > >  	set_chplan_hdl) /*59*/>
> > > > > > >
> > > > > > > -	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param),
> > > > >
> > > > > led_blink_hdl)
> > > > >
> > > > > > > /*60*/
> > > > > >
> > > > > > This is worrisome.  Doyou fully understand the impact of this?
> > > > > > If
> > > > > > not,
> > > > > > the change is probably not a good idea.
> > > > >
> > > > > This is that macro definition:
> > > > >
> > > > > #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
> > > > >
> > > > > struct C2HEvent_Header {
> > > > >
> > > > > #ifdef __LITTLE_ENDIAN
> > > > >
> > > > >         unsigned int len:16;
> > > > >         unsigned int ID:8;
> > > > >         unsigned int seq:8;
> > > > >
> > > > > #else
> > > > >
> > > > >         unsigned int seq:8;
> > > > >         unsigned int ID:8;
> > > > >         unsigned int len:16;
> > > > >
> > > > > #endif
> > > > >
> > > > >         unsigned int rsvd;
> > > > >
> > > > > };
> > > > >
> > > > > It's a bit convoluted with regard to my experience. Probably I
> > > > > don't
> > > > > understand it fully, but it seems to me to not having effects to
> > > > > the
> > > > > code where I removed its use within core/rtw_cmd.c.
> > > > >
> > > > > What am I missing?
> > > >
> > > > It seems that the function is being put into an array.  Probably
> > > > someone
> > > > expects to find it there.  Probably you have shifted all of the
> > > > functions that come afterwards back one slot so that they are all in
> > > > the wrong places.
> > > >
> > > > julia
> > >
> > > Thanks for your explanation. Obviously this implies that the function
> > > cannot be removed, unless one fill the slot that is deleted by to not
> > > calling this macro at the right moment.
> > >
> > > I also suppose that providing a function pointer with a NULL value
> > > wouldn't work either.
> >
> > It would work.  That array is full of NULL function pointers.
> >
> Interesting, thanks.
>
> I'm going to remove that function and replace its name in the macro with a
> NULL function pointer.
>
> I couldn't believe it would work when I wrote about that.

Have you checked that a value of NULL in that place is going to have the
same effect as the function?

julia

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

On Tuesday, April 13, 2021 8:57:20 PM CEST Julia Lawall wrote:
> On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > On Tuesday, April 13, 2021 8:20:50 PM CEST Dan Carpenter wrote:
> > > On Tue, Apr 13, 2021 at 06:47:06PM +0200, Fabio M. De Francesco 
wrote:
> > > > On Tuesday, April 13, 2021 6:27:17 PM CEST Julia Lawall wrote:
> > > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > > On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall wrote:
> > > > > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > > > > Removed the led_blink_hdl() function (declaration,
> > > > > > > > definition,
> > > > > > > > and
> > > > > > > > caller code) because it's useless. It only seems to check
> > > > > > > > whether
> > > > > > > > or
> > > > > > > > not a given pointer is NULL. There are other (simpler)
> > > > > > > > means
> > > > > > > > for
> > > > > > > > that
> > > > > > > > purpose.
> > > > > > > > 
> > > > > > > > Signed-off-by: Fabio M. De Francesco
> > > > > > > > <fmdefrancesco@gmail.com>
> > > > > > > > ---
> > > > > > > > 
> > > > > > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
> > > > > > > >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9
> > > > > > > >  ---------
> > > > > > > >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > > > > > >  3 files changed, 11 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > > > > > 0297fbad7bce..4c44dfd21514 100644
> > > > > > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
> > > > > > > > 
> > > > > > > >  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> > > > > > > >  	GEN_MLME_EXT_HANDLER(sizeof(struct 
SetChannelPlan_param),
> > > > > > > >  	set_chplan_hdl) /*59*/>
> > > > > > > > 
> > > > > > > > -	GEN_MLME_EXT_HANDLER(sizeof(struct 
LedBlink_param),
> > > > > > 
> > > > > > led_blink_hdl)
> > > > > > 
> > > > > > > > /*60*/
> > > > > > > 
> > > > > > > This is worrisome.  Doyou fully understand the impact of
> > > > > > > this?
> > > > > > > If
> > > > > > > not,
> > > > > > > the change is probably not a good idea.
> > > > > > 
> > > > > > This is that macro definition:
> > > > > > 
> > > > > > #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
> > > > > > 
> > > > > > struct C2HEvent_Header {
> > > > > > 
> > > > > > #ifdef __LITTLE_ENDIAN
> > > > > > 
> > > > > >         unsigned int len:16;
> > > > > >         unsigned int ID:8;
> > > > > >         unsigned int seq:8;
> > > > > > 
> > > > > > #else
> > > > > > 
> > > > > >         unsigned int seq:8;
> > > > > >         unsigned int ID:8;
> > > > > >         unsigned int len:16;
> > > > > > 
> > > > > > #endif
> > > > > > 
> > > > > >         unsigned int rsvd;
> > > > > > 
> > > > > > };
> > > > > > 
> > > > > > It's a bit convoluted with regard to my experience. Probably I
> > > > > > don't
> > > > > > understand it fully, but it seems to me to not having effects
> > > > > > to
> > > > > > the
> > > > > > code where I removed its use within core/rtw_cmd.c.
> > > > > > 
> > > > > > What am I missing?
> > > > > 
> > > > > It seems that the function is being put into an array.  Probably
> > > > > someone
> > > > > expects to find it there.  Probably you have shifted all of the
> > > > > functions that come afterwards back one slot so that they are all
> > > > > in
> > > > > the wrong places.
> > > > > 
> > > > > julia
> > > > 
> > > > Thanks for your explanation. Obviously this implies that the
> > > > function
> > > > cannot be removed, unless one fill the slot that is deleted by to
> > > > not
> > > > calling this macro at the right moment.
> > > > 
> > > > I also suppose that providing a function pointer with a NULL value
> > > > wouldn't work either.
> > > 
> > > It would work.  That array is full of NULL function pointers.
> > 
> > Interesting, thanks.
> > 
> > I'm going to remove that function and replace its name in the macro
> > with a NULL function pointer.
> > 
> > I couldn't believe it would work when I wrote about that.
> 
> Have you checked that a value of NULL in that place is going to have the
> same effect as the function?
>
No, I have not, but perhaps is not relevant.

I want to give to the macro the name of an empty function that I define in 
the same header where there the prototype of led_blink_hdl() is defined 
now: something like "u8 empty_function { return 0; }"

Can it work
What do you think about it?

Fabio
> 
> julia

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Julia Lawall]

On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:

> On Tuesday, April 13, 2021 8:57:20 PM CEST Julia Lawall wrote:
> > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > On Tuesday, April 13, 2021 8:20:50 PM CEST Dan Carpenter wrote:
> > > > On Tue, Apr 13, 2021 at 06:47:06PM +0200, Fabio M. De Francesco
> wrote:
> > > > > On Tuesday, April 13, 2021 6:27:17 PM CEST Julia Lawall wrote:
> > > > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > > > On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall wrote:
> > > > > > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > > > > > Removed the led_blink_hdl() function (declaration,
> > > > > > > > > definition,
> > > > > > > > > and
> > > > > > > > > caller code) because it's useless. It only seems to check
> > > > > > > > > whether
> > > > > > > > > or
> > > > > > > > > not a given pointer is NULL. There are other (simpler)
> > > > > > > > > means
> > > > > > > > > for
> > > > > > > > > that
> > > > > > > > > purpose.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Fabio M. De Francesco
> > > > > > > > > <fmdefrancesco@gmail.com>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
> > > > > > > > >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9
> > > > > > > > >  ---------
> > > > > > > > >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > > > > > > >  3 files changed, 11 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > > > > > > 0297fbad7bce..4c44dfd21514 100644
> > > > > > > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] = {
> > > > > > > > >
> > > > > > > > >  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> > > > > > > > >  	GEN_MLME_EXT_HANDLER(sizeof(struct
> SetChannelPlan_param),
> > > > > > > > >  	set_chplan_hdl) /*59*/>
> > > > > > > > >
> > > > > > > > > -	GEN_MLME_EXT_HANDLER(sizeof(struct
> LedBlink_param),
> > > > > > >
> > > > > > > led_blink_hdl)
> > > > > > >
> > > > > > > > > /*60*/
> > > > > > > >
> > > > > > > > This is worrisome.  Doyou fully understand the impact of
> > > > > > > > this?
> > > > > > > > If
> > > > > > > > not,
> > > > > > > > the change is probably not a good idea.
> > > > > > >
> > > > > > > This is that macro definition:
> > > > > > >
> > > > > > > #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
> > > > > > >
> > > > > > > struct C2HEvent_Header {
> > > > > > >
> > > > > > > #ifdef __LITTLE_ENDIAN
> > > > > > >
> > > > > > >         unsigned int len:16;
> > > > > > >         unsigned int ID:8;
> > > > > > >         unsigned int seq:8;
> > > > > > >
> > > > > > > #else
> > > > > > >
> > > > > > >         unsigned int seq:8;
> > > > > > >         unsigned int ID:8;
> > > > > > >         unsigned int len:16;
> > > > > > >
> > > > > > > #endif
> > > > > > >
> > > > > > >         unsigned int rsvd;
> > > > > > >
> > > > > > > };
> > > > > > >
> > > > > > > It's a bit convoluted with regard to my experience. Probably I
> > > > > > > don't
> > > > > > > understand it fully, but it seems to me to not having effects
> > > > > > > to
> > > > > > > the
> > > > > > > code where I removed its use within core/rtw_cmd.c.
> > > > > > >
> > > > > > > What am I missing?
> > > > > >
> > > > > > It seems that the function is being put into an array.  Probably
> > > > > > someone
> > > > > > expects to find it there.  Probably you have shifted all of the
> > > > > > functions that come afterwards back one slot so that they are all
> > > > > > in
> > > > > > the wrong places.
> > > > > >
> > > > > > julia
> > > > >
> > > > > Thanks for your explanation. Obviously this implies that the
> > > > > function
> > > > > cannot be removed, unless one fill the slot that is deleted by to
> > > > > not
> > > > > calling this macro at the right moment.
> > > > >
> > > > > I also suppose that providing a function pointer with a NULL value
> > > > > wouldn't work either.
> > > >
> > > > It would work.  That array is full of NULL function pointers.
> > >
> > > Interesting, thanks.
> > >
> > > I'm going to remove that function and replace its name in the macro
> > > with a NULL function pointer.
> > >
> > > I couldn't believe it would work when I wrote about that.
> >
> > Have you checked that a value of NULL in that place is going to have the
> > same effect as the function?
> >
> No, I have not, but perhaps is not relevant.
>
> I want to give to the macro the name of an empty function that I define in
> the same header where there the prototype of led_blink_hdl() is defined
> now: something like "u8 empty_function { return 0; }"
>
> Can it work
> What do you think about it?

The previous function didn't return 0.  It returned something else.

To do anything this, you have to find where it is called and what result
the call site expects.  If you don't have that information, it does not
seem safe to modify the function.

julia


>
> Fabio
> >
> > julia
>
>
>
>
>

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

On Tuesday, April 13, 2021 9:25:05 PM CEST Julia Lawall wrote:
> On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > On Tuesday, April 13, 2021 8:57:20 PM CEST Julia Lawall wrote:
> > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > On Tuesday, April 13, 2021 8:20:50 PM CEST Dan Carpenter wrote:
> > > > > On Tue, Apr 13, 2021 at 06:47:06PM +0200, Fabio M. De Francesco
> > 
> > wrote:
> > > > > > On Tuesday, April 13, 2021 6:27:17 PM CEST Julia Lawall wrote:
> > > > > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > > > > On Tuesday, April 13, 2021 6:04:16 PM CEST Julia Lawall 
wrote:
> > > > > > > > > On Tue, 13 Apr 2021, Fabio M. De Francesco wrote:
> > > > > > > > > > Removed the led_blink_hdl() function (declaration,
> > > > > > > > > > definition,
> > > > > > > > > > and
> > > > > > > > > > caller code) because it's useless. It only seems to
> > > > > > > > > > check
> > > > > > > > > > whether
> > > > > > > > > > or
> > > > > > > > > > not a given pointer is NULL. There are other (simpler)
> > > > > > > > > > means
> > > > > > > > > > for
> > > > > > > > > > that
> > > > > > > > > > purpose.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Fabio M. De Francesco
> > > > > > > > > > <fmdefrancesco@gmail.com>
> > > > > > > > > > ---
> > > > > > > > > > 
> > > > > > > > > >  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
> > > > > > > > > >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9
> > > > > > > > > >  ---------
> > > > > > > > > >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
> > > > > > > > > >  3 files changed, 11 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > > > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > > > > > > > > > 0297fbad7bce..4c44dfd21514 100644
> > > > > > > > > > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > > > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > > > > > > > > > @@ -150,7 +150,6 @@ static struct cmd_hdl wlancmds[] =
> > > > > > > > > > {
> > > > > > > > > > 
> > > > > > > > > >  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> > > > > > > > > >  	GEN_MLME_EXT_HANDLER(sizeof(struct
> > 
> > SetChannelPlan_param),
> > 
> > > > > > > > > >  	set_chplan_hdl) /*59*/>
> > > > > > > > > > 
> > > > > > > > > > -	GEN_MLME_EXT_HANDLER(sizeof(struct
> > 
> > LedBlink_param),
> > 
> > > > > > > > led_blink_hdl)
> > > > > > > > 
> > > > > > > > > > /*60*/
> > > > > > > > > 
> > > > > > > > > This is worrisome.  Doyou fully understand the impact of
> > > > > > > > > this?
> > > > > > > > > If
> > > > > > > > > not,
> > > > > > > > > the change is probably not a good idea.
> > > > > > > > 
> > > > > > > > This is that macro definition:
> > > > > > > > 
> > > > > > > > #define GEN_MLME_EXT_HANDLER(size, cmd) {size, cmd},
> > > > > > > > 
> > > > > > > > struct C2HEvent_Header {
> > > > > > > > 
> > > > > > > > #ifdef __LITTLE_ENDIAN
> > > > > > > > 
> > > > > > > >         unsigned int len:16;
> > > > > > > >         unsigned int ID:8;
> > > > > > > >         unsigned int seq:8;
> > > > > > > > 
> > > > > > > > #else
> > > > > > > > 
> > > > > > > >         unsigned int seq:8;
> > > > > > > >         unsigned int ID:8;
> > > > > > > >         unsigned int len:16;
> > > > > > > > 
> > > > > > > > #endif
> > > > > > > > 
> > > > > > > >         unsigned int rsvd;
> > > > > > > > 
> > > > > > > > };
> > > > > > > > 
> > > > > > > > It's a bit convoluted with regard to my experience.
> > > > > > > > Probably I
> > > > > > > > don't
> > > > > > > > understand it fully, but it seems to me to not having
> > > > > > > > effects
> > > > > > > > to
> > > > > > > > the
> > > > > > > > code where I removed its use within core/rtw_cmd.c.
> > > > > > > > 
> > > > > > > > What am I missing?
> > > > > > > 
> > > > > > > It seems that the function is being put into an array. 
> > > > > > > Probably
> > > > > > > someone
> > > > > > > expects to find it there.  Probably you have shifted all of
> > > > > > > the
> > > > > > > functions that come afterwards back one slot so that they are
> > > > > > > all
> > > > > > > in
> > > > > > > the wrong places.
> > > > > > > 
> > > > > > > julia
> > > > > > 
> > > > > > Thanks for your explanation. Obviously this implies that the
> > > > > > function
> > > > > > cannot be removed, unless one fill the slot that is deleted by
> > > > > > to
> > > > > > not
> > > > > > calling this macro at the right moment.
> > > > > > 
> > > > > > I also suppose that providing a function pointer with a NULL
> > > > > > value
> > > > > > wouldn't work either.
> > > > > 
> > > > > It would work.  That array is full of NULL function pointers.
> > > > 
> > > > Interesting, thanks.
> > > > 
> > > > I'm going to remove that function and replace its name in the macro
> > > > with a NULL function pointer.
> > > > 
> > > > I couldn't believe it would work when I wrote about that.
> > > 
> > > Have you checked that a value of NULL in that place is going to have
> > > the
> > > same effect as the function?
> > 
> > No, I have not, but perhaps is not relevant.
> > 
> > I want to give to the macro the name of an empty function that I define
> > in the same header where there the prototype of led_blink_hdl() is
> > defined now: something like "u8 empty_function { return 0; }"
> > 
> > Can it work
> > What do you think about it?
> 
> The previous function didn't return 0.  It returned something else.
> 
> To do anything this, you have to find where it is called and what result
> the call site expects.  If you don't have that information, it does not
> seem safe to modify the function.
> 
> julia
> 
> > Fabio
> > 
> > > julia
>
OK, let's summarize:

1) The driver doesn't call that function from anywhere else than the macro.
2) You have explained that the macro add its symbol to a slot in an array 
that would shift all the subsequent elements down if that macro is not used 
exactly in the line where it is.
3) Dan Carpenter said that that array is full of null functions (or empty 
slots?).

Unless that function is called anonymously dereferencing its address from 
the position it occupies in the array, I'm not able to see what else means 
can any caller use.

I know I have much less experience than you with C: what can go wrong?

Thanks for your time,

Fabio

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Matthew Wilcox]

On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco wrote:
> 1) The driver doesn't call that function from anywhere else than the macro.
> 2) You have explained that the macro add its symbol to a slot in an array 
> that would shift all the subsequent elements down if that macro is not used 
> exactly in the line where it is.
> 3) Dan Carpenter said that that array is full of null functions (or empty 
> slots?).
> 
> Unless that function is called anonymously dereferencing its address from 
> the position it occupies in the array, I'm not able to see what else means 
> can any caller use.
> 
> I know I have much less experience than you with C: what can go wrong?

Here's where the driver calls that function:

$ git grep wlancmds drivers/staging/rtl8723bs/
drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl wlancmds[] = {
drivers/staging/rtl8723bs/core/rtw_cmd.c:               if (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
drivers/staging/rtl8723bs/core/rtw_cmd.c:                       cmd_hdl = wlancmds[pcmd->cmdcode].h2cfuns;

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco wrote:
> > 1) The driver doesn't call that function from anywhere else than the
> > macro. 2) You have explained that the macro add its symbol to a slot
> > in an array that would shift all the subsequent elements down if that
> > macro is not used exactly in the line where it is.
> > 3) Dan Carpenter said that that array is full of null functions (or
> > empty slots?).
> > 
> > Unless that function is called anonymously dereferencing its address
> > from the position it occupies in the array, I'm not able to see what
> > else means can any caller use.
> > 
> > I know I have much less experience than you with C: what can go wrong?
> 
> Here's where the driver calls that function:
> 
> $ git grep wlancmds drivers/staging/rtl8723bs/
> drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl wlancmds[]
> = { drivers/staging/rtl8723bs/core/rtw_cmd.c:               if
> (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> drivers/staging/rtl8723bs/core/rtw_cmd.c:                       cmd_hdl
> = wlancmds[pcmd->cmdcode].h2cfuns;
>
OK, I had imagined an anonymous call from its location in the array (as I 
wrote in the last phrase of my message). However, I thought that it could 
have been an improbable possibility, not a real one.

Linux uses a lot of interesting ideas that newcomers like me should learn. 
Things here are trickier than they appear at first sight.

Thanks,

Fabio

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Dan Carpenter]

On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco wrote:
> On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco wrote:
> > > 1) The driver doesn't call that function from anywhere else than the
> > > macro. 2) You have explained that the macro add its symbol to a slot
> > > in an array that would shift all the subsequent elements down if that
> > > macro is not used exactly in the line where it is.
> > > 3) Dan Carpenter said that that array is full of null functions (or
> > > empty slots?).
> > > 
> > > Unless that function is called anonymously dereferencing its address
> > > from the position it occupies in the array, I'm not able to see what
> > > else means can any caller use.
> > > 
> > > I know I have much less experience than you with C: what can go wrong?
> > 
> > Here's where the driver calls that function:
> > 
> > $ git grep wlancmds drivers/staging/rtl8723bs/
> > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl wlancmds[]
> > = { drivers/staging/rtl8723bs/core/rtw_cmd.c:               if
> > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > drivers/staging/rtl8723bs/core/rtw_cmd.c:                       cmd_hdl
> > = wlancmds[pcmd->cmdcode].h2cfuns;
> >
> OK, I had imagined an anonymous call from its location in the array (as I 
> wrote in the last phrase of my message). However, I thought that it could 
> have been an improbable possibility, not a real one.
> 
> Linux uses a lot of interesting ideas that newcomers like me should learn. 
> Things here are trickier than they appear at first sight.

One trick would be to build the Smatch cross function database.

https://www.spinics.net/lists/smatch/msg00568.html

Then you could do:

$ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
file | caller | function | type | parameter | key | value |
drivers/staging/rtl8723bs/core/rtw_cmd.c |       rtw_cmd_thread | rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |                 | uchar(*)(struct adapter*, uchar*)
drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread | rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |                 | uchar(*)(struct adapter*, uchar*)
drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread | rtw_cmd_thread ptr cmd_hdl |           BUF_SIZE |  1 |            pbuf | 1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960


Which says that led_blink_hdl() is called as a function pointer called
"cmd_hdl" from rtw_cmd_thread().

Hm...  It says it can be called from either rtw_cmd_thread() function
(the rtl8723bs or rtl8188eu version) which is not ideal.  But also
basically harmless so whatever...

regards,
dan carpenter

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

On Wednesday, April 14, 2021 7:21:50 AM CEST Dan Carpenter wrote:
> On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco wrote:
> > On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco 
wrote:
> > > > 1) The driver doesn't call that function from anywhere else than
> > > > the
> > > > macro. 2) You have explained that the macro add its symbol to a
> > > > slot
> > > > in an array that would shift all the subsequent elements down if
> > > > that
> > > > macro is not used exactly in the line where it is.
> > > > 3) Dan Carpenter said that that array is full of null functions (or
> > > > empty slots?).
> > > > 
> > > > Unless that function is called anonymously dereferencing its
> > > > address
> > > > from the position it occupies in the array, I'm not able to see
> > > > what
> > > > else means can any caller use.
> > > > 
> > > > I know I have much less experience than you with C: what can go
> > > > wrong?
> > > 
> > > Here's where the driver calls that function:
> > > 
> > > $ git grep wlancmds drivers/staging/rtl8723bs/
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl
> > > wlancmds[] = { drivers/staging/rtl8723bs/core/rtw_cmd.c:            
> > >   if
> > > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c:                      
> > > cmd_hdl
> > > = wlancmds[pcmd->cmdcode].h2cfuns;
> > 
> > OK, I had imagined an anonymous call from its location in the array (as
> > I wrote in the last phrase of my message). However, I thought that it
> > could have been an improbable possibility, not a real one.
> > 
> > Linux uses a lot of interesting ideas that newcomers like me should
> > learn. Things here are trickier than they appear at first sight.
> 
> One trick would be to build the Smatch cross function database.
> 
> https://www.spinics.net/lists/smatch/msg00568.html
> 
> Then you could do:
> 
> $ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
> file | caller | function | type | parameter | key | value |
> drivers/staging/rtl8723bs/core/rtw_cmd.c |       rtw_cmd_thread |
> rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |                 |
> uchar(*)(struct adapter*, uchar*)
> drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread |
> rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |                 |
> uchar(*)(struct adapter*, uchar*)
> drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread |
> rtw_cmd_thread ptr cmd_hdl |           BUF_SIZE |  1 |            pbuf |
> 1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960
> 
> 
> Which says that led_blink_hdl() is called as a function pointer called
> "cmd_hdl" from rtw_cmd_thread().
> 
> Hm...  It says it can be called from either rtw_cmd_thread() function
> (the rtl8723bs or rtl8188eu version) which is not ideal.  But also
> basically harmless so whatever...
> 
> regards,
> dan carpenter
>
Nice tool, thanks. I'll surely use it when it is needed to find out which  
callers use a function pointer.

However I cannot see how it can help in this context. That function *does* 
something, even if I cannot understand why someone needs a function to test 
the initialization of a pointer. Furthermore it is actually called by 
rtw_cmd_thread() (as you found out by using smatch) that expect one of the 
two possible values that led_blink_hdl() returns.

That said, what trick could I use for the purpose of getting rid of that 
function? At this point I'm not sure it could be made.

Regards,

Fabio
 

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Dan Carpenter]

On Wed, Apr 14, 2021 at 08:33:48AM +0200, Fabio M. De Francesco wrote:
> On Wednesday, April 14, 2021 7:21:50 AM CEST Dan Carpenter wrote:
> > On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco wrote:
> > > On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > > > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco 
> wrote:
> > > > > 1) The driver doesn't call that function from anywhere else than
> > > > > the
> > > > > macro. 2) You have explained that the macro add its symbol to a
> > > > > slot
> > > > > in an array that would shift all the subsequent elements down if
> > > > > that
> > > > > macro is not used exactly in the line where it is.
> > > > > 3) Dan Carpenter said that that array is full of null functions (or
> > > > > empty slots?).
> > > > > 
> > > > > Unless that function is called anonymously dereferencing its
> > > > > address
> > > > > from the position it occupies in the array, I'm not able to see
> > > > > what
> > > > > else means can any caller use.
> > > > > 
> > > > > I know I have much less experience than you with C: what can go
> > > > > wrong?
> > > > 
> > > > Here's where the driver calls that function:
> > > > 
> > > > $ git grep wlancmds drivers/staging/rtl8723bs/
> > > > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl
> > > > wlancmds[] = { drivers/staging/rtl8723bs/core/rtw_cmd.c:            
> > > >   if
> > > > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > > > drivers/staging/rtl8723bs/core/rtw_cmd.c:                      
> > > > cmd_hdl
> > > > = wlancmds[pcmd->cmdcode].h2cfuns;
> > > 
> > > OK, I had imagined an anonymous call from its location in the array (as
> > > I wrote in the last phrase of my message). However, I thought that it
> > > could have been an improbable possibility, not a real one.
> > > 
> > > Linux uses a lot of interesting ideas that newcomers like me should
> > > learn. Things here are trickier than they appear at first sight.
> > 
> > One trick would be to build the Smatch cross function database.
> > 
> > https://www.spinics.net/lists/smatch/msg00568.html 
> > 
> > Then you could do:
> > 
> > $ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
> > file | caller | function | type | parameter | key | value |
> > drivers/staging/rtl8723bs/core/rtw_cmd.c |       rtw_cmd_thread |
> > rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |                 |
> > uchar(*)(struct adapter*, uchar*)
> > drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread |
> > rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |                 |
> > uchar(*)(struct adapter*, uchar*)
> > drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread |
> > rtw_cmd_thread ptr cmd_hdl |           BUF_SIZE |  1 |            pbuf |
> > 1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960
> > 
> > 
> > Which says that led_blink_hdl() is called as a function pointer called
> > "cmd_hdl" from rtw_cmd_thread().
> > 
> > Hm...  It says it can be called from either rtw_cmd_thread() function
> > (the rtl8723bs or rtl8188eu version) which is not ideal.  But also
> > basically harmless so whatever...
> > 
> > regards,
> > dan carpenter
> >
> Nice tool, thanks. I'll surely use it when it is needed to find out which  
> callers use a function pointer.
> 
> However I cannot see how it can help in this context. That function *does* 
> something, even if I cannot understand why someone needs a function to test 
> the initialization of a pointer. Furthermore it is actually called by 
> rtw_cmd_thread() (as you found out by using smatch) that expect one of the 
> two possible values that led_blink_hdl() returns.
> 
> That said, what trick could I use for the purpose of getting rid of that 
> function? At this point I'm not sure it could be made.

If you look at how this is called:

drivers/staging/rtl8723bs/core/rtw_cmd.c
   449                  memcpy(pcmdbuf, pcmd->parmbuf, pcmd->cmdsz);
   450  
   451                  if (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
   452                          cmd_hdl = wlancmds[pcmd->cmdcode].h2cfuns;
   453  
   454                          if (cmd_hdl) {
   455                                  ret = cmd_hdl(pcmd->padapter, pcmdbuf);
                                                                      ^^^^^^^

   456                                  pcmd->res = ret;
   457                          }
   458  
   459                          pcmdpriv->cmd_seq++;
   460                  } else {
   461                          pcmd->res = H2C_PARAMETERS_ERROR;
   462                  }
   463  
   464                  cmd_hdl = NULL;

The led_blink_hdl() function returns success if "pcmdbuf" is non-NULL
and fail if it's NULL.  "pcmdbuf" is never supposed to be NULL.  (The
"supposed" caveat is because there may be a race in rtw_sdio_if1_init()
which briefly allows a NULL "pcmdbuf", but that is an unrelated bug).

Anyway, there is no point to the led_blink_hdl() function.  Likely
they intended it to do something but never got around to implementing
it.  Just delete it.

regards,
dan carpenter

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

On Wednesday, April 14, 2021 9:00:25 AM CEST Dan Carpenter wrote:
> On Wed, Apr 14, 2021 at 08:33:48AM +0200, Fabio M. De Francesco wrote:
> > On Wednesday, April 14, 2021 7:21:50 AM CEST Dan Carpenter wrote:
> > > On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco 
wrote:
> > > > On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > > > > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco
> > 
> > wrote:
> > > > > > 1) The driver doesn't call that function from anywhere else
> > > > > > than
> > > > > > the
> > > > > > macro. 2) You have explained that the macro add its symbol to a
> > > > > > slot
> > > > > > in an array that would shift all the subsequent elements down
> > > > > > if
> > > > > > that
> > > > > > macro is not used exactly in the line where it is.
> > > > > > 3) Dan Carpenter said that that array is full of null functions
> > > > > > (or
> > > > > > empty slots?).
> > > > > > 
> > > > > > Unless that function is called anonymously dereferencing its
> > > > > > address
> > > > > > from the position it occupies in the array, I'm not able to see
> > > > > > what
> > > > > > else means can any caller use.
> > > > > > 
> > > > > > I know I have much less experience than you with C: what can go
> > > > > > wrong?
> > > > > 
> > > > > Here's where the driver calls that function:
> > > > > 
> > > > > $ git grep wlancmds drivers/staging/rtl8723bs/
> > > > > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl
> > > > > 
> > > > > wlancmds[] = { drivers/staging/rtl8723bs/core/rtw_cmd.c:
> > > > >   if
> > > > > 
> > > > > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > > > > drivers/staging/rtl8723bs/core/rtw_cmd.c:
> > > > > cmd_hdl
> > > > > = wlancmds[pcmd->cmdcode].h2cfuns;
> > > > 
> > > > OK, I had imagined an anonymous call from its location in the array
> > > > (as
> > > > I wrote in the last phrase of my message). However, I thought that
> > > > it
> > > > could have been an improbable possibility, not a real one.
> > > > 
> > > > Linux uses a lot of interesting ideas that newcomers like me should
> > > > learn. Things here are trickier than they appear at first sight.
> > > 
> > > One trick would be to build the Smatch cross function database.
> > > 
> > > https://www.spinics.net/lists/smatch/msg00568.html
> > > 
> > > Then you could do:
> > > 
> > > $ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
> > > file | caller | function | type | parameter | key | value |
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c |       rtw_cmd_thread |
> > > rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |               
> > >  |
> > > uchar(*)(struct adapter*, uchar*)
> > > drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread |
> > > rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |               
> > >  |
> > > uchar(*)(struct adapter*, uchar*)
> > > drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread |
> > > rtw_cmd_thread ptr cmd_hdl |           BUF_SIZE |  1 |           
> > > pbuf |
> > > 1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960
> > > 
> > > 
> > > Which says that led_blink_hdl() is called as a function pointer
> > > called
> > > "cmd_hdl" from rtw_cmd_thread().
> > > 
> > > Hm...  It says it can be called from either rtw_cmd_thread() function
> > > (the rtl8723bs or rtl8188eu version) which is not ideal.  But also
> > > basically harmless so whatever...
> > > 
> > > regards,
> > > dan carpenter
> > 
> > Nice tool, thanks. I'll surely use it when it is needed to find out
> > which callers use a function pointer.
> > 
> > However I cannot see how it can help in this context. That function
> > *does* something, even if I cannot understand why someone needs a
> > function to test the initialization of a pointer. Furthermore it is
> > actually called by rtw_cmd_thread() (as you found out by using smatch)
> > that expect one of the two possible values that led_blink_hdl()
> > returns.
> > 
> > That said, what trick could I use for the purpose of getting rid of
> > that
> > function? At this point I'm not sure it could be made.
> 
> If you look at how this is called:
> 
> drivers/staging/rtl8723bs/core/rtw_cmd.c
>    449                  memcpy(pcmdbuf, pcmd->parmbuf, pcmd->cmdsz);
>    450
>    451                  if (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
>    452                          cmd_hdl =
> wlancmds[pcmd->cmdcode].h2cfuns; 453
>    454                          if (cmd_hdl) {
>    455                                  ret = cmd_hdl(pcmd->padapter,
> pcmdbuf); ^^^^^^^
> 
>    456                                  pcmd->res = ret;
>    457                          }
>    458
>    459                          pcmdpriv->cmd_seq++;
>    460                  } else {
>    461                          pcmd->res = H2C_PARAMETERS_ERROR;
>    462                  }
>    463
>    464                  cmd_hdl = NULL;
> 
> The led_blink_hdl() function returns success if "pcmdbuf" is non-NULL
> and fail if it's NULL.  "pcmdbuf" is never supposed to be NULL.  (The
> "supposed" caveat is because there may be a race in rtw_sdio_if1_init()
> which briefly allows a NULL "pcmdbuf", but that is an unrelated bug).
> 
> Anyway, there is no point to the led_blink_hdl() function.  Likely
> they intended it to do something but never got around to implementing
> it.  Just delete it.
> 
> regards,
> dan carpenter
>
Fine. I'm about to submit a patch.

Is there any formal means to acknowledge (in the patch) your contribution 
to the resolution of this problem?

Regards,

Fabio

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Dan Carpenter]

On Wed, Apr 14, 2021 at 09:59:36AM +0200, Fabio M. De Francesco wrote:
> Fine. I'm about to submit a patch.
> 
> Is there any formal means to acknowledge (in the patch) your contribution 
> to the resolution of this problem?

It doesn't matter.  Suggested-by.

regards,
dan carpenter

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio Aiuto]

On Wed, Apr 14, 2021 at 08:21:50AM +0300, Dan Carpenter wrote:
> On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco wrote:
> > On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco wrote:
> > > > 1) The driver doesn't call that function from anywhere else than the
> > > > macro. 2) You have explained that the macro add its symbol to a slot
> > > > in an array that would shift all the subsequent elements down if that
> > > > macro is not used exactly in the line where it is.
> > > > 3) Dan Carpenter said that that array is full of null functions (or
> > > > empty slots?).
> > > > 
> > > > Unless that function is called anonymously dereferencing its address
> > > > from the position it occupies in the array, I'm not able to see what
> > > > else means can any caller use.
> > > > 
> > > > I know I have much less experience than you with C: what can go wrong?
> > > 
> > > Here's where the driver calls that function:
> > > 
> > > $ git grep wlancmds drivers/staging/rtl8723bs/
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl wlancmds[]
> > > = { drivers/staging/rtl8723bs/core/rtw_cmd.c:               if
> > > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > > drivers/staging/rtl8723bs/core/rtw_cmd.c:                       cmd_hdl
> > > = wlancmds[pcmd->cmdcode].h2cfuns;
> > >
> > OK, I had imagined an anonymous call from its location in the array (as I 
> > wrote in the last phrase of my message). However, I thought that it could 
> > have been an improbable possibility, not a real one.
> > 
> > Linux uses a lot of interesting ideas that newcomers like me should learn. 
> > Things here are trickier than they appear at first sight.
> 
> One trick would be to build the Smatch cross function database.
> 
> https://www.spinics.net/lists/smatch/msg00568.html
> 
> Then you could do:
> 
> $ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
> file | caller | function | type | parameter | key | value |
> drivers/staging/rtl8723bs/core/rtw_cmd.c |       rtw_cmd_thread | rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |                 | uchar(*)(struct adapter*, uchar*)
> drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread | rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |                 | uchar(*)(struct adapter*, uchar*)
> drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread | rtw_cmd_thread ptr cmd_hdl |           BUF_SIZE |  1 |            pbuf | 1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960
> 
> 
> Which says that led_blink_hdl() is called as a function pointer called
> "cmd_hdl" from rtw_cmd_thread().
> 
> Hm...  It says it can be called from either rtw_cmd_thread() function
> (the rtl8723bs or rtl8188eu version) which is not ideal.  But also
> basically harmless so whatever...
> 
> regards,
> dan carpenter
> 

very powerful tool.

I tried this:

fabio@agape:~/src/git/kernels/staging$ ~/src/git/smatch/smatch_data/db/smdb.py led_blink_hdl
Traceback (most recent call last):
  File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 725, in <module>
    print_caller_info("", func)
  File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 366, in print_caller_info
    ptrs = get_function_pointers(func)
  File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 53, in get_function_pointers
    get_function_pointers_helper(func)
  File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 38, in get_function_pointers_helper
    cur.execute("select distinct ptr from function_ptr where function = '%s';" %(func))
sqlite3.OperationalError: no such table: function_ptr

I run smatch version 1.71 on Debian Buster machine

what's happened?

thanks in advance,

fabio

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Dan Carpenter]

On Wed, Apr 14, 2021 at 09:40:36AM +0200, Fabio Aiuto wrote:
> On Wed, Apr 14, 2021 at 08:21:50AM +0300, Dan Carpenter wrote:
> > On Tue, Apr 13, 2021 at 10:08:32PM +0200, Fabio M. De Francesco wrote:
> > > On Tuesday, April 13, 2021 9:48:44 PM CEST Matthew Wilcox wrote:
> > > > On Tue, Apr 13, 2021 at 09:45:03PM +0200, Fabio M. De Francesco wrote:
> > > > > 1) The driver doesn't call that function from anywhere else than the
> > > > > macro. 2) You have explained that the macro add its symbol to a slot
> > > > > in an array that would shift all the subsequent elements down if that
> > > > > macro is not used exactly in the line where it is.
> > > > > 3) Dan Carpenter said that that array is full of null functions (or
> > > > > empty slots?).
> > > > > 
> > > > > Unless that function is called anonymously dereferencing its address
> > > > > from the position it occupies in the array, I'm not able to see what
> > > > > else means can any caller use.
> > > > > 
> > > > > I know I have much less experience than you with C: what can go wrong?
> > > > 
> > > > Here's where the driver calls that function:
> > > > 
> > > > $ git grep wlancmds drivers/staging/rtl8723bs/
> > > > drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl wlancmds[]
> > > > = { drivers/staging/rtl8723bs/core/rtw_cmd.c:               if
> > > > (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
> > > > drivers/staging/rtl8723bs/core/rtw_cmd.c:                       cmd_hdl
> > > > = wlancmds[pcmd->cmdcode].h2cfuns;
> > > >
> > > OK, I had imagined an anonymous call from its location in the array (as I 
> > > wrote in the last phrase of my message). However, I thought that it could 
> > > have been an improbable possibility, not a real one.
> > > 
> > > Linux uses a lot of interesting ideas that newcomers like me should learn. 
> > > Things here are trickier than they appear at first sight.
> > 
> > One trick would be to build the Smatch cross function database.
> > 
> > https://www.spinics.net/lists/smatch/msg00568.html 
> > 
> > Then you could do:
> > 
> > $ ~/path/to/smatch_data/db/smdb.py led_blink_hdl
> > file | caller | function | type | parameter | key | value |
> > drivers/staging/rtl8723bs/core/rtw_cmd.c |       rtw_cmd_thread | rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |                 | uchar(*)(struct adapter*, uchar*)
> > drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread | rtw_cmd_thread ptr cmd_hdl |           INTERNAL | -1 |                 | uchar(*)(struct adapter*, uchar*)
> > drivers/staging/rtl8188eu/core/rtw_cmd.c |       rtw_cmd_thread | rtw_cmd_thread ptr cmd_hdl |           BUF_SIZE |  1 |            pbuf | 1,4,6,8,12,14,16,19-20,23-24,48,740,884,892,900,960
> > 
> > 
> > Which says that led_blink_hdl() is called as a function pointer called
> > "cmd_hdl" from rtw_cmd_thread().
> > 
> > Hm...  It says it can be called from either rtw_cmd_thread() function
> > (the rtl8723bs or rtl8188eu version) which is not ideal.  But also
> > basically harmless so whatever...
> > 
> > regards,
> > dan carpenter
> > 
> 
> very powerful tool.
> 
> I tried this:
> 
> fabio@agape:~/src/git/kernels/staging$ ~/src/git/smatch/smatch_data/db/smdb.py led_blink_hdl
> Traceback (most recent call last):
>   File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 725, in <module>
>     print_caller_info("", func)
>   File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 366, in print_caller_info
>     ptrs = get_function_pointers(func)
>   File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 53, in get_function_pointers
>     get_function_pointers_helper(func)
>   File "/home/fabio/src/git/smatch/smatch_data/db/smdb.py", line 38, in get_function_pointers_helper
>     cur.execute("select distinct ptr from function_ptr where function = '%s';" %(func))
> sqlite3.OperationalError: no such table: function_ptr
> 
> I run smatch version 1.71 on Debian Buster machine
> 

It takes a few hours to build the DB.  The instructions are in the
email.

	~/path/to/smatch/smatch_scripts/build_kernel_data.sh

regards,
dan carpenter

Re: [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink_hdl()

[Greg Kroah-Hartman]

On Tue, Apr 13, 2021 at 05:59:08PM +0200, Fabio M. De Francesco wrote:
> Removed the led_blink_hdl() function (declaration, definition, and
> caller code) because it's useless. It only seems to check whether or not a
> given pointer is NULL. There are other (simpler) means for that purpose.

But you do not actually do that here.  Why not?  You just removed
something, does it still work properly with that removal?

> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Why the leading ":" in your subject line?

> ---
>  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 1 -
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
>  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 1 -
>  3 files changed, 11 deletions(-)

Does this patch require some other patch to be applied first?

thanks,

greg k-h

[Outreachy kernel] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

Removed the led_blink_hdl() function (declaration and definition).
Declared dummy_function() in include/rtw_mlme_ext.h and defined it in
core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER
macro to make use of dummy_function().

Reported-by: Julia Lawall <julia.lawall@inria.fr>
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_cmd.c         | 4 +++-
 drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
 drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++-
 3 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index 0297fbad7bce..7b6102a2bb2c 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -87,6 +87,8 @@ static struct _cmd_callback rtw_cmd_callback[] = {
 	{GEN_CMD_CODE(_RunInThreadCMD), NULL},/*64*/
 };
 
+u8 dummy_functioni(struct adapter *var0, u8 *var1) { return 0; }
+
 static struct cmd_hdl wlancmds[] = {
 	GEN_DRV_CMD_HANDLER(0, NULL) /*0*/
 	GEN_DRV_CMD_HANDLER(0, NULL)
@@ -150,7 +152,7 @@ static struct cmd_hdl wlancmds[] = {
 
 	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
 	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param), set_chplan_hdl) /*59*/
-	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), led_blink_hdl) /*60*/
+	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), dummy_function) /*60*/
 
 	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelSwitch_param), set_csa_hdl) /*61*/
 	GEN_MLME_EXT_HANDLER(sizeof(struct TDLSoption_param), tdls_hdl) /*62*/
diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
index 873d3792ac8e..963ea80083c8 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
@@ -6189,15 +6189,6 @@ u8 set_chplan_hdl(struct adapter *padapter, unsigned char *pbuf)
 	return	H2C_SUCCESS;
 }
 
-u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf)
-{
-
-	if (!pbuf)
-		return H2C_PARAMETERS_ERROR;
-
-	return	H2C_SUCCESS;
-}
-
 u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf)
 {
 	return	H2C_REJECTED;
diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
index 5e6cf63956b8..57977da78eb3 100644
--- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
+++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
@@ -745,11 +745,12 @@ u8 chk_bmc_sleepq_hdl(struct adapter *padapter, unsigned char *pbuf);
 u8 tx_beacon_hdl(struct adapter *padapter, unsigned char *pbuf);
 u8 set_ch_hdl(struct adapter *padapter, u8 *pbuf);
 u8 set_chplan_hdl(struct adapter *padapter, unsigned char *pbuf);
-u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf);
 u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf);	/* Kurt: Handling DFS channel switch announcement ie. */
 u8 tdls_hdl(struct adapter *padapter, unsigned char *pbuf);
 u8 run_in_thread_hdl(struct adapter *padapter, u8 *pbuf);
 
+/* Dummy function used to fill elements of an array of function pointers */
+u8 dummy_function(struct adapter *, u8 *);
 
 #define GEN_DRV_CMD_HANDLER(size, cmd)	{size, &cmd ## _hdl},
 #define GEN_MLME_EXT_HANDLER(size, cmd)	{size, cmd},
-- 
2.31.1

Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio Aiuto]

On Wed, Apr 14, 2021 at 01:52:43PM +0200, Fabio M. De Francesco wrote:
> Removed the led_blink_hdl() function (declaration and definition).
> Declared dummy_function() in include/rtw_mlme_ext.h and defined it in
> core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER
> macro to make use of dummy_function().
> 
> Reported-by: Julia Lawall <julia.lawall@inria.fr>
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 4 +++-
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
>  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++-
>  3 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index 0297fbad7bce..7b6102a2bb2c 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -87,6 +87,8 @@ static struct _cmd_callback rtw_cmd_callback[] = {
>  	{GEN_CMD_CODE(_RunInThreadCMD), NULL},/*64*/
>  };
>  
> +u8 dummy_functioni(struct adapter *var0, u8 *var1) { return 0; }
> +

I think that this won't compile

>  static struct cmd_hdl wlancmds[] = {
>  	GEN_DRV_CMD_HANDLER(0, NULL) /*0*/
>  	GEN_DRV_CMD_HANDLER(0, NULL)
> @@ -150,7 +152,7 @@ static struct cmd_hdl wlancmds[] = {
>  
>  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
>  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param), set_chplan_hdl) /*59*/
> -	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), led_blink_hdl) /*60*/
> +	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), dummy_function) /*60*/
>  
>  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelSwitch_param), set_csa_hdl) /*61*/
>  	GEN_MLME_EXT_HANDLER(sizeof(struct TDLSoption_param), tdls_hdl) /*62*/
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 873d3792ac8e..963ea80083c8 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -6189,15 +6189,6 @@ u8 set_chplan_hdl(struct adapter *padapter, unsigned char *pbuf)
>  	return	H2C_SUCCESS;
>  }
>  
> -u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf)
> -{
> -
> -	if (!pbuf)
> -		return H2C_PARAMETERS_ERROR;
> -
> -	return	H2C_SUCCESS;
> -}
> -
>  u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf)
>  {
>  	return	H2C_REJECTED;
> diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> index 5e6cf63956b8..57977da78eb3 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_mlme_ext.h
> @@ -745,11 +745,12 @@ u8 chk_bmc_sleepq_hdl(struct adapter *padapter, unsigned char *pbuf);
>  u8 tx_beacon_hdl(struct adapter *padapter, unsigned char *pbuf);
>  u8 set_ch_hdl(struct adapter *padapter, u8 *pbuf);
>  u8 set_chplan_hdl(struct adapter *padapter, unsigned char *pbuf);
> -u8 led_blink_hdl(struct adapter *padapter, unsigned char *pbuf);
>  u8 set_csa_hdl(struct adapter *padapter, unsigned char *pbuf);	/* Kurt: Handling DFS channel switch announcement ie. */
>  u8 tdls_hdl(struct adapter *padapter, unsigned char *pbuf);
>  u8 run_in_thread_hdl(struct adapter *padapter, u8 *pbuf);
>  
> +/* Dummy function used to fill elements of an array of function pointers */
> +u8 dummy_function(struct adapter *, u8 *);

here 'dummy_function' prototype is declared

>  
>  #define GEN_DRV_CMD_HANDLER(size, cmd)	{size, &cmd ## _hdl},
>  #define GEN_MLME_EXT_HANDLER(size, cmd)	{size, cmd},
> -- 
> 2.31.1
> 
> 

thank you,

fabio

Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()

[Greg Kroah-Hartman]

On Wed, Apr 14, 2021 at 01:52:43PM +0200, Fabio M. De Francesco wrote:
> Removed the led_blink_hdl() function (declaration and definition).
> Declared dummy_function() in include/rtw_mlme_ext.h and defined it in
> core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER
> macro to make use of dummy_function().

No no no.

If you want to remove is function declaration and use, then do it
properly.

The code is crazy, I agree, but it should not be difficult to just
remove this correctly instead of papering over this mess.

Also note that no one actually calls this function if you look at the
logic here.  It might take some good knowledge of C to unwind this crud,
but once done, you should be able to "prove" it's not called and how to
remove it correctly.

And no, I'm not going to say how to do it, that's an exercise best left
for the reader.  But I will hint that this was done in the past, in
2014, in another driver in the tree with a codebase much like this one,
so it shouldn't be hard to find an example of it.  Only took me a few
minutes...

good luck!

greg k-h

Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

On Wednesday, April 14, 2021 2:18:13 PM CEST Greg Kroah-Hartman wrote:
> On Wed, Apr 14, 2021 at 01:52:43PM +0200, Fabio M. De Francesco wrote:
> > Removed the led_blink_hdl() function (declaration and definition).
> > Declared dummy_function() in include/rtw_mlme_ext.h and defined it in
> > core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER
> > macro to make use of dummy_function().
> 
> No no no.
> 
> If you want to remove is function declaration and use, then do it
> properly.
> 
> The code is crazy, I agree, but it should not be difficult to just
> remove this correctly instead of papering over this mess.
> 
> Also note that no one actually calls this function if you look at the
> logic here.  
>
> It might take some good knowledge of C to unwind this crud,
> but once done, you should be able to "prove" it's not called
>
Proving that no one actually calls it it's beyond my 
current knowledge of programming with C.

Matthew W., who is for sure more experienced than I am , 
wrote that that function pointer in the array is used somewhere else. 

Copied and pasted here from his message:

"Here's where the driver calls that function:
$ git grep wlancmds drivers/staging/rtl8723bs/
drivers/staging/rtl8723bs/core/rtw_cmd.c:static struct cmd_hdl wlancmds[] = {
drivers/staging/rtl8723bs/core/rtw_cmd.c:               if (pcmd->cmdcode < ARRAY_SIZE(wlancmds)) {
drivers/staging/rtl8723bs/core/rtw_cmd.c:                       cmd_hdl = wlancmds[pcmd->cmdcode].h2cfuns;
>
> and how to
> remove it correctly.
>
I think that doing it correctly depends on the "prove" which you requested.
Doesn't it?
> 
> And no, I'm not going to say how to do it, that's an exercise best left
> for the reader.
>
It sounds perfectly reasonable and I agree in full.
>
> But I will hint that this was done in the past, in
> 2014, in another driver in the tree with a codebase much like this one,
> so it shouldn't be hard to find an example of it.  Only took me a few
> minutes...
>
I'm sure it took you only a few minutes. If this can be accomplished by using grep 
on git log output I need some time to read this command manual again. I suppose 
that the search should be made by combining "remove", "function", "drivers/staging",
and "2014". At the moment I don't know how to do that.

Notwithstanding I have said all that you read above, you can be sure that I won't give
up so easily even if it will take days :) 
> 
> good luck!
>
Thanks, 

Fabio
> 
> greg k-h

Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()

[Dan Carpenter]

On Wed, Apr 14, 2021 at 01:52:43PM +0200, Fabio M. De Francesco wrote:
> Removed the led_blink_hdl() function (declaration and definition).
> Declared dummy_function() in include/rtw_mlme_ext.h and defined it in
> core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER
> macro to make use of dummy_function().
> 
> Reported-by: Julia Lawall <julia.lawall@inria.fr>
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 4 +++-
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
>  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++-
>  3 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> index 0297fbad7bce..7b6102a2bb2c 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> @@ -87,6 +87,8 @@ static struct _cmd_callback rtw_cmd_callback[] = {
>  	{GEN_CMD_CODE(_RunInThreadCMD), NULL},/*64*/
>  };
>  
> +u8 dummy_functioni(struct adapter *var0, u8 *var1) { return 0; }
> +
>  static struct cmd_hdl wlancmds[] = {
>  	GEN_DRV_CMD_HANDLER(0, NULL) /*0*/
>  	GEN_DRV_CMD_HANDLER(0, NULL)
> @@ -150,7 +152,7 @@ static struct cmd_hdl wlancmds[] = {
>  
>  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
>  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param), set_chplan_hdl) /*59*/
> -	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), led_blink_hdl) /*60*/
> +	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), dummy_function) /*60*/

No, no.  Don't create a dummy function. Do it like so:

	GEN_DRV_CMD_HANDLER(0, NULL) /* 60 */

regards,
dan carpenter

Re: [Outreachy kernel] [PATCH] staging: rtl8723bs: Remove useless led_blink_hdl()

[Fabio M. De Francesco]

On Wednesday, April 14, 2021 3:24:14 PM CEST Dan Carpenter wrote:
> On Wed, Apr 14, 2021 at 01:52:43PM +0200, Fabio M. De Francesco wrote:
> > Removed the led_blink_hdl() function (declaration and definition).
> > Declared dummy_function() in include/rtw_mlme_ext.h and defined it in
> > core/rtw_cmd.c. Changed the second parameter of GEN_MLME_EXT_HANDLER
> > macro to make use of dummy_function().
> > 
> > Reported-by: Julia Lawall <julia.lawall@inria.fr>
> > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> >  drivers/staging/rtl8723bs/core/rtw_cmd.c         | 4 +++-
> >  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c    | 9 ---------
> >  drivers/staging/rtl8723bs/include/rtw_mlme_ext.h | 3 ++-
> >  3 files changed, 5 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > b/drivers/staging/rtl8723bs/core/rtw_cmd.c index
> > 0297fbad7bce..7b6102a2bb2c 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
> > @@ -87,6 +87,8 @@ static struct _cmd_callback rtw_cmd_callback[] = {
> > 
> >  	{GEN_CMD_CODE(_RunInThreadCMD), NULL},/*64*/
> >  
> >  };
> > 
> > +u8 dummy_functioni(struct adapter *var0, u8 *var1) { return 0; }
> > +
> > 
> >  static struct cmd_hdl wlancmds[] = {
> >  
> >  	GEN_DRV_CMD_HANDLER(0, NULL) /*0*/
> >  	GEN_DRV_CMD_HANDLER(0, NULL)
> > 
> > @@ -150,7 +152,7 @@ static struct cmd_hdl wlancmds[] = {
> > 
> >  	GEN_MLME_EXT_HANDLER(0, h2c_msg_hdl) /*58*/
> >  	GEN_MLME_EXT_HANDLER(sizeof(struct SetChannelPlan_param),
> >  	set_chplan_hdl) /*59*/> 
> > -	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param), 
led_blink_hdl)
> > /*60*/ +	GEN_MLME_EXT_HANDLER(sizeof(struct LedBlink_param),
> > dummy_function) /*60*/
> No, no.  Don't create a dummy function. Do it like so:
> 
> 	GEN_DRV_CMD_HANDLER(0, NULL) /* 60 */
> 
> regards,
> dan carpenter
>
I'm replying late because I didn't want to blindly use that solution; I 
mean that I wanted to understand why I can simply put 0 and NULL into that 
macro. I had seen it made in other lines that initialize wlancmds[] 
elements, but I wasn't sure if it could work for the specific slot where 
the pointer to led_blinck_hdl was supposed to be placed.

Now I think that it is why, in this case, cmd_hdl would be set to NULL by 
the call to wlancmds[pcmd->cmdcode].h2cfuns and the cmd_hdl() function 
wouldn't be called because cmd_hdl is tested within an "if" statement.

Therefore a simple GEN_DRV_CMD_HANDLER(0, NULL) at slot number would be the 
simplest and most obvious solution.

Is the above argument sound?

Thanks for your kind help,

Fabio