diff mbox series

[v2] wifi: iwlwifi: mld: fix building with CONFIG_PM_SLEEP disabled

Message ID f435bd9c8186176ffa12fd3650fac71cacdeebe1.1743946314.git.lukas@wunner.de (mailing list archive)
State Not Applicable
Delegated to: Johannes Berg
Headers show
Series [v2] wifi: iwlwifi: mld: fix building with CONFIG_PM_SLEEP disabled | expand

Checks

Context Check Description
wifibot/fixes_present success Fixes tag not required for -next series
wifibot/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
wifibot/tree_selection success Guessed tree name to be wireless-next
wifibot/ynl success Generated files up to date; no warnings/errors; no diff in generated;
wifibot/build_32bit success Errors and warnings before: 0 this patch: 0
wifibot/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
wifibot/build_clang success Errors and warnings before: 0 this patch: 0
wifibot/build_clang_rust success No Rust files in patch. Skipping build
wifibot/build_tools success No tools touched, skip
wifibot/check_selftest success No net selftest shell script
wifibot/checkpatch success total: 0 errors, 0 warnings, 0 checks, 38 lines checked
wifibot/deprecated_api success None detected
wifibot/header_inline success No static functions without inline keyword in header files
wifibot/kdoc success Errors and warnings before: 0 this patch: 0
wifibot/source_inline success Was 0 now: 0
wifibot/verify_fixes success Fixes tag looks correct
wifibot/verify_signedoff success Signed-off-by tag matches author and committer

Commit Message

Lukas Wunner April 6, 2025, 1:43 p.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

The newly added driver causes multiple build problems when CONFIG_PM_SLEEP
is disabled:

drivers/net/wireless/intel/iwlwifi/mld/mac80211.c:1982:12: error: 'iwl_mld_resume' defined but not used [-Werror=unused-function]
 1982 | static int iwl_mld_resume(struct ieee80211_hw *hw)
      |            ^~~~~~~~~~~~~~
drivers/net/wireless/intel/iwlwifi/mld/mac80211.c:1960:1: error: 'iwl_mld_suspend' defined but not used [-Werror=unused-function]
 1960 | iwl_mld_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan *wowlan)
      | ^~~~~~~~~~~~~~~
drivers/net/wireless/intel/iwlwifi/mld/mac80211.c:1946:13: error: 'iwl_mld_set_wakeup' defined but not used [-Werror=unused-function]
 1946 | static void iwl_mld_set_wakeup(struct ieee80211_hw *hw, bool enabled)
      |             ^~~~~~~~~~~~~~~~~~
drivers/net/wireless/intel/iwlwifi/mld/mac80211.c: In function 'iwl_mld_mac80211_start':
drivers/net/wireless/intel/iwlwifi/mld/mac80211.c:504:20: error: 'ret' is used uninitialized [-Werror=uninitialized]
  504 |         if (!in_d3 || ret) {
      |                    ^~
drivers/net/wireless/intel/iwlwifi/mld/mac80211.c:478:13: note: 'ret' was declared here
  478 |         int ret;
      |             ^~~

Hide the unused functions and make sure the 'ret' variable is initialized
before being used.

Fixes: d1e879ec600f ("wifi: iwlwifi: add iwlmld sub-driver")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/r/202504032255.N6ptuCNG-lkp@intel.com/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
[lukas: initialize ret to 0, use static inline]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes v1 -> v2:
 Initialize ret to 0 instead of removing it from the if-clause (Miriam).
 Use static inline for iwl_mld_no_wowlan_suspend() instead of an #ifdef
 in accordance with section 21 of Documentation/process/coding-style.rst.

Link to v1:
 https://lore.kernel.org/r/20250325084340.378724-1-arnd@kernel.org/

 drivers/net/wireless/intel/iwlwifi/mld/d3.h       | 4 ++++
 drivers/net/wireless/intel/iwlwifi/mld/mac80211.c | 4 +++-
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Korenblit, Miriam Rachel April 7, 2025, 6:42 a.m. UTC | #1 
> -----Original Message-----
> From: Lukas Wunner <lukas@wunner.de>
> Sent: Sunday, 6 April 2025 16:44
> To: Korenblit, Miriam Rachel <miriam.rachel.korenblit@intel.com>; Berg,
> Johannes <johannes.berg@intel.com>; Grumbach, Emmanuel
> <emmanuel.grumbach@intel.com>; Berg, Benjamin <benjamin.berg@intel.com>;
> Ben Shimol, Yedidya <yedidya.ben.shimol@intel.com>; Arnd Bergmann
> <arnd@arndb.de>; Stern, Avraham <avraham.stern@intel.com>; Gabay, Daniel
> <daniel.gabay@intel.com>; Anjaneyulu, Pagadala Yesu
> <pagadala.yesu.anjaneyulu@intel.com>; Triebitz, Shaul
> <shaul.triebitz@intel.com>
> Cc: linux-wireless@vger.kernel.org
> Subject: [PATCH v2] wifi: iwlwifi: mld: fix building with CONFIG_PM_SLEEP
> disabled
> 
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The newly added driver causes multiple build problems when CONFIG_PM_SLEEP
> is disabled:
> 
> drivers/net/wireless/intel/iwlwifi/mld/mac80211.c:1982:12: error:
> 'iwl_mld_resume' defined but not used [-Werror=unused-function]
>  1982 | static int iwl_mld_resume(struct ieee80211_hw *hw)
>       |            ^~~~~~~~~~~~~~
> drivers/net/wireless/intel/iwlwifi/mld/mac80211.c:1960:1: error:
> 'iwl_mld_suspend' defined but not used [-Werror=unused-function]
>  1960 | iwl_mld_suspend(struct ieee80211_hw *hw, struct cfg80211_wowlan
> *wowlan)
>       | ^~~~~~~~~~~~~~~
> drivers/net/wireless/intel/iwlwifi/mld/mac80211.c:1946:13: error:
> 'iwl_mld_set_wakeup' defined but not used [-Werror=unused-function]
>  1946 | static void iwl_mld_set_wakeup(struct ieee80211_hw *hw, bool enabled)
>       |             ^~~~~~~~~~~~~~~~~~
> drivers/net/wireless/intel/iwlwifi/mld/mac80211.c: In function
> 'iwl_mld_mac80211_start':
> drivers/net/wireless/intel/iwlwifi/mld/mac80211.c:504:20: error: 'ret' is used
> uninitialized [-Werror=uninitialized]
>   504 |         if (!in_d3 || ret) {
>       |                    ^~
> drivers/net/wireless/intel/iwlwifi/mld/mac80211.c:478:13: note: 'ret' was
> declared here
>   478 |         int ret;
>       |             ^~~
> 
> Hide the unused functions and make sure the 'ret' variable is initialized before
> being used.
> 
> Fixes: d1e879ec600f ("wifi: iwlwifi: add iwlmld sub-driver")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/r/202504032255.N6ptuCNG-lkp@intel.com/
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> [lukas: initialize ret to 0, use static inline]
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> Changes v1 -> v2:
>  Initialize ret to 0 instead of removing it from the if-clause (Miriam).
>  Use static inline for iwl_mld_no_wowlan_suspend() instead of an #ifdef  in
> accordance with section 21 of Documentation/process/coding-style.rst.
> 
> Link to v1:
>  https://lore.kernel.org/r/20250325084340.378724-1-arnd@kernel.org/
> 
>  drivers/net/wireless/intel/iwlwifi/mld/d3.h       | 4 ++++
>  drivers/net/wireless/intel/iwlwifi/mld/mac80211.c | 4 +++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/mld/d3.h
> b/drivers/net/wireless/intel/iwlwifi/mld/d3.h
> index 618d6fb..4157f82 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mld/d3.h
> +++ b/drivers/net/wireless/intel/iwlwifi/mld/d3.h
> @@ -34,6 +34,7 @@ struct iwl_mld_wowlan_data {
>  	struct iwl_mld_rekey_data rekey_data;
>  };
> 
> +#ifdef CONFIG_PM_SLEEP
>  int iwl_mld_no_wowlan_resume(struct iwl_mld *mld);  int
> iwl_mld_no_wowlan_suspend(struct iwl_mld *mld);  int
> iwl_mld_wowlan_suspend(struct iwl_mld *mld, @@ -47,5 +48,8 @@ void
> iwl_mld_ipv6_addr_change(struct ieee80211_hw *hw,
>  			      struct ieee80211_vif *vif,
>  			      struct inet6_dev *idev);
>  #endif
> +#else
> +static inline int iwl_mld_no_wowlan_suspend(struct iwl_mld *mld) {
> +return 0;
You are not supposed to get to this function not under the ifdef. So if you do, I'd return an error value, not 0?
 } #endif
> 
>  #endif /* __iwl_mld_d3_h__ */
> diff --git a/drivers/net/wireless/intel/iwlwifi/mld/mac80211.c
> b/drivers/net/wireless/intel/iwlwifi/mld/mac80211.c
> index 6851064..18cd607 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mld/mac80211.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mld/mac80211.c
> @@ -475,8 +475,8 @@ int iwl_mld_register_hw(struct iwl_mld *mld)  int
> iwl_mld_mac80211_start(struct ieee80211_hw *hw)  {
>  	struct iwl_mld *mld = IWL_MAC80211_GET_MLD(hw);
> -	int ret;
>  	bool in_d3 = false;
> +	int ret = 0;
> 
>  	lockdep_assert_wiphy(mld->wiphy);
> 
> @@ -1943,6 +1943,7 @@ static void iwl_mld_sta_rc_update(struct
> ieee80211_hw *hw,
>  	}
>  }
> 
> +#ifdef CONFIG_PM_SLEEP
>  static void iwl_mld_set_wakeup(struct ieee80211_hw *hw, bool enabled)  {
>  	struct iwl_mld *mld = IWL_MAC80211_GET_MLD(hw); @@ -1994,6
> +1995,7 @@ static int iwl_mld_resume(struct ieee80211_hw *hw)
> 
>  	return 0;
>  }
> +#endif
> 
>  static int iwl_mld_alloc_ptk_pn(struct iwl_mld *mld,
>  				struct iwl_mld_sta *mld_sta,
> --
> 2.43.0
>
Lukas Wunner April 7, 2025, 7:09 a.m. UTC | #2 
On Mon, Apr 07, 2025 at 06:42:47AM +0000, Korenblit, Miriam Rachel wrote:
> > From: Lukas Wunner <lukas@wunner.de>
> > Sent: Sunday, 6 April 2025 16:44
[...]
> > --- a/drivers/net/wireless/intel/iwlwifi/mld/d3.h
> > +++ b/drivers/net/wireless/intel/iwlwifi/mld/d3.h
> > @@ -34,6 +34,7 @@ struct iwl_mld_wowlan_data {
> >  	struct iwl_mld_rekey_data rekey_data;
> >  };
> > 
> > +#ifdef CONFIG_PM_SLEEP
> >  int iwl_mld_no_wowlan_resume(struct iwl_mld *mld);  int
> > iwl_mld_no_wowlan_suspend(struct iwl_mld *mld);  int
> > iwl_mld_wowlan_suspend(struct iwl_mld *mld, @@ -47,5 +48,8 @@ void
> > iwl_mld_ipv6_addr_change(struct ieee80211_hw *hw,
> >  			      struct ieee80211_vif *vif,
> >  			      struct inet6_dev *idev);
> >  #endif
> > +#else
> > +static inline int iwl_mld_no_wowlan_suspend(struct iwl_mld *mld) {
> > +return 0;
> 
> You are not supposed to get to this function not under the ifdef.
> So if you do, I'd return an error value, not 0?

No.  This code section of iwl_mld_mac80211_stop()...

	/* if the suspend flow fails the fw is in error. Stop it here, and it
	 * will be started upon wakeup
	 */
	if (!suspend || iwl_mld_no_wowlan_suspend(mld))
		iwl_mld_stop_fw(mld);

...would unconditionally call iwl_mld_stop_fw() if the empty inline stub
of iwl_mld_no_wowlan_suspend() returned an error value.

That doesn't seem to be the desired behavior here, judging by the code
comment:  Apparently iwl_mld_stop_fw() is only supposed to be called
if the firmware "is in error".  So returning 0 is very much intentional.

Thanks,

Lukas
Korenblit, Miriam Rachel April 8, 2025, 8:56 a.m. UTC | #3 
> -----Original Message-----
> From: Lukas Wunner <lukas@wunner.de>
> Sent: Monday, 7 April 2025 10:09
> To: Korenblit, Miriam Rachel <miriam.rachel.korenblit@intel.com>
> Cc: Berg, Johannes <johannes.berg@intel.com>; Grumbach, Emmanuel
> <emmanuel.grumbach@intel.com>; Berg, Benjamin <benjamin.berg@intel.com>;
> Ben Shimol, Yedidya <yedidya.ben.shimol@intel.com>; Arnd Bergmann
> <arnd@arndb.de>; Stern, Avraham <avraham.stern@intel.com>; Gabay, Daniel
> <daniel.gabay@intel.com>; Anjaneyulu, Pagadala Yesu
> <pagadala.yesu.anjaneyulu@intel.com>; Triebitz, Shaul
> <shaul.triebitz@intel.com>; linux-wireless@vger.kernel.org
> Subject: Re: [PATCH v2] wifi: iwlwifi: mld: fix building with CONFIG_PM_SLEEP
> disabled
> 
> On Mon, Apr 07, 2025 at 06:42:47AM +0000, Korenblit, Miriam Rachel wrote:
> > > From: Lukas Wunner <lukas@wunner.de>
> > > Sent: Sunday, 6 April 2025 16:44
> [...]
> > > --- a/drivers/net/wireless/intel/iwlwifi/mld/d3.h
> > > +++ b/drivers/net/wireless/intel/iwlwifi/mld/d3.h
> > > @@ -34,6 +34,7 @@ struct iwl_mld_wowlan_data {
> > >  	struct iwl_mld_rekey_data rekey_data;  };
> > >
> > > +#ifdef CONFIG_PM_SLEEP
> > >  int iwl_mld_no_wowlan_resume(struct iwl_mld *mld);  int
> > > iwl_mld_no_wowlan_suspend(struct iwl_mld *mld);  int
> > > iwl_mld_wowlan_suspend(struct iwl_mld *mld, @@ -47,5 +48,8 @@ void
> > > iwl_mld_ipv6_addr_change(struct ieee80211_hw *hw,
> > >  			      struct ieee80211_vif *vif,
> > >  			      struct inet6_dev *idev);
> > >  #endif
> > > +#else
> > > +static inline int iwl_mld_no_wowlan_suspend(struct iwl_mld *mld) {
> > > +return 0;
> >
> > You are not supposed to get to this function not under the ifdef.
> > So if you do, I'd return an error value, not 0?
> 
> No.  This code section of iwl_mld_mac80211_stop()...
> 
> 	/* if the suspend flow fails the fw is in error. Stop it here, and it
> 	 * will be started upon wakeup
> 	 */
> 	if (!suspend || iwl_mld_no_wowlan_suspend(mld))
> 		iwl_mld_stop_fw(mld);
> 
> ...would unconditionally call iwl_mld_stop_fw() if the empty inline stub of
> iwl_mld_no_wowlan_suspend() returned an error value.
> 
> That doesn't seem to be the desired behavior here, judging by the code
> comment:  Apparently iwl_mld_stop_fw() is only supposed to be called if the
> firmware "is in error".  So returning 0 is very much intentional.
> 
> Thanks,
> 
> Lukas
Acked-by: Miri Korenblit <miriam.rachel.korenblit@intel.com>
diff mbox series

Patch

diff --git a/drivers/net/wireless/intel/iwlwifi/mld/d3.h b/drivers/net/wireless/intel/iwlwifi/mld/d3.h
index 618d6fb..4157f82 100644
--- a/drivers/net/wireless/intel/iwlwifi/mld/d3.h
+++ b/drivers/net/wireless/intel/iwlwifi/mld/d3.h
@@ -34,6 +34,7 @@  struct iwl_mld_wowlan_data {
 	struct iwl_mld_rekey_data rekey_data;
 };
 
+#ifdef CONFIG_PM_SLEEP
 int iwl_mld_no_wowlan_resume(struct iwl_mld *mld);
 int iwl_mld_no_wowlan_suspend(struct iwl_mld *mld);
 int iwl_mld_wowlan_suspend(struct iwl_mld *mld,
@@ -47,5 +48,8 @@  void iwl_mld_ipv6_addr_change(struct ieee80211_hw *hw,
 			      struct ieee80211_vif *vif,
 			      struct inet6_dev *idev);
 #endif
+#else
+static inline int iwl_mld_no_wowlan_suspend(struct iwl_mld *mld) { return 0; }
+#endif
 
 #endif /* __iwl_mld_d3_h__ */
diff --git a/drivers/net/wireless/intel/iwlwifi/mld/mac80211.c b/drivers/net/wireless/intel/iwlwifi/mld/mac80211.c
index 6851064..18cd607 100644
--- a/drivers/net/wireless/intel/iwlwifi/mld/mac80211.c
+++ b/drivers/net/wireless/intel/iwlwifi/mld/mac80211.c
@@ -475,8 +475,8 @@  int iwl_mld_register_hw(struct iwl_mld *mld)
 int iwl_mld_mac80211_start(struct ieee80211_hw *hw)
 {
 	struct iwl_mld *mld = IWL_MAC80211_GET_MLD(hw);
-	int ret;
 	bool in_d3 = false;
+	int ret = 0;
 
 	lockdep_assert_wiphy(mld->wiphy);
 
@@ -1943,6 +1943,7 @@  static void iwl_mld_sta_rc_update(struct ieee80211_hw *hw,
 	}
 }
 
+#ifdef CONFIG_PM_SLEEP
 static void iwl_mld_set_wakeup(struct ieee80211_hw *hw, bool enabled)
 {
 	struct iwl_mld *mld = IWL_MAC80211_GET_MLD(hw);
@@ -1994,6 +1995,7 @@  static int iwl_mld_resume(struct ieee80211_hw *hw)
 
 	return 0;
 }
+#endif
 
 static int iwl_mld_alloc_ptk_pn(struct iwl_mld *mld,
 				struct iwl_mld_sta *mld_sta,