LinuxLists.cc - [Outreachy kernel] [PATCH] :staging: rtl8723bs: Remove useless led_blink

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20210413155908.8691-1-fmdefrancesco%40gmail.com.
>

2021-04-13 21:28:37

by Greg Kroah-Hartman

[permalink] [raw]

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

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 <[email protected]>

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

2021-04-13 21:31:37

[permalink] [raw]

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

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 <[email protected]>
> > ---
> >
> > 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
> > [email protected]. To view this discussion
> > on the web visit
> > https://groups.google.com/d/msgid/outreachy-kernel/20210413155908.8691
> > -1-fmdefrancesco%40gmail.com.

2021-04-13 21:34:13

[permalink] [raw]

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

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 <[email protected]>
> > > ---
> > >
> > > 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

2021-04-13 21:52:45

[permalink] [raw]

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

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 <[email protected]>
> > > > ---
> > > >
> > > > 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

2021-04-13 22:48:20

[permalink] [raw]

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

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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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

2021-04-13 23:10:49

[permalink] [raw]

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

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 <[email protected]>
> > > > > > > ---
> > > > > > >
> > > > > > > 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

2021-04-13 23:42:09

[permalink] [raw]

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

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
> > > > > > > > > > <[email protected]>
> > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > 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

2021-04-14 00:03:39

[permalink] [raw]

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

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

2021-04-14 06:13:54

[permalink] [raw]

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

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 <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > 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

2021-04-14 06:18:53

[permalink] [raw]

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

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
> > > > > > > > <[email protected]>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > 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

2021-04-14 06:41:06

[permalink] [raw]

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

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
> > > > > > > > > <[email protected]>
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > 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
>
>
>
>
>

2021-04-14 06:42:56

by Matthew Wilcox

[permalink] [raw]

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

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;

2021-04-14 12:38:37

[permalink] [raw]

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

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

2021-04-14 13:52:41

[permalink] [raw]

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

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

2021-04-14 14:13:14

[permalink] [raw]

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

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

2021-04-14 14:37:57

[permalink] [raw]

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

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

2021-04-14 20:03:08

by Fabio Aiuto

[permalink] [raw]

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

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

2021-04-14 20:03:27

[permalink] [raw]

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

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

2021-04-14 20:30:46