[PATCH 1/2] wifi: ath9k: work around AR_CFG 0xdeadbeef chip hang

[PATCH 1/2] wifi: ath9k: work around AR_CFG 0xdeadbeef chip hang

[Issam Hamdi]

From: Simon Wunderlich <simon.wunderlich@open-mesh.com>

QCA 802.11n chips (especially AR9330/AR9340) sometimes end up in a state in
which a read of AR_CFG always returns 0xdeadbeef. This should not happen
when when the power_mode of the device is ATH9K_PM_AWAKE.

This problem is not yet detected by any other workaround in ath9k. No way
is known to reproduce the problem easily.

This patch originally developed by "Simon Wunderlich <simon.wunderlich@open-mesh.com>"
and "Sven Eckelmann <sven.eckelmann@open-mesh.com>"

Co-developed-by: Simon Wunderlich <sw@simonwunderlich.de>
Co-developed-by: Sven Eckelmann <se@simonwunderlich.de>
Signed-off-by: Issam Hamdi <ih@simonwunderlich.de>
---
 drivers/net/wireless/ath/ath9k/ath9k.h |  3 +++
 drivers/net/wireless/ath/ath9k/debug.c |  1 +
 drivers/net/wireless/ath/ath9k/debug.h |  1 +
 drivers/net/wireless/ath/ath9k/init.c  |  1 +
 drivers/net/wireless/ath/ath9k/link.c  | 31 ++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath9k/main.c  |  4 ++++
 6 files changed, 41 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 29ca65a732a6..c1ce081445a9 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -739,11 +739,13 @@ void ath9k_csa_update(struct ath_softc *sc);
 #define ATH_ANI_MAX_SKIP_COUNT    10
 #define ATH_PAPRD_TIMEOUT         100 /* msecs */
 #define ATH_PLL_WORK_INTERVAL     100
+#define ATH_HANG_WORK_INTERVAL    4000
 
 void ath_hw_check_work(struct work_struct *work);
 void ath_reset_work(struct work_struct *work);
 bool ath_hw_check(struct ath_softc *sc);
 void ath_hw_pll_work(struct work_struct *work);
+void ath_hw_hang_work(struct work_struct *work);
 void ath_paprd_calibrate(struct work_struct *work);
 void ath_ani_calibrate(struct timer_list *t);
 void ath_start_ani(struct ath_softc *sc);
@@ -1044,6 +1046,7 @@ struct ath_softc {
 #endif
 	struct delayed_work hw_check_work;
 	struct delayed_work hw_pll_work;
+	struct delayed_work hw_hang_work;
 	struct timer_list sleep_timer;
 
 #ifdef CONFIG_ATH9K_BTCOEX_SUPPORT
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index eff894958a73..6b2469a01f17 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -750,6 +750,7 @@ static int read_file_reset(struct seq_file *file, void *data)
 		[RESET_TYPE_CALIBRATION] = "Calibration error",
 		[RESET_TX_DMA_ERROR] = "Tx DMA stop error",
 		[RESET_RX_DMA_ERROR] = "Rx DMA stop error",
+		[RESET_TYPE_DEADBEEF] = "deadbeef hang",
 	};
 	int i;
 
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 389459c04d14..6ebb6053a8c1 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -53,6 +53,7 @@ enum ath_reset_type {
 	RESET_TYPE_CALIBRATION,
 	RESET_TX_DMA_ERROR,
 	RESET_RX_DMA_ERROR,
+	RESET_TYPE_DEADBEEF,
 	__RESET_TYPE_MAX
 };
 
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index f9e77c4624d9..833474d7281f 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -740,6 +740,7 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 	INIT_WORK(&sc->paprd_work, ath_paprd_calibrate);
 	INIT_DELAYED_WORK(&sc->hw_pll_work, ath_hw_pll_work);
 	INIT_DELAYED_WORK(&sc->hw_check_work, ath_hw_check_work);
+	INIT_DELAYED_WORK(&sc->hw_hang_work, ath_hw_hang_work);
 
 	ath9k_init_channel_context(sc);
 
diff --git a/drivers/net/wireless/ath/ath9k/link.c b/drivers/net/wireless/ath/ath9k/link.c
index d1e5767aab3c..37438960c278 100644
--- a/drivers/net/wireless/ath/ath9k/link.c
+++ b/drivers/net/wireless/ath/ath9k/link.c
@@ -142,6 +142,37 @@ void ath_hw_pll_work(struct work_struct *work)
 				     msecs_to_jiffies(ATH_PLL_WORK_INTERVAL));
 }
 
+static bool ath_hw_hang_deadbeef(struct ath_softc *sc)
+{
+	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	u32 reg;
+
+	/* check for stucked MAC */
+	ath9k_ps_wakeup(sc);
+	reg = REG_READ(sc->sc_ah, AR_CFG);
+	ath9k_ps_restore(sc);
+
+	if (reg != 0xdeadbeef)
+		return false;
+
+	ath_dbg(common, RESET,
+		"0xdeadbeef hang is detected. Schedule chip reset\n");
+	ath9k_queue_reset(sc, RESET_TYPE_DEADBEEF);
+
+	return true;
+}
+
+void ath_hw_hang_work(struct work_struct *work)
+{
+	struct ath_softc *sc = container_of(work, struct ath_softc,
+					    hw_hang_work.work);
+
+	ath_hw_hang_deadbeef(sc);
+
+	ieee80211_queue_delayed_work(sc->hw, &sc->hw_hang_work,
+				     msecs_to_jiffies(ATH_HANG_WORK_INTERVAL));
+}
+
 /*
  * PA Pre-distortion.
  */
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index b92c89dad8de..024028ce8417 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -186,6 +186,7 @@ static void __ath_cancel_work(struct ath_softc *sc)
 	cancel_work_sync(&sc->paprd_work);
 	cancel_delayed_work_sync(&sc->hw_check_work);
 	cancel_delayed_work_sync(&sc->hw_pll_work);
+	cancel_delayed_work_sync(&sc->hw_hang_work);
 
 #ifdef CONFIG_ATH9K_BTCOEX_SUPPORT
 	if (ath9k_hw_mci_is_enabled(sc->sc_ah))
@@ -208,6 +209,9 @@ void ath_restart_work(struct ath_softc *sc)
 		ieee80211_queue_delayed_work(sc->hw, &sc->hw_pll_work,
 				     msecs_to_jiffies(ATH_PLL_WORK_INTERVAL));
 
+	ieee80211_queue_delayed_work(sc->hw, &sc->hw_hang_work,
+				     msecs_to_jiffies(ATH_HANG_WORK_INTERVAL));
+
 	ath_start_ani(sc);
 }
 

base-commit: 2b94751626a6d49bbe42a19cc1503bd391016bd5
-- 
2.39.2

[PATCH 2/2] wifi: ath9k: Reset chip on potential deaf state

[Issam Hamdi]

From: Simon Wunderlich <simon.wunderlich@open-mesh.com>

The chip is switching seemingly random into a state which can be described
as "deaf". No or nearly no interrupts are generated anymore for incoming
packets. Existing links either break down after a while and new links will
not be established.

The driver doesn't know if there is no other device available or if it
ended up in an "deaf" state. Resetting the chip proactively avoids
permanent problems in case the chip really was in its "deaf" state but
maybe causes unnecessary resets in case it wasn't "deaf".

This patch originally developed by "Simon Wunderlich <simon.wunderlich@open-mesh.com>"
and "Sven Eckelmann <sven.eckelmann@open-mesh.com>"

Co-developed-by: Simon Wunderlich <sw@simonwunderlich.de>
Co-developed-by: Sven Eckelmann <se@simonwunderlich.de>
Signed-off-by: Issam Hamdi <ih@simonwunderlich.de>
---
 drivers/net/wireless/ath/ath9k/ath9k.h |  3 ++
 drivers/net/wireless/ath/ath9k/debug.c |  1 +
 drivers/net/wireless/ath/ath9k/debug.h |  1 +
 drivers/net/wireless/ath/ath9k/link.c  | 48 +++++++++++++++++++++++++-
 4 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index c1ce081445a9..2b98c69fa37f 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -1026,6 +1026,9 @@ struct ath_softc {
 	short nbcnvifs;
 	unsigned long ps_usecount;
 
+	unsigned long last_check_time;
+	u32 last_check_interrupts;
+
 	struct ath_rx rx;
 	struct ath_tx tx;
 	struct ath_beacon beacon;
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index 6b2469a01f17..4128cf691166 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -751,6 +751,7 @@ static int read_file_reset(struct seq_file *file, void *data)
 		[RESET_TX_DMA_ERROR] = "Tx DMA stop error",
 		[RESET_RX_DMA_ERROR] = "Rx DMA stop error",
 		[RESET_TYPE_DEADBEEF] = "deadbeef hang",
+		[RESET_TYPE_DEAF] = "deaf hang",
 	};
 	int i;
 
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 6ebb6053a8c1..76e27860455c 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -54,6 +54,7 @@ enum ath_reset_type {
 	RESET_TX_DMA_ERROR,
 	RESET_RX_DMA_ERROR,
 	RESET_TYPE_DEADBEEF,
+	RESET_TYPE_DEAF,
 	__RESET_TYPE_MAX
 };
 
diff --git a/drivers/net/wireless/ath/ath9k/link.c b/drivers/net/wireless/ath/ath9k/link.c
index 37438960c278..d1762cc3129d 100644
--- a/drivers/net/wireless/ath/ath9k/link.c
+++ b/drivers/net/wireless/ath/ath9k/link.c
@@ -162,13 +162,59 @@ static bool ath_hw_hang_deadbeef(struct ath_softc *sc)
 	return true;
 }
 
+static bool ath_hw_hang_deaf(struct ath_softc *sc)
+{
+#if !defined(CPTCFG_ATH9K_DEBUGFS) || defined(CPTCFG_ATH9K_TX99)
+	return false;
+#else
+	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	u32 interrupts, interrupt_per_s;
+	unsigned int interval;
+
+	/* get historic data */
+	interval = jiffies_to_msecs(jiffies - sc->last_check_time);
+	if (sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA)
+		interrupts = sc->debug.stats.istats.rxlp;
+	else
+		interrupts = sc->debug.stats.istats.rxok;
+
+	interrupts -= sc->last_check_interrupts;
+
+	/* save current data */
+	sc->last_check_time = jiffies;
+	if (sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA)
+		sc->last_check_interrupts = sc->debug.stats.istats.rxlp;
+	else
+		sc->last_check_interrupts = sc->debug.stats.istats.rxok;
+
+	/* sanity check, should be 4 seconds */
+	if (interval > 10000 || interval < 1000)
+		return false;
+
+	/* should be at least one interrupt per second */
+	interrupt_per_s = interrupts / (interval / 1000);
+	if (interrupt_per_s >= 1)
+		return false;
+
+	ath_dbg(common, RESET,
+		"RX deaf hang is detected. Schedule chip reset\n");
+	ath9k_queue_reset(sc, RESET_TYPE_DEAF);
+
+	return true;
+#endif
+}
+
 void ath_hw_hang_work(struct work_struct *work)
 {
 	struct ath_softc *sc = container_of(work, struct ath_softc,
 					    hw_hang_work.work);
 
-	ath_hw_hang_deadbeef(sc);
+	if (ath_hw_hang_deadbeef(sc))
+		goto requeue_worker;
 
+	ath_hw_hang_deaf(sc);
+
+requeue_worker:
 	ieee80211_queue_delayed_work(sc->hw, &sc->hw_hang_work,
 				     msecs_to_jiffies(ATH_HANG_WORK_INTERVAL));
 }
-- 
2.39.2

Re: [PATCH 2/2] wifi: ath9k: Reset chip on potential deaf state

[Kalle Valo]

Issam Hamdi <ih@simonwunderlich.de> writes:

> From: Simon Wunderlich <simon.wunderlich@open-mesh.com>
>
> The chip is switching seemingly random into a state which can be described
> as "deaf". No or nearly no interrupts are generated anymore for incoming
> packets. Existing links either break down after a while and new links will
> not be established.
>
> The driver doesn't know if there is no other device available or if it
> ended up in an "deaf" state. Resetting the chip proactively avoids
> permanent problems in case the chip really was in its "deaf" state but
> maybe causes unnecessary resets in case it wasn't "deaf".
>
> This patch originally developed by "Simon Wunderlich <simon.wunderlich@open-mesh.com>"
> and "Sven Eckelmann <sven.eckelmann@open-mesh.com>"
>
> Co-developed-by: Simon Wunderlich <sw@simonwunderlich.de>
> Co-developed-by: Sven Eckelmann <se@simonwunderlich.de>
> Signed-off-by: Issam Hamdi <ih@simonwunderlich.de>

s-o-b missing here as well.

> --- a/drivers/net/wireless/ath/ath9k/link.c
> +++ b/drivers/net/wireless/ath/ath9k/link.c
> @@ -162,13 +162,59 @@ static bool ath_hw_hang_deadbeef(struct ath_softc *sc)
>  	return true;
>  }
>  
> +static bool ath_hw_hang_deaf(struct ath_softc *sc)
> +{
> +#if !defined(CPTCFG_ATH9K_DEBUGFS) || defined(CPTCFG_ATH9K_TX99)
> +	return false;
> +#else

We don't use CPTCFG_ in upstream.

Try to avoid '#if' if possible, what about IS_ENABLED():

if (!IS_ENABLED(CONFIG_ATH9K_DEBUGFS) || !IS_ENABLED(CONFIG_ATH9K_TX99)
	return false;

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 2/2] wifi: ath9k: Reset chip on potential deaf state

[Toke Høiland-Jørgensen]

Issam Hamdi <ih@simonwunderlich.de> writes:

> From: Simon Wunderlich <simon.wunderlich@open-mesh.com>
>
> The chip is switching seemingly random into a state which can be described
> as "deaf". No or nearly no interrupts are generated anymore for incoming
> packets. Existing links either break down after a while and new links will
> not be established.
>
> The driver doesn't know if there is no other device available or if it
> ended up in an "deaf" state. Resetting the chip proactively avoids
> permanent problems in case the chip really was in its "deaf" state but
> maybe causes unnecessary resets in case it wasn't "deaf".

Proactively resetting the device if there is no traffic on the network
for four seconds seems like a tad aggressive. Do you have any
information on under which conditions this actually happens in practice?
I assume this is a patch that has been lying around in openwrt for a
while, or something?

As for the code itself, see below:

> This patch originally developed by "Simon Wunderlich <simon.wunderlich@open-mesh.com>"
> and "Sven Eckelmann <sven.eckelmann@open-mesh.com>"
>
> Co-developed-by: Simon Wunderlich <sw@simonwunderlich.de>
> Co-developed-by: Sven Eckelmann <se@simonwunderlich.de>
> Signed-off-by: Issam Hamdi <ih@simonwunderlich.de>
> ---
>  drivers/net/wireless/ath/ath9k/ath9k.h |  3 ++
>  drivers/net/wireless/ath/ath9k/debug.c |  1 +
>  drivers/net/wireless/ath/ath9k/debug.h |  1 +
>  drivers/net/wireless/ath/ath9k/link.c  | 48 +++++++++++++++++++++++++-
>  4 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
> index c1ce081445a9..2b98c69fa37f 100644
> --- a/drivers/net/wireless/ath/ath9k/ath9k.h
> +++ b/drivers/net/wireless/ath/ath9k/ath9k.h
> @@ -1026,6 +1026,9 @@ struct ath_softc {
>  	short nbcnvifs;
>  	unsigned long ps_usecount;
>  
> +	unsigned long last_check_time;
> +	u32 last_check_interrupts;
> +
>  	struct ath_rx rx;
>  	struct ath_tx tx;
>  	struct ath_beacon beacon;
> diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
> index 6b2469a01f17..4128cf691166 100644
> --- a/drivers/net/wireless/ath/ath9k/debug.c
> +++ b/drivers/net/wireless/ath/ath9k/debug.c
> @@ -751,6 +751,7 @@ static int read_file_reset(struct seq_file *file, void *data)
>  		[RESET_TX_DMA_ERROR] = "Tx DMA stop error",
>  		[RESET_RX_DMA_ERROR] = "Rx DMA stop error",
>  		[RESET_TYPE_DEADBEEF] = "deadbeef hang",
> +		[RESET_TYPE_DEAF] = "deaf hang",
>  	};
>  	int i;
>  
> diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
> index 6ebb6053a8c1..76e27860455c 100644
> --- a/drivers/net/wireless/ath/ath9k/debug.h
> +++ b/drivers/net/wireless/ath/ath9k/debug.h
> @@ -54,6 +54,7 @@ enum ath_reset_type {
>  	RESET_TX_DMA_ERROR,
>  	RESET_RX_DMA_ERROR,
>  	RESET_TYPE_DEADBEEF,
> +	RESET_TYPE_DEAF,
>  	__RESET_TYPE_MAX
>  };
>  
> diff --git a/drivers/net/wireless/ath/ath9k/link.c b/drivers/net/wireless/ath/ath9k/link.c
> index 37438960c278..d1762cc3129d 100644
> --- a/drivers/net/wireless/ath/ath9k/link.c
> +++ b/drivers/net/wireless/ath/ath9k/link.c
> @@ -162,13 +162,59 @@ static bool ath_hw_hang_deadbeef(struct ath_softc *sc)
>  	return true;
>  }
>  
> +static bool ath_hw_hang_deaf(struct ath_softc *sc)
> +{
> +#if !defined(CPTCFG_ATH9K_DEBUGFS) || defined(CPTCFG_ATH9K_TX99)
> +	return false;
> +#else
> +	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
> +	u32 interrupts, interrupt_per_s;
> +	unsigned int interval;
> +
> +	/* get historic data */
> +	interval = jiffies_to_msecs(jiffies - sc->last_check_time);
> +	if (sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA)
> +		interrupts = sc->debug.stats.istats.rxlp;
> +	else
> +		interrupts = sc->debug.stats.istats.rxok;
> +
> +	interrupts -= sc->last_check_interrupts;

Relying on the debugfs counters for this seems like an odd roundabout
way of going about things. Why not just record the last time an RX
interrupt was received directly in the interrupt handler code, and then
have the watchdog check if that time was too far in the past?

Recording both TX and RX times may even help distinguish between 'deaf'
and 'idle' (cf the comment above): if we transmitted something, but got
no RX, that's a good indication of the deaf state; but if nothing
happened in either direction, it's probably just the network that's
idle. I think? :)

-Toke

Re: [PATCH 2/2] wifi: ath9k: Reset chip on potential deaf state

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 2260 bytes --]

On Tuesday, November 5, 2024 2:02:31 PM CET Toke Høiland-Jørgensen wrote:
> Issam Hamdi <ih@simonwunderlich.de> writes:
> > From: Simon Wunderlich <simon.wunderlich@open-mesh.com>
> > 
> > The chip is switching seemingly random into a state which can be described
> > as "deaf". No or nearly no interrupts are generated anymore for incoming
> > packets. Existing links either break down after a while and new links will
> > not be established.
> > 
> > The driver doesn't know if there is no other device available or if it
> > ended up in an "deaf" state. Resetting the chip proactively avoids
> > permanent problems in case the chip really was in its "deaf" state but
> > maybe causes unnecessary resets in case it wasn't "deaf".
> 
> Proactively resetting the device if there is no traffic on the network
> for four seconds seems like a tad aggressive. Do you have any
> information on under which conditions this actually happens in practice?
> I assume this is a patch that has been lying around in openwrt for a
> while, or something?

Hi Toke,

this patch has been around for a long time (8 years or so?), and it has been 
integrated in various vendor firmwares (at least three I know of) as well as 
mesh community firmwares [1]. The circumstances leading to this "deafness" is 
still unclear, but we see that some particular chips (especially 2-stream 11n 
SoCs, but also others) can go 'deaf' when running AP or mesh (or both) after 
some time. It's probably a hardware issue, and doing a channel scan to trigger 
a chip reset (which one normally can't do on an AP interface) recovers the 
hardware. This patch provides a workaround within the kernel.

We submitted it only as RFC back then [2], and since we had colleagues 
suffering the same problem again we though we give it another shot to finally 
get it integrated upstream. :)

The idea is that if the radio is idle anyway, a quick reset (which takes a few 
tens of ms maximum) doesn't hurt much, and it helps to recover non-functional 
APs or mesh points.

Cheers,
      Simon

[1] https://github.com/freifunk-gluon/gluon/pull/2114
[2] https://patchwork.kernel.org/project/linux-wireless/patch/
20161117083614.19188-2-sven.eckelmann@open-mesh.com/

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] wifi: ath9k: Reset chip on potential deaf state

[Toke Høiland-Jørgensen]

Simon Wunderlich <sw@simonwunderlich.de> writes:

> On Tuesday, November 5, 2024 2:02:31 PM CET Toke Høiland-Jørgensen wrote:
>> Issam Hamdi <ih@simonwunderlich.de> writes:
>> > From: Simon Wunderlich <simon.wunderlich@open-mesh.com>
>> > 
>> > The chip is switching seemingly random into a state which can be described
>> > as "deaf". No or nearly no interrupts are generated anymore for incoming
>> > packets. Existing links either break down after a while and new links will
>> > not be established.
>> > 
>> > The driver doesn't know if there is no other device available or if it
>> > ended up in an "deaf" state. Resetting the chip proactively avoids
>> > permanent problems in case the chip really was in its "deaf" state but
>> > maybe causes unnecessary resets in case it wasn't "deaf".
>> 
>> Proactively resetting the device if there is no traffic on the network
>> for four seconds seems like a tad aggressive. Do you have any
>> information on under which conditions this actually happens in practice?
>> I assume this is a patch that has been lying around in openwrt for a
>> while, or something?
>
> Hi Toke,
>
> this patch has been around for a long time (8 years or so?), and it has been 
> integrated in various vendor firmwares (at least three I know of) as well as 
> mesh community firmwares [1]. The circumstances leading to this "deafness" is 
> still unclear, but we see that some particular chips (especially 2-stream 11n 
> SoCs, but also others) can go 'deaf' when running AP or mesh (or both) after 
> some time. It's probably a hardware issue, and doing a channel scan to trigger 
> a chip reset (which one normally can't do on an AP interface) recovers the 
> hardware. This patch provides a workaround within the kernel.
>
> We submitted it only as RFC back then [2], and since we had colleagues 
> suffering the same problem again we though we give it another shot to finally 
> get it integrated upstream. :)
>
> The idea is that if the radio is idle anyway, a quick reset (which takes a few 
> tens of ms maximum) doesn't hurt much, and it helps to recover non-functional 
> APs or mesh points.

Alright, thanks for the extra context (would have been nice if this was
part of the commit message in the initial submission ;)).

(including both your emails in one):

> Forgot to comment here: On the AR934x hardware we worked on in the very 
> beginning, we actually still had a few interrupts coming even if the hardware 
> was 'deaf'. This why we did not implement it with a timer, but counted the 
> number of interrupts for a given time and compared it to a minimum expected 
> ratio, as done in this patch.
>
> I understand your argument for the TX part, but I think it actually breaks the 
> AP mode and prevents the recovery: if we can't hear any clients, they will not 
> use the Internet and the AP has not much to TX either. So an already deaf AP 
> has nothing to transmit just as an idle AP, but for a different reason ...

Right, okay. I guess that is also why you prefer this one to Felix'
similar patch that was also linked from that gluon issue[0]?

However, I still don't like tying this to the debugfs: if this is
something that the driver needs to react to, it should not depend on
debug features. Even if OpenWrt and derivatives always compile-in the
debugfs, not everyone does, as we discovered back when we accidentally
broke the driver when it wasn't there :)

So how about something like the patch below - it keeps the "average per
time interval" behaviour, but uses the same approach as Felix' patch to
avoid relying on debugfs. WDYT?

-Toke


[0] https://github.com/freifunk-gluon/gluon/commit/fa0740cca4683f90bbf05157fd80109d2c40aa84



diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 29ca65a732a6..bcfc8df0efe5 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -1018,6 +1018,8 @@ struct ath_softc {
 
 	u8 gtt_cnt;
 	u32 intrstatus;
+	u32 rx_active_check_time;
+	u32 rx_active_count;
 	u16 ps_flags; /* PS_* */
 	bool ps_enabled;
 	bool ps_idle;
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index 51abc470125b..a1e3bcf796f2 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -750,6 +750,7 @@ static int read_file_reset(struct seq_file *file, void *data)
 		[RESET_TYPE_CALIBRATION] = "Calibration error",
 		[RESET_TX_DMA_ERROR] = "Tx DMA stop error",
 		[RESET_RX_DMA_ERROR] = "Rx DMA stop error",
+		[RESET_TYPE_RX_INACTIVE] = "Rx path inactive",
 	};
 	int i;
 
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 389459c04d14..cb3e75969875 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -53,6 +53,7 @@ enum ath_reset_type {
 	RESET_TYPE_CALIBRATION,
 	RESET_TX_DMA_ERROR,
 	RESET_RX_DMA_ERROR,
+	RESET_TYPE_RX_INACTIVE,
 	__RESET_TYPE_MAX
 };
 
diff --git a/drivers/net/wireless/ath/ath9k/link.c b/drivers/net/wireless/ath/ath9k/link.c
index d1e5767aab3c..054c0781287e 100644
--- a/drivers/net/wireless/ath/ath9k/link.c
+++ b/drivers/net/wireless/ath/ath9k/link.c
@@ -50,7 +50,36 @@ static bool ath_tx_complete_check(struct ath_softc *sc)
 		"tx hung, resetting the chip\n");
 	ath9k_queue_reset(sc, RESET_TYPE_TX_HANG);
 	return false;
+}
+
+#define RX_INACTIVE_CHECK_INTERVAL (4 * MSEC_PER_SEC)
+
+static bool ath_hw_rx_inactive_check(struct ath_softc *sc)
+{
+	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	u32 interval, count;
+
+	interval = jiffies_to_msecs(jiffies - sc->rx_active_check_time);
+	count = sc->rx_active_count;
+
+	if (interval < RX_INACTIVE_CHECK_INTERVAL)
+		return true; /* too soon to check */
 
+	sc->rx_active_count = 0;
+	sc->rx_active_check_time = jiffies;
+
+	/* Need at least one interrupt per second, and we should only react if
+	 * we are within a factor two of the expected interval
+	 */
+	if (interval > RX_INACTIVE_CHECK_INTERVAL * 2 ||
+	    count >= interval / MSEC_PER_SEC)
+		return true;
+
+	ath_dbg(common, RESET,
+		"RX inactivity is detected. Schedule chip reset\n");
+	ath9k_queue_reset(sc, RESET_TYPE_RX_INACTIVE);
+
+	return false;
 }
 
 void ath_hw_check_work(struct work_struct *work)
@@ -58,8 +87,8 @@ void ath_hw_check_work(struct work_struct *work)
 	struct ath_softc *sc = container_of(work, struct ath_softc,
 					    hw_check_work.work);
 
-	if (!ath_hw_check(sc) ||
-	    !ath_tx_complete_check(sc))
+	if (!ath_hw_check(sc) || !ath_tx_complete_check(sc) ||
+	    !ath_hw_rx_inactive_check(sc))
 		return;
 
 	ieee80211_queue_delayed_work(sc->hw, &sc->hw_check_work,
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index b92c89dad8de..998f717a1a86 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -453,6 +453,7 @@ void ath9k_tasklet(struct tasklet_struct *t)
 			ath_rx_tasklet(sc, 0, true);
 
 		ath_rx_tasklet(sc, 0, false);
+		sc->rx_active_count++;
 	}
 
 	if (status & ATH9K_INT_TX) {

Re: [PATCH 2/2] wifi: ath9k: Reset chip on potential deaf state

[Hamdi Issam]

On 11/5/24 16:10, Toke Høiland-Jørgensen wrote:
> Right, okay. I guess that is also why you prefer this one to Felix'
> similar patch that was also linked from that gluon issue[0]?
>
> However, I still don't like tying this to the debugfs: if this is
> something that the driver needs to react to, it should not depend on
> debug features. Even if OpenWrt and derivatives always compile-in the
> debugfs, not everyone does, as we discovered back when we accidentally
> broke the driver when it wasn't there :)
>
> So how about something like the patch below - it keeps the "average per
> time interval" behaviour, but uses the same approach as Felix' patch to
> avoid relying on debugfs. WDYT?
Yes, you are right, I will update the patch to not depend on debugfs.

Regards,

Issam

Re: [PATCH 2/2] wifi: ath9k: Reset chip on potential deaf state

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

On Tuesday, November 5, 2024 2:02:31 PM CET Toke Høiland-Jørgensen wrote:
> Relying on the debugfs counters for this seems like an odd roundabout
> way of going about things. Why not just record the last time an RX
> interrupt was received directly in the interrupt handler code, and then
> have the watchdog check if that time was too far in the past?
> 
> Recording both TX and RX times may even help distinguish between 'deaf'
> and 'idle' (cf the comment above): if we transmitted something, but got
> no RX, that's a good indication of the deaf state; but if nothing
> happened in either direction, it's probably just the network that's
> idle. I think? 
> 
> -Toke

Forgot to comment here: On the AR934x hardware we worked on in the very 
beginning, we actually still had a few interrupts coming even if the hardware 
was 'deaf'. This why we did not implement it with a timer, but counted the 
number of interrupts for a given time and compared it to a minimum expected 
ratio, as done in this patch.

I understand your argument for the TX part, but I think it actually breaks the 
AP mode and prevents the recovery: if we can't hear any clients, they will not 
use the Internet and the AP has not much to TX either. So an already deaf AP 
has nothing to transmit just as an idle AP, but for a different reason ...

Cheers,
       Simon

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] wifi: ath9k: work around AR_CFG 0xdeadbeef chip hang

[Kalle Valo]

Issam Hamdi <ih@simonwunderlich.de> writes:

> From: Simon Wunderlich <simon.wunderlich@open-mesh.com>
>
> QCA 802.11n chips (especially AR9330/AR9340) sometimes end up in a state in
> which a read of AR_CFG always returns 0xdeadbeef. This should not happen
> when when the power_mode of the device is ATH9K_PM_AWAKE.
>
> This problem is not yet detected by any other workaround in ath9k. No way
> is known to reproduce the problem easily.
>
> This patch originally developed by "Simon Wunderlich <simon.wunderlich@open-mesh.com>"
> and "Sven Eckelmann <sven.eckelmann@open-mesh.com>"
>
> Co-developed-by: Simon Wunderlich <sw@simonwunderlich.de>
> Co-developed-by: Sven Eckelmann <se@simonwunderlich.de>
> Signed-off-by: Issam Hamdi <ih@simonwunderlich.de>

s-o-b missing from Simon and Sven, more info:

https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/2] wifi: ath9k: work around AR_CFG 0xdeadbeef chip hang

[Toke Høiland-Jørgensen]

Issam Hamdi <ih@simonwunderlich.de> writes:

> From: Simon Wunderlich <simon.wunderlich@open-mesh.com>
>
> QCA 802.11n chips (especially AR9330/AR9340) sometimes end up in a state in
> which a read of AR_CFG always returns 0xdeadbeef. This should not happen
> when when the power_mode of the device is ATH9K_PM_AWAKE.
>
> This problem is not yet detected by any other workaround in ath9k. No way
> is known to reproduce the problem easily.
>
> This patch originally developed by "Simon Wunderlich <simon.wunderlich@open-mesh.com>"
> and "Sven Eckelmann <sven.eckelmann@open-mesh.com>"
>
> Co-developed-by: Simon Wunderlich <sw@simonwunderlich.de>
> Co-developed-by: Sven Eckelmann <se@simonwunderlich.de>
> Signed-off-by: Issam Hamdi <ih@simonwunderlich.de>
> ---
>  drivers/net/wireless/ath/ath9k/ath9k.h |  3 +++
>  drivers/net/wireless/ath/ath9k/debug.c |  1 +
>  drivers/net/wireless/ath/ath9k/debug.h |  1 +
>  drivers/net/wireless/ath/ath9k/init.c  |  1 +
>  drivers/net/wireless/ath/ath9k/link.c  | 31 ++++++++++++++++++++++++++
>  drivers/net/wireless/ath/ath9k/main.c  |  4 ++++
>  6 files changed, 41 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
> index 29ca65a732a6..c1ce081445a9 100644
> --- a/drivers/net/wireless/ath/ath9k/ath9k.h
> +++ b/drivers/net/wireless/ath/ath9k/ath9k.h
> @@ -739,11 +739,13 @@ void ath9k_csa_update(struct ath_softc *sc);
>  #define ATH_ANI_MAX_SKIP_COUNT    10
>  #define ATH_PAPRD_TIMEOUT         100 /* msecs */
>  #define ATH_PLL_WORK_INTERVAL     100
> +#define ATH_HANG_WORK_INTERVAL    4000
>  
>  void ath_hw_check_work(struct work_struct *work);
>  void ath_reset_work(struct work_struct *work);
>  bool ath_hw_check(struct ath_softc *sc);
>  void ath_hw_pll_work(struct work_struct *work);
> +void ath_hw_hang_work(struct work_struct *work);
>  void ath_paprd_calibrate(struct work_struct *work);
>  void ath_ani_calibrate(struct timer_list *t);
>  void ath_start_ani(struct ath_softc *sc);
> @@ -1044,6 +1046,7 @@ struct ath_softc {
>  #endif
>  	struct delayed_work hw_check_work;
>  	struct delayed_work hw_pll_work;
> +	struct delayed_work hw_hang_work;
>  	struct timer_list sleep_timer;
>  
>  #ifdef CONFIG_ATH9K_BTCOEX_SUPPORT
> diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
> index eff894958a73..6b2469a01f17 100644
> --- a/drivers/net/wireless/ath/ath9k/debug.c
> +++ b/drivers/net/wireless/ath/ath9k/debug.c
> @@ -750,6 +750,7 @@ static int read_file_reset(struct seq_file *file, void *data)
>  		[RESET_TYPE_CALIBRATION] = "Calibration error",
>  		[RESET_TX_DMA_ERROR] = "Tx DMA stop error",
>  		[RESET_RX_DMA_ERROR] = "Rx DMA stop error",
> +		[RESET_TYPE_DEADBEEF] = "deadbeef hang",
>  	};
>  	int i;
>  
> diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
> index 389459c04d14..6ebb6053a8c1 100644
> --- a/drivers/net/wireless/ath/ath9k/debug.h
> +++ b/drivers/net/wireless/ath/ath9k/debug.h
> @@ -53,6 +53,7 @@ enum ath_reset_type {
>  	RESET_TYPE_CALIBRATION,
>  	RESET_TX_DMA_ERROR,
>  	RESET_RX_DMA_ERROR,
> +	RESET_TYPE_DEADBEEF,
>  	__RESET_TYPE_MAX
>  };
>  
> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
> index f9e77c4624d9..833474d7281f 100644
> --- a/drivers/net/wireless/ath/ath9k/init.c
> +++ b/drivers/net/wireless/ath/ath9k/init.c
> @@ -740,6 +740,7 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
>  	INIT_WORK(&sc->paprd_work, ath_paprd_calibrate);
>  	INIT_DELAYED_WORK(&sc->hw_pll_work, ath_hw_pll_work);
>  	INIT_DELAYED_WORK(&sc->hw_check_work, ath_hw_check_work);
> +	INIT_DELAYED_WORK(&sc->hw_hang_work, ath_hw_hang_work);
>  
>  	ath9k_init_channel_context(sc);
>  
> diff --git a/drivers/net/wireless/ath/ath9k/link.c b/drivers/net/wireless/ath/ath9k/link.c
> index d1e5767aab3c..37438960c278 100644
> --- a/drivers/net/wireless/ath/ath9k/link.c
> +++ b/drivers/net/wireless/ath/ath9k/link.c
> @@ -142,6 +142,37 @@ void ath_hw_pll_work(struct work_struct *work)
>  				     msecs_to_jiffies(ATH_PLL_WORK_INTERVAL));
>  }
>  
> +static bool ath_hw_hang_deadbeef(struct ath_softc *sc)
> +{
> +	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
> +	u32 reg;
> +
> +	/* check for stucked MAC */
> +	ath9k_ps_wakeup(sc);
> +	reg = REG_READ(sc->sc_ah, AR_CFG);
> +	ath9k_ps_restore(sc);
> +
> +	if (reg != 0xdeadbeef)
> +		return false;

ath9k_hw_check_alive() already does this exact check...

-Toke

[PATCH v2 1/2] wifi: ath9k: work around AR_CFG 0xdeadbeef chip hang

[Issam Hamdi]

From: Simon Wunderlich <simon.wunderlich@open-mesh.com>

QCA 802.11n chips (especially AR9330/AR9340) sometimes end up in a state in
which a read of AR_CFG always returns 0xdeadbeef. This should not happen
when the power_mode of the device is ATH9K_PM_AWAKE.

This problem is not yet detected by any other workaround in ath9k. No way
is known to reproduce the problem easily.

This patch originally developed by "Simon Wunderlich <simon.wunderlich@open-mesh.com>"
and "Sven Eckelmann <sven.eckelmann@open-mesh.com>"

Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
Signed-off-by: Sven Eckelmann <se@simonwunderlich.de>
Signed-off-by: Issam Hamdi <ih@simonwunderlich.de>
---
v2: update the Co-developed-by to Signed-off-by
---
 drivers/net/wireless/ath/ath9k/ath9k.h |  3 +++
 drivers/net/wireless/ath/ath9k/debug.c |  1 +
 drivers/net/wireless/ath/ath9k/debug.h |  1 +
 drivers/net/wireless/ath/ath9k/init.c  |  1 +
 drivers/net/wireless/ath/ath9k/link.c  | 31 ++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath9k/main.c  |  4 ++++
 6 files changed, 41 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 29ca65a732a6..c1ce081445a9 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -739,11 +739,13 @@ void ath9k_csa_update(struct ath_softc *sc);
 #define ATH_ANI_MAX_SKIP_COUNT    10
 #define ATH_PAPRD_TIMEOUT         100 /* msecs */
 #define ATH_PLL_WORK_INTERVAL     100
+#define ATH_HANG_WORK_INTERVAL    4000
 
 void ath_hw_check_work(struct work_struct *work);
 void ath_reset_work(struct work_struct *work);
 bool ath_hw_check(struct ath_softc *sc);
 void ath_hw_pll_work(struct work_struct *work);
+void ath_hw_hang_work(struct work_struct *work);
 void ath_paprd_calibrate(struct work_struct *work);
 void ath_ani_calibrate(struct timer_list *t);
 void ath_start_ani(struct ath_softc *sc);
@@ -1044,6 +1046,7 @@ struct ath_softc {
 #endif
 	struct delayed_work hw_check_work;
 	struct delayed_work hw_pll_work;
+	struct delayed_work hw_hang_work;
 	struct timer_list sleep_timer;
 
 #ifdef CONFIG_ATH9K_BTCOEX_SUPPORT
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index eff894958a73..6b2469a01f17 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -750,6 +750,7 @@ static int read_file_reset(struct seq_file *file, void *data)
 		[RESET_TYPE_CALIBRATION] = "Calibration error",
 		[RESET_TX_DMA_ERROR] = "Tx DMA stop error",
 		[RESET_RX_DMA_ERROR] = "Rx DMA stop error",
+		[RESET_TYPE_DEADBEEF] = "deadbeef hang",
 	};
 	int i;
 
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 389459c04d14..6ebb6053a8c1 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -53,6 +53,7 @@ enum ath_reset_type {
 	RESET_TYPE_CALIBRATION,
 	RESET_TX_DMA_ERROR,
 	RESET_RX_DMA_ERROR,
+	RESET_TYPE_DEADBEEF,
 	__RESET_TYPE_MAX
 };
 
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index f9e77c4624d9..833474d7281f 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -740,6 +740,7 @@ static int ath9k_init_softc(u16 devid, struct ath_softc *sc,
 	INIT_WORK(&sc->paprd_work, ath_paprd_calibrate);
 	INIT_DELAYED_WORK(&sc->hw_pll_work, ath_hw_pll_work);
 	INIT_DELAYED_WORK(&sc->hw_check_work, ath_hw_check_work);
+	INIT_DELAYED_WORK(&sc->hw_hang_work, ath_hw_hang_work);
 
 	ath9k_init_channel_context(sc);
 
diff --git a/drivers/net/wireless/ath/ath9k/link.c b/drivers/net/wireless/ath/ath9k/link.c
index d1e5767aab3c..37438960c278 100644
--- a/drivers/net/wireless/ath/ath9k/link.c
+++ b/drivers/net/wireless/ath/ath9k/link.c
@@ -142,6 +142,37 @@ void ath_hw_pll_work(struct work_struct *work)
 				     msecs_to_jiffies(ATH_PLL_WORK_INTERVAL));
 }
 
+static bool ath_hw_hang_deadbeef(struct ath_softc *sc)
+{
+	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	u32 reg;
+
+	/* check for stucked MAC */
+	ath9k_ps_wakeup(sc);
+	reg = REG_READ(sc->sc_ah, AR_CFG);
+	ath9k_ps_restore(sc);
+
+	if (reg != 0xdeadbeef)
+		return false;
+
+	ath_dbg(common, RESET,
+		"0xdeadbeef hang is detected. Schedule chip reset\n");
+	ath9k_queue_reset(sc, RESET_TYPE_DEADBEEF);
+
+	return true;
+}
+
+void ath_hw_hang_work(struct work_struct *work)
+{
+	struct ath_softc *sc = container_of(work, struct ath_softc,
+					    hw_hang_work.work);
+
+	ath_hw_hang_deadbeef(sc);
+
+	ieee80211_queue_delayed_work(sc->hw, &sc->hw_hang_work,
+				     msecs_to_jiffies(ATH_HANG_WORK_INTERVAL));
+}
+
 /*
  * PA Pre-distortion.
  */
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index b92c89dad8de..024028ce8417 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -186,6 +186,7 @@ static void __ath_cancel_work(struct ath_softc *sc)
 	cancel_work_sync(&sc->paprd_work);
 	cancel_delayed_work_sync(&sc->hw_check_work);
 	cancel_delayed_work_sync(&sc->hw_pll_work);
+	cancel_delayed_work_sync(&sc->hw_hang_work);
 
 #ifdef CONFIG_ATH9K_BTCOEX_SUPPORT
 	if (ath9k_hw_mci_is_enabled(sc->sc_ah))
@@ -208,6 +209,9 @@ void ath_restart_work(struct ath_softc *sc)
 		ieee80211_queue_delayed_work(sc->hw, &sc->hw_pll_work,
 				     msecs_to_jiffies(ATH_PLL_WORK_INTERVAL));
 
+	ieee80211_queue_delayed_work(sc->hw, &sc->hw_hang_work,
+				     msecs_to_jiffies(ATH_HANG_WORK_INTERVAL));
+
 	ath_start_ani(sc);
 }
 

base-commit: 2b94751626a6d49bbe42a19cc1503bd391016bd5
-- 
2.39.2

[PATCH v2 2/2] wifi: ath9k: Reset chip on potential deaf state

[Issam Hamdi]

From: Simon Wunderlich <simon.wunderlich@open-mesh.com>

The chip is switching seemingly random into a state which can be described
as "deaf". No or nearly no interrupts are generated anymore for incoming
packets. Existing links either break down after a while and new links will
not be established.

The driver doesn't know if there is no other device available or if it
ended up in an "deaf" state. Resetting the chip proactively avoids
permanent problems in case the chip really was in its "deaf" state but
maybe causes unnecessary resets in case it wasn't "deaf".

The circumstances leading to this "deafness" is still unclear,
but we see that some particular chips (especially 2-stream 11n
SoCs, but also others) can go 'deaf' when running AP or mesh (or both)
after some time. It's probably a hardware issue, and doing a channel
scan to trigger a chip reset (which one normally can't do on an AP interface)
recovers the hardware. This patch provides a workaround within the kernel.

The idea is that if the radio is idle anyway, a quick reset (which takes a few
tens of ms maximum) doesn't hurt much, and it helps to recover non-functional
APs or mesh points.

This patch originally developed by "Simon Wunderlich <simon.wunderlich@open-mesh.com>"
and "Sven Eckelmann <sven.eckelmann@open-mesh.com>"

Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
Signed-off-by: Sven Eckelmann <se@simonwunderlich.de>
Signed-off-by: Issam Hamdi <ih@simonwunderlich.de>
---
v2: change the "Co-developed-by" to "Signed-off-by", remove the dependency
on CONFIG_ATH9K_DEBUGFS and add more information in the commit description
---
 drivers/net/wireless/ath/ath9k/ath9k.h |  3 ++
 drivers/net/wireless/ath/ath9k/debug.c |  1 +
 drivers/net/wireless/ath/ath9k/debug.h |  1 +
 drivers/net/wireless/ath/ath9k/link.c  | 48 +++++++++++++++++++++++++-
 4 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index c1ce081445a9..2b98c69fa37f 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -1026,6 +1026,9 @@ struct ath_softc {
 	short nbcnvifs;
 	unsigned long ps_usecount;
 
+	unsigned long last_check_time;
+	u32 last_check_interrupts;
+
 	struct ath_rx rx;
 	struct ath_tx tx;
 	struct ath_beacon beacon;
diff --git a/drivers/net/wireless/ath/ath9k/debug.c b/drivers/net/wireless/ath/ath9k/debug.c
index 6b2469a01f17..4128cf691166 100644
--- a/drivers/net/wireless/ath/ath9k/debug.c
+++ b/drivers/net/wireless/ath/ath9k/debug.c
@@ -751,6 +751,7 @@ static int read_file_reset(struct seq_file *file, void *data)
 		[RESET_TX_DMA_ERROR] = "Tx DMA stop error",
 		[RESET_RX_DMA_ERROR] = "Rx DMA stop error",
 		[RESET_TYPE_DEADBEEF] = "deadbeef hang",
+		[RESET_TYPE_DEAF] = "deaf hang",
 	};
 	int i;
 
diff --git a/drivers/net/wireless/ath/ath9k/debug.h b/drivers/net/wireless/ath/ath9k/debug.h
index 6ebb6053a8c1..76e27860455c 100644
--- a/drivers/net/wireless/ath/ath9k/debug.h
+++ b/drivers/net/wireless/ath/ath9k/debug.h
@@ -54,6 +54,7 @@ enum ath_reset_type {
 	RESET_TX_DMA_ERROR,
 	RESET_RX_DMA_ERROR,
 	RESET_TYPE_DEADBEEF,
+	RESET_TYPE_DEAF,
 	__RESET_TYPE_MAX
 };
 
diff --git a/drivers/net/wireless/ath/ath9k/link.c b/drivers/net/wireless/ath/ath9k/link.c
index 37438960c278..2700598c6336 100644
--- a/drivers/net/wireless/ath/ath9k/link.c
+++ b/drivers/net/wireless/ath/ath9k/link.c
@@ -162,13 +162,59 @@ static bool ath_hw_hang_deadbeef(struct ath_softc *sc)
 	return true;
 }
 
+static bool ath_hw_hang_deaf(struct ath_softc *sc)
+{
+#ifdef CONFIG_ATH9K_TX99
+	return false;
+#else
+	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
+	u32 interrupts, interrupt_per_s;
+	unsigned int interval;
+
+	/* get historic data */
+	interval = jiffies_to_msecs(jiffies - sc->last_check_time);
+	if (sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA)
+		interrupts = sc->debug.stats.istats.rxlp;
+	else
+		interrupts = sc->debug.stats.istats.rxok;
+
+	interrupts -= sc->last_check_interrupts;
+
+	/* save current data */
+	sc->last_check_time = jiffies;
+	if (sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA)
+		sc->last_check_interrupts = sc->debug.stats.istats.rxlp;
+	else
+		sc->last_check_interrupts = sc->debug.stats.istats.rxok;
+
+	/* sanity check, should be 4 seconds */
+	if (interval > 10000 || interval < 1000)
+		return false;
+
+	/* should be at least one interrupt per second */
+	interrupt_per_s = interrupts / (interval / 1000);
+	if (interrupt_per_s >= 1)
+		return false;
+
+	ath_dbg(common, RESET,
+		"RX deaf hang is detected. Schedule chip reset\n");
+	ath9k_queue_reset(sc, RESET_TYPE_DEAF);
+
+	return true;
+#endif
+}
+
 void ath_hw_hang_work(struct work_struct *work)
 {
 	struct ath_softc *sc = container_of(work, struct ath_softc,
 					    hw_hang_work.work);
 
-	ath_hw_hang_deadbeef(sc);
+	if (ath_hw_hang_deadbeef(sc))
+		goto requeue_worker;
 
+	ath_hw_hang_deaf(sc);
+
+requeue_worker:
 	ieee80211_queue_delayed_work(sc->hw, &sc->hw_hang_work,
 				     msecs_to_jiffies(ATH_HANG_WORK_INTERVAL));
 }
-- 
2.39.2

Re: [PATCH v2 2/2] wifi: ath9k: Reset chip on potential deaf state

[Sven Eckelmann]

On Wednesday, 6 November 2024 10:04:39 CET Issam Hamdi wrote:
[...]
> This patch originally developed by "Simon Wunderlich <simon.wunderlich@open-mesh.com>"
> and "Sven Eckelmann <sven.eckelmann@open-mesh.com>"

Am I the only person which finds this style of adding information about "Co-
authors" weird?

[...]
> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
> Signed-off-by: Sven Eckelmann <se@simonwunderlich.de>
> Signed-off-by: Issam Hamdi <ih@simonwunderlich.de>
> ---
> v2: change the "Co-developed-by" to "Signed-off-by", remove the dependency

I think Kalle meant that "Co-developed-by" should be followed by a 
"Signed-off-by" - not that "Co-developed-by" should be removed.

I was not part of the delivery path for this version of the patch. But
current Signed-off-by seem to suggest this.

> on CONFIG_ATH9K_DEBUGFS and add more information in the commit description

And please don't reply to the old thread when sending a new patchset - this 
becomes really unreadable after a while. You can simply use the method which 
b4 uses and just reference the old thread in your mail. Something like:

Changes in v2:
- change the "Co-developed-by" to "Signed-off-by"
- remove the dependency on CONFIG_ATH9K_DEBUGFS
- add more information in the commit description
- Link to v1: https://lore.kernel.org/r/20241104171627.3789199-1-ih@simonwunderlich.de

[...]
> +static bool ath_hw_hang_deaf(struct ath_softc *sc)
> +{
> +#ifdef CONFIG_ATH9K_TX99
> +	return false;
> +#else
> +	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
> +	u32 interrupts, interrupt_per_s;
> +	unsigned int interval;
> +
> +	/* get historic data */
> +	interval = jiffies_to_msecs(jiffies - sc->last_check_time);
> +	if (sc->sc_ah->caps.hw_caps & ATH9K_HW_CAP_EDMA)
> +		interrupts = sc->debug.stats.istats.rxlp;
> +	else
> +		interrupts = sc->debug.stats.istats.rxok;

You can't simply access sc->debug.stats.istats. sc->debug is only available 
when building with CONFIG_ATH9K_DEBUGFS. See ath9k.c

struct ath_softc {
[...]
#ifdef CONFIG_ATH9K_DEBUGFS
	struct ath9k_debug debug;
#endif
[...]
}

> +       /* sanity check, should be 4 seconds */
> +       if (interval > 10000 || interval < 1000)

Here you have hardcoded values but the actual interval is hidden behind 
ATH_HANG_WORK_INTERVAL. Two things which now are rather disconnected and might 
cause problems in the future (when somebody fiddles around with 
ATH_HANG_WORK_INTERVAL).

Overall, the proposal from Toke seems to be a lot better integrated in the HW 
check style which was introduced by Felix in the beginning of 2017 [1].

At the same time there was a proposal by Felix [2] - which diverged too much 
from our original patch (and as a result caused too many resets) [3]. I would 
therefore propose to check Toke's version and test handles the problem 
correctly.

Kind regards,
	Sven

[1] https://github.com/openwrt/openwrt/commit/b94177e10fc72f9309eae7459c3570e5c080e960
[2] https://patchwork.kernel.org/project/linux-wireless/patch/20170125163654.66431-3-nbd@nbd.name/
[3] https://lore.kernel.org/all/2081606.z26xgMiW1A@prime/

Re: [PATCH v2 2/2] wifi: ath9k: Reset chip on potential deaf state

[Toke Høiland-Jørgensen]

Sven Eckelmann <se@simonwunderlich.de> writes:

> Overall, the proposal from Toke seems to be a lot better integrated in the HW 
> check style which was introduced by Felix in the beginning of 2017 [1].
>
> At the same time there was a proposal by Felix [2] - which diverged too much 
> from our original patch (and as a result caused too many resets) [3]. I would 
> therefore propose to check Toke's version and test handles the problem 
> correctly.

Yes, agreed. I did actually write up a commit message for that, let me
send it as a proper patch...

-Toke

Re: [PATCH v2 2/2] wifi: ath9k: Reset chip on potential deaf state

[Kalle Valo]

Sven Eckelmann <se@simonwunderlich.de> writes:

> On Wednesday, 6 November 2024 10:04:39 CET Issam Hamdi wrote:
> [...]
>> This patch originally developed by "Simon Wunderlich <simon.wunderlich@open-mesh.com>"
>> and "Sven Eckelmann <sven.eckelmann@open-mesh.com>"
>
> Am I the only person which finds this style of adding information about "Co-
> authors" weird?

No, you are not. I also find it weird.

> [...]
>> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
>> Signed-off-by: Sven Eckelmann <se@simonwunderlich.de>
>> Signed-off-by: Issam Hamdi <ih@simonwunderlich.de>
>> ---
>> v2: change the "Co-developed-by" to "Signed-off-by", remove the dependency
>
> I think Kalle meant that "Co-developed-by" should be followed by a 
> "Signed-off-by" - not that "Co-developed-by" should be removed.

Correct, to my understanding having both c-d-b and s-o-b is the
preferred format.

>> on CONFIG_ATH9K_DEBUGFS and add more information in the commit description
>
> And please don't reply to the old thread when sending a new patchset - this 
> becomes really unreadable after a while. You can simply use the method which 
> b4 uses and just reference the old thread in your mail. Something like:
>
> Changes in v2:
> - change the "Co-developed-by" to "Signed-off-by"
> - remove the dependency on CONFIG_ATH9K_DEBUGFS
> - add more information in the commit description
> - Link to v1: https://lore.kernel.org/r/20241104171627.3789199-1-ih@simonwunderlich.de

Yes, this style is very much recommended. Having links to older versions
helps reviewers.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 1/2] wifi: ath9k: work around AR_CFG 0xdeadbeef chip hang

[Sven Eckelmann]

On Wednesday, 6 November 2024 10:04:38 CET Issam Hamdi wrote:
> +static bool ath_hw_hang_deadbeef(struct ath_softc *sc)
> +{
> +       struct ath_common *common = ath9k_hw_common(sc->sc_ah);
> +       u32 reg;
> +
> +       /* check for stucked MAC */
> +       ath9k_ps_wakeup(sc);
> +       reg = REG_READ(sc->sc_ah, AR_CFG);
> +       ath9k_ps_restore(sc);
> +
> +       if (reg != 0xdeadbeef)
> +               return false;
> +
> +       ath_dbg(common, RESET,
> +               "0xdeadbeef hang is detected. Schedule chip reset\n");
> +       ath9k_queue_reset(sc, RESET_TYPE_DEADBEEF);
> +
> +       return true;
> +}

I don't really get why this was proposed again. Can you please explain why the 
reworked version of this patch [1] (by Felix) is not good enough?

If there are any deficits with Felix's version then it would be nice to know 
get informed about it (in the commit message) and then work on the deficits. 
Having two different checks at the same time for the same thing in upstream 
Linux doesn't seem to be helpful.

I have more problems with the submission style - see next mail.

Kind regards,
	Sven

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a34d0a0da1abae46a5f6ebd06fb0ec484ca099d9