linux-kernel - Re: [PATCH 21/34] iwlegacy: don't warn for unused variables with DEBUG

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]

Date: Wed, 3 Apr 2024 07:26:10 -0700
From: Jeff Johnson <quic_jjohnson@...cinc.com>
To: Arnd Bergmann <arnd@...nel.org>, <linux-kernel@...r.kernel.org>,
        Stanislaw
 Gruszka <stf_xl@...pl>, Kalle Valo <kvalo@...nel.org>
CC: Arnd Bergmann <arnd@...db.de>, <linux-wireless@...r.kernel.org>
Subject: Re: [PATCH 21/34] iwlegacy: don't warn for unused variables with
 DEBUG_FS=n

On 4/3/2024 1:06 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.de>
> 
> The reference to il_rate_mcs is inside of an #ifdef, causing a W=1 warning:
> 
> drivers/net/wireless/intel/iwlegacy/4965-rs.c:189:38: error: unused variable 'il_rate_mcs' [-Werror,-Wunused-const-variable]
> static const struct il_rate_mcs_info il_rate_mcs[RATE_COUNT] = {
> 
> Replace the #ifdef with a PTR_IF() for better compile time analysis.
> The dead code will still get eliminated, but the warning goes away.
> 
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
>  drivers/net/wireless/intel/iwlegacy/4965-rs.c | 15 ++-------------
>  drivers/net/wireless/intel/iwlegacy/common.h  |  2 --
>  2 files changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlegacy/4965-rs.c b/drivers/net/wireless/intel/iwlegacy/4965-rs.c
> index 718efb1aa1b0..1aa2cee5d131 100644
> --- a/drivers/net/wireless/intel/iwlegacy/4965-rs.c
> +++ b/drivers/net/wireless/intel/iwlegacy/4965-rs.c
> @@ -132,15 +132,8 @@ static void il4965_rs_fill_link_cmd(struct il_priv *il,
>  static void il4965_rs_stay_in_table(struct il_lq_sta *lq_sta,
>  				    bool force_search);
>  
> -#ifdef CONFIG_MAC80211_DEBUGFS
>  static void il4965_rs_dbgfs_set_mcs(struct il_lq_sta *lq_sta,
>  				    u32 *rate_n_flags, int idx);
> -#else
> -static void
> -il4965_rs_dbgfs_set_mcs(struct il_lq_sta *lq_sta, u32 * rate_n_flags, int idx)
> -{
> -}
> -#endif
>  
>  /*
>   * The following tables contain the expected throughput metrics for all rates
> @@ -2495,8 +2488,6 @@ il4965_rs_free_sta(void *il_r, struct ieee80211_sta *sta, void *il_sta)
>  	D_RATE("leave\n");
>  }
>  
> -#ifdef CONFIG_MAC80211_DEBUGFS
> -

if the const table is only used by this function, why not do the trivial thing
and relocate it to be inside this #ifdef, or even to be within the function
itself?

>  static void
>  il4965_rs_dbgfs_set_mcs(struct il_lq_sta *lq_sta, u32 * rate_n_flags, int idx)
>  {
> @@ -2758,7 +2749,6 @@ il4965_rs_add_debugfs(void *il, void *il_sta, struct dentry *dir)
>  	debugfs_create_u8("tx_agg_tid_enable", 0600, dir,
>  			  &lq_sta->tx_agg_tid_en);
>  }
> -#endif
>  
>  /*
>   * Initialization of rate scaling information is done by driver after
> @@ -2781,9 +2771,8 @@ static const struct rate_control_ops rs_4965_ops = {
>  	.free = il4965_rs_free,
>  	.alloc_sta = il4965_rs_alloc_sta,
>  	.free_sta = il4965_rs_free_sta,
> -#ifdef CONFIG_MAC80211_DEBUGFS
> -	.add_sta_debugfs = il4965_rs_add_debugfs,
> -#endif
> +	.add_sta_debugfs = PTR_IF(IS_ENABLED(CONFIG_DEBUG_FS),

CONFIG_DEBUG_FS != CONFIG_MAC80211_DEBUGFS, is that intentional?

> +				  il4965_rs_add_debugfs),

so using this you don't get a warning about all those static functions and
tables being unused when the feature is disabled? that's pretty cool.
is that because the compiler sees them instead of the preprocessor stripping
them out?

>  };
>  
>  int
> diff --git a/drivers/net/wireless/intel/iwlegacy/common.h b/drivers/net/wireless/intel/iwlegacy/common.h
> index 69687fcf963f..b9f1daf0901b 100644
> --- a/drivers/net/wireless/intel/iwlegacy/common.h
> +++ b/drivers/net/wireless/intel/iwlegacy/common.h
> @@ -2804,9 +2804,7 @@ struct il_lq_sta {
>  	struct il_scale_tbl_info lq_info[LQ_SIZE];	/* "active", "search" */
>  	struct il_traffic_load load[TID_MAX_LOAD_COUNT];
>  	u8 tx_agg_tid_en;
> -#ifdef CONFIG_MAC80211_DEBUGFS
>  	u32 dbg_fixed_rate;
> -#endif

and yes it is trivial, but we now waste this memory when the feature is not
enabled

>  	struct il_priv *drv;
>  
>  	/* used to be in sta_info */