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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <42d24db0-b403-4c4b-8607-38f3cd55cf63@stanley.mountain>
Date: Sun, 8 Dec 2024 13:27:15 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Rodrigo Gobbi <rodrigo.gobbi.7@...il.com>
Cc: gregkh@...uxfoundation.org, philipp.g.hortmann@...il.com,
	~lkcamp/patches@...ts.sr.ht, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] staging: rtl8723bs: remove code depending on cflag

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@...il.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