Re: [PATCH v3] staging: rtl8723bs: remove code depending on cflag

[Dan Carpenter <dan.carpenter@linaro.org>]

On Fri, Dec 06, 2024 at 07:53:15PM -0300, Rodrigo Gobbi wrote:
> Remove code that is depending on DBG_RX_SIGNAL_DISPLAY_RAW_DATA cflag
> since it's not compiling and there is no reference for it's usage;
> 
> Signed-off-by: Rodrigo Gobbi <rodrigo.gobbi.7@gmail.com>
> ---
> The previous version suggested to fix the tiny error over the cflag usage.
> Greg pointed at [1] that is not common to build using cflags. Since there
> is no reference about this debug usage flag, I'm removing all the code depending on it.
> Tks and regards.
> 
> [1] https://lore.kernel.org/lkml/20241125225308.8702-1-rodrigo.gobbi.7@gmail.com/t/#med5299468bc43fa89d18892d6d869a93d6138475
> ---
> Changelog
> v3: remove code as suggested;
> v2: https://lore.kernel.org/lkml/20241125225308.8702-1-rodrigo.gobbi.7@gmail.com/t/#mf22f30a9c689bd793988d7e7a58c0b119206116c
> v1: https://lore.kernel.org/linux-staging/2024112500-authentic-primarily-b5da@gregkh/T/#mea4fba3775a1015f345dfe322854c55db0cddf43
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme_ext.c |  1 -
>  drivers/staging/rtl8723bs/hal/hal_com.c       | 55 -------------------
>  .../staging/rtl8723bs/hal/rtl8723b_rxdesc.c   |  4 --
>  drivers/staging/rtl8723bs/include/hal_com.h   |  5 --
>  drivers/staging/rtl8723bs/include/rtw_recv.h  | 18 ------
>  5 files changed, 83 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> index 317f3db19397..952ce6dd5af9 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme_ext.c
> @@ -4959,7 +4959,6 @@ void _linked_info_dump(struct adapter *padapter)
>  					rtw_hal_get_def_var(padapter, HW_DEF_RA_INFO_DUMP, &i);
>  			}
>  		}
> -		rtw_hal_set_def_var(padapter, HAL_DEF_DBG_RX_INFO_DUMP, NULL);

When I'm reading this commit I think, "How is this related to removing the
ifdef?"  The commit message should answer any kind of reasonable questions
a review is probably going to ask but it doesn't give any information on
this.  I figured it out by reviewing the code, but I shouldn't *have* to.
It should be in the commit message.

This commit would easier to review if it were broken up in a different
way.

[patch 1] just delete DBG_RX_SIGNAL_DISPLAY_RAW_DATA ifdefed code
[patch 2] delete HAL_DEF_DBG_RX_INFO_DUMP and related code
[patch 3] delete SetHalDefVar() and all the callers

>  	}
>  }
>  
> diff --git a/drivers/staging/rtl8723bs/hal/hal_com.c b/drivers/staging/rtl8723bs/hal/hal_com.c
> index 95fb38283c58..b41ec89932af 100644
> --- a/drivers/staging/rtl8723bs/hal/hal_com.c
> +++ b/drivers/staging/rtl8723bs/hal/hal_com.c
> @@ -682,14 +682,6 @@ u8 SetHalDefVar(
>  	u8 bResult = _SUCCESS;
>  
>  	switch (variable) {
> -	case HAL_DEF_DBG_RX_INFO_DUMP:
> -
> -		if (odm->bLinked) {
> -			#ifdef DBG_RX_SIGNAL_DISPLAY_RAW_DATA
> -			rtw_dump_raw_rssi_info(adapter);
> -			#endif
> -		}
> -		break;

So just leave this like:

	case HAL_DEF_DBG_RX_INFO_DUMP:
		break;

Otherwise I have to wonder, are there any other callers which pass
HAL_DEF_DBG_RX_INFO_DUMP?  Because if there are, then now they go
to the default case and that triggers a bug.

patch 2 would delete all HAL_DEF_DBG_RX_INFO_DUMP related code.
That is an easy patch to review.  There are probably other unused
values in the enum.

patch 3 would delete SetHalDefVar() and all related code.  Again
pretty easy to review because any missed callers would trigger a
build error.

regards,
dan carpenter