[Outreachy,kernel] :staging: rtl8723bs: Remove useless led_blink_hdl()
diff mbox series

Message ID 20210413155908.8691-1-fmdefrancesco@gmail.com
State New
Headers show
Series
  • [Outreachy,kernel] :staging: rtl8723bs: Remove useless led_blink_hdl()
Related show

Commit Message

Fabio M. De Francesco April 13, 2021, 3:59 p.m. UTC
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(-)

Comments

Julia Lawall April 13, 2021, 4:04 p.m. UTC | #1
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.
>
Greg Kroah-Hartman April 13, 2021, 4:06 p.m. UTC | #2
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
Fabio M. De Francesco April 13, 2021, 4:19 p.m. UTC | #3
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.
Julia Lawall April 13, 2021, 4:27 p.m. UTC | #4
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
Fabio M. De Francesco April 13, 2021, 4:47 p.m. UTC | #5
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
Dan Carpenter April 13, 2021, 6:20 p.m. UTC | #6
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
Fabio M. De Francesco April 13, 2021, 6:30 p.m. UTC | #7
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
Julia Lawall April 13, 2021, 6:57 p.m. UTC | #8
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
Fabio M. De Francesco April 13, 2021, 7:16 p.m. UTC | #9
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
Julia Lawall April 13, 2021, 7:25 p.m. UTC | #10
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
>
>
>
>
>
Fabio M. De Francesco April 13, 2021, 7:45 p.m. UTC | #11
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
Matthew Wilcox April 13, 2021, 7:48 p.m. UTC | #12
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;
Fabio M. De Francesco April 13, 2021, 8:08 p.m. UTC | #13
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
Dan Carpenter April 14, 2021, 5:21 a.m. UTC | #14
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
Fabio M. De Francesco April 14, 2021, 6:33 a.m. UTC | #15
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
Dan Carpenter April 14, 2021, 7 a.m. UTC | #16
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
Fabio Aiuto April 14, 2021, 7:40 a.m. UTC | #17
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
Dan Carpenter April 14, 2021, 7:47 a.m. UTC | #18
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
Fabio M. De Francesco April 14, 2021, 7:59 a.m. UTC | #19
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
Dan Carpenter April 14, 2021, 8:06 a.m. UTC | #20
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

Patch
diff mbox series

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);