diff mbox series

iwlwifi: Make missed beacon timeout configurable

Message ID 20220226045047.643695-1-mikezackles@gmail.com
State New
Headers show
Series iwlwifi: Make missed beacon timeout configurable | expand

Commit Message

Zachary Michaels Feb. 26, 2022, 4:50 a.m. UTC
Makes the beacon timeout a module parameter, allowing the original default (16
missed beacons) to be kept while also enabling users that experience problems to
increase the timeout.

For context, this patch has been in circulation for over a year as a workaround
for unstable/unusable wifi, as documented in
https://bugzilla.kernel.org/show_bug.cgi?id=203709
The linked bug report has received nearly 300 comments in the nearly three years
it has been open. During that time evidence has been presented indicating that
this is likely to be a firmware bug, but no firmware fix has been documented on
the tracker, and new issue reports continue to surface. This patch provides an
escape hatch for users that are stuck.

Signed-off-by: Zachary Michaels <mikezackles@gmail.com>
---
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c       | 4 ++++
 drivers/net/wireless/intel/iwlwifi/iwl-modparams.h | 2 ++
 drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c  | 3 ++-
 drivers/net/wireless/intel/iwlwifi/mvm/mvm.h       | 1 -
 4 files changed, 8 insertions(+), 2 deletions(-)

Comments

Dennis Baurichter Aug. 2, 2022, 6:50 p.m. UTC | #1
Hello,

the below patch was sent to linux-wireless 5 months ago, but (as far as 
I can see on lore.kernel.org) didn't get a reply, so I guess it might 
have slipped through? May I ask to consider it for inclusion?

As stated below, this is a workaround for frequent wifi disconnects 
described in https://bugzilla.kernel.org/show_bug.cgi?id=203709. As an 
example, on one laptop with Intel Dual Band Wireless AC 3160 these 
disconnects occured sporadically initially, but later became very 
frequent, e.g. around 100 disconnects within 5 hours (with the laptop 1m 
from the wifi access point). Setting the beacon_timeout parameter 
provided by the patch allowed me to completely "fix" / work around the 
problem, with no disconnects anymore. Many more examples can be found in 
the linked bug report.

Regards,
Dennis

On 2022-02-26 at 05:50, Zachary Michaels wrote:
> Makes the beacon timeout a module parameter, allowing the original default (16
> missed beacons) to be kept while also enabling users that experience problems to
> increase the timeout.
> 
> For context, this patch has been in circulation for over a year as a workaround
> for unstable/unusable wifi, as documented in
> https://bugzilla.kernel.org/show_bug.cgi?id=203709
> The linked bug report has received nearly 300 comments in the nearly three years
> it has been open. During that time evidence has been presented indicating that
> this is likely to be a firmware bug, but no firmware fix has been documented on
> the tracker, and new issue reports continue to surface. This patch provides an
> escape hatch for users that are stuck.
> 
> Signed-off-by: Zachary Michaels <mikezackles@gmail.com>
> ---
>   drivers/net/wireless/intel/iwlwifi/iwl-drv.c       | 4 ++++
>   drivers/net/wireless/intel/iwlwifi/iwl-modparams.h | 2 ++
>   drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c  | 3 ++-
>   drivers/net/wireless/intel/iwlwifi/mvm/mvm.h       | 1 -
>   4 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> index 506d05953314..c5f5787da35c 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
> @@ -1753,6 +1753,7 @@ struct iwl_mod_params iwlwifi_mod_params = {
>   	.power_level = IWL_POWER_INDEX_1,
>   	.uapsd_disable = IWL_DISABLE_UAPSD_BSS | IWL_DISABLE_UAPSD_P2P_CLIENT,
>   	.enable_ini = true,
> +	.beacon_timeout = 16,
>   	/* the rest are 0 by default */
>   };
>   IWL_EXPORT_SYMBOL(iwlwifi_mod_params);
> @@ -1868,6 +1869,9 @@ module_param_named(enable_ini, iwlwifi_mod_params.enable_ini,
>   		   bool, S_IRUGO | S_IWUSR);
>   MODULE_PARM_DESC(enable_ini,
>   		 "Enable debug INI TLV FW debug infrastructure (default: true");
> +module_param_named(beacon_timeout, iwlwifi_mod_params.beacon_timeout, uint, 0644);
> +MODULE_PARM_DESC(beacon_timeout,
> +		 "Number of missed beacons before disconnecting (default: 16)");
>   
>   /*
>    * set bt_coex_active to true, uCode will do kill/defer
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-modparams.h b/drivers/net/wireless/intel/iwlwifi/iwl-modparams.h
> index 004ebdac4535..198c5ac2575b 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-modparams.h
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-modparams.h
> @@ -62,6 +62,7 @@ enum iwl_uapsd_disable {
>    * @disable_11ac: disable VHT capabilities, default = false.
>    * @remove_when_gone: remove an inaccessible device from the PCIe bus.
>    * @enable_ini: enable new FW debug infratructure (INI TLVs)
> + * @beacon_timeout: number of missed beacons before disconnect, default = 16
>    */
>   struct iwl_mod_params {
>   	int swcrypto;
> @@ -84,6 +85,7 @@ struct iwl_mod_params {
>   	bool disable_11ax;
>   	bool remove_when_gone;
>   	bool enable_ini;
> +	u32 beacon_timeout;
>   };
>   
>   static inline bool iwl_enable_rx_ampdu(void)
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c b/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
> index fd7d4abfb454..c3d13326a203 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
> @@ -8,6 +8,7 @@
>   #include <linux/crc32.h>
>   #include <net/mac80211.h>
>   #include "iwl-io.h"
> +#include "iwl-modparams.h"
>   #include "iwl-prph.h"
>   #include "fw-api.h"
>   #include "mvm.h"
> @@ -1420,7 +1421,7 @@ void iwl_mvm_rx_missed_beacons_notif(struct iwl_mvm *mvm,
>   	 * TODO: the threshold should be adjusted based on latency conditions,
>   	 * and/or in case of a CS flow on one of the other AP vifs.
>   	 */
> -	if (rx_missed_bcon > IWL_MVM_MISSED_BEACONS_THRESHOLD_LONG)
> +	if (rx_missed_bcon > iwlwifi_mod_params.beacon_timeout)
>   		iwl_mvm_connection_loss(mvm, vif, "missed beacons");
>   	else if (rx_missed_bcon_since_rx > IWL_MVM_MISSED_BEACONS_THRESHOLD)
>   		ieee80211_beacon_loss(vif);
> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
> index da8330b5e6d5..27ccfc94474f 100644
> --- a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
> @@ -38,7 +38,6 @@
>   /* RSSI offset for WkP */
>   #define IWL_RSSI_OFFSET 50
>   #define IWL_MVM_MISSED_BEACONS_THRESHOLD 8
> -#define IWL_MVM_MISSED_BEACONS_THRESHOLD_LONG 16
>   
>   /* A TimeUnit is 1024 microsecond */
>   #define MSEC_TO_TU(_msec)	(_msec*1000/1024)
Kalle Valo Aug. 3, 2022, 4:32 a.m. UTC | #2
Dennis Baurichter <dennisba@mail.uni-paderborn.de> writes:

> Hello,
>
> the below patch was sent to linux-wireless 5 months ago, but (as far
> as I can see on lore.kernel.org) didn't get a reply, so I guess it
> might have slipped through? May I ask to consider it for inclusion?
>
> As stated below, this is a workaround for frequent wifi disconnects
> described in https://bugzilla.kernel.org/show_bug.cgi?id=203709. As an
> example, on one laptop with Intel Dual Band Wireless AC 3160 these
> disconnects occured sporadically initially, but later became very
> frequent, e.g. around 100 disconnects within 5 hours (with the laptop
> 1m from the wifi access point). Setting the beacon_timeout parameter
> provided by the patch allowed me to completely "fix" / work around the
> problem, with no disconnects anymore. Many more examples can be found
> in the linked bug report.

The patch did get to patchwork and is assigned to Gregory:

https://patchwork.kernel.org/project/linux-wireless/patch/20220226045047.643695-1-mikezackles@gmail.com/

Gregory, please take a look. This looks to be a common problem.

Not really fond of a module parameter approach, I think it should be
automatic. Why not just increase the default beacon timeout value?
Zachary Michaels Aug. 3, 2022, 3:50 p.m. UTC | #3
> Why not just increase the default beacon timeout value?

I made it a parameter because I was under the vague impression that a
larger timeout wouldn't be ideal on a well-behaved network, but this
isn't my area of expertise. I just thought this would be the most
conservative change since it would support identical default
operation.

Thanks for taking a look.
Greenman, Gregory Aug. 14, 2022, 1:05 p.m. UTC | #4
On Wed, 2022-08-03 at 08:50 -0700, Zachary Michaels wrote:
> > Why not just increase the default beacon timeout value?
> 
> I made it a parameter because I was under the vague impression that a
> larger timeout wouldn't be ideal on a well-behaved network, but this
> isn't my area of expertise. I just thought this would be the most
> conservative change since it would support identical default
> operation.
> 
> Thanks for taking a look.

I see that this issue https://bugzilla.kernel.org/show_bug.cgi?id=203709
has a long history from 2019. But, anyway, what value for
beacon_timeout solves the problem?

May I ask you to perform two experiments that could help us find
the root cause for this issue? The first is to run with a default driver and
to collect a firmware dump (here is a wiki with directions how to do that,
https://wireless.wiki.kernel.org/en/users/drivers/iwlwifi/debugging#firmware_debugging)
and the second is to load the defaut driver with modparam power_scheme=1. This will
completely disable power save and will let us know if the issue is somehow caused by
the NIC going to sleep and missing beacons.

Thanks,
Gregory
Zachary Michaels Aug. 14, 2022, 4:01 p.m. UTC | #5
On Sun, Aug 14, 2022 at 6:05 AM Greenman, Gregory
<gregory.greenman@intel.com> wrote:
>
> On Wed, 2022-08-03 at 08:50 -0700, Zachary Michaels wrote:
> > > Why not just increase the default beacon timeout value?
> >
> > I made it a parameter because I was under the vague impression that a
> > larger timeout wouldn't be ideal on a well-behaved network, but this
> > isn't my area of expertise. I just thought this would be the most
> > conservative change since it would support identical default
> > operation.
> >
> > Thanks for taking a look.
>
> I see that this issue https://bugzilla.kernel.org/show_bug.cgi?id=203709
> has a long history from 2019. But, anyway, what value for
> beacon_timeout solves the problem?

It seems that most people have had success with 256, but that number
may be somewhat arbitrary.

> May I ask you to perform two experiments that could help us find
> the root cause for this issue? The first is to run with a default driver and
> to collect a firmware dump (here is a wiki with directions how to do that,
> https://wireless.wiki.kernel.org/en/users/drivers/iwlwifi/debugging#firmware_debugging)
> and the second is to load the defaut driver with modparam power_scheme=1. This will
> completely disable power save and will let us know if the issue is somehow caused by
> the NIC going to sleep and missing beacons.
>
> Thanks,
> Gregory

Unfortunately I am no longer experiencing this issue, but someone else
on this chain may be able to help. (Someone having problems requested
that I submit the patch.)

There may be some relevant information in the linked bug report if you
care to sift through it. There are reports that turning off power save
did not work, and there are also some firmware dumps.

Best,
-Zach
diff mbox series

Patch

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index 506d05953314..c5f5787da35c 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -1753,6 +1753,7 @@  struct iwl_mod_params iwlwifi_mod_params = {
 	.power_level = IWL_POWER_INDEX_1,
 	.uapsd_disable = IWL_DISABLE_UAPSD_BSS | IWL_DISABLE_UAPSD_P2P_CLIENT,
 	.enable_ini = true,
+	.beacon_timeout = 16,
 	/* the rest are 0 by default */
 };
 IWL_EXPORT_SYMBOL(iwlwifi_mod_params);
@@ -1868,6 +1869,9 @@  module_param_named(enable_ini, iwlwifi_mod_params.enable_ini,
 		   bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(enable_ini,
 		 "Enable debug INI TLV FW debug infrastructure (default: true");
+module_param_named(beacon_timeout, iwlwifi_mod_params.beacon_timeout, uint, 0644);
+MODULE_PARM_DESC(beacon_timeout,
+		 "Number of missed beacons before disconnecting (default: 16)");
 
 /*
  * set bt_coex_active to true, uCode will do kill/defer
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-modparams.h b/drivers/net/wireless/intel/iwlwifi/iwl-modparams.h
index 004ebdac4535..198c5ac2575b 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-modparams.h
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-modparams.h
@@ -62,6 +62,7 @@  enum iwl_uapsd_disable {
  * @disable_11ac: disable VHT capabilities, default = false.
  * @remove_when_gone: remove an inaccessible device from the PCIe bus.
  * @enable_ini: enable new FW debug infratructure (INI TLVs)
+ * @beacon_timeout: number of missed beacons before disconnect, default = 16
  */
 struct iwl_mod_params {
 	int swcrypto;
@@ -84,6 +85,7 @@  struct iwl_mod_params {
 	bool disable_11ax;
 	bool remove_when_gone;
 	bool enable_ini;
+	u32 beacon_timeout;
 };
 
 static inline bool iwl_enable_rx_ampdu(void)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c b/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
index fd7d4abfb454..c3d13326a203 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac-ctxt.c
@@ -8,6 +8,7 @@ 
 #include <linux/crc32.h>
 #include <net/mac80211.h>
 #include "iwl-io.h"
+#include "iwl-modparams.h"
 #include "iwl-prph.h"
 #include "fw-api.h"
 #include "mvm.h"
@@ -1420,7 +1421,7 @@  void iwl_mvm_rx_missed_beacons_notif(struct iwl_mvm *mvm,
 	 * TODO: the threshold should be adjusted based on latency conditions,
 	 * and/or in case of a CS flow on one of the other AP vifs.
 	 */
-	if (rx_missed_bcon > IWL_MVM_MISSED_BEACONS_THRESHOLD_LONG)
+	if (rx_missed_bcon > iwlwifi_mod_params.beacon_timeout)
 		iwl_mvm_connection_loss(mvm, vif, "missed beacons");
 	else if (rx_missed_bcon_since_rx > IWL_MVM_MISSED_BEACONS_THRESHOLD)
 		ieee80211_beacon_loss(vif);
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
index da8330b5e6d5..27ccfc94474f 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
@@ -38,7 +38,6 @@ 
 /* RSSI offset for WkP */
 #define IWL_RSSI_OFFSET 50
 #define IWL_MVM_MISSED_BEACONS_THRESHOLD 8
-#define IWL_MVM_MISSED_BEACONS_THRESHOLD_LONG 16
 
 /* A TimeUnit is 1024 microsecond */
 #define MSEC_TO_TU(_msec)	(_msec*1000/1024)