Subject: [PATCH 1/4] ath6kl: Fix bug in scheduling hb_timer

hb_timer should be scheduled only when hb_poll is non-zero.
But in ath6kl_recovery_work() the timer is scheduled based
on fw_recovery.enable instead which is wrong.

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/recovery.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/recovery.c b/drivers/net/wireless/ath/ath6kl/recovery.c
index 03edeb8..c30df50 100644
--- a/drivers/net/wireless/ath/ath6kl/recovery.c
+++ b/drivers/net/wireless/ath/ath6kl/recovery.c
@@ -34,7 +34,7 @@ static void ath6kl_recovery_work(struct work_struct *work)

ar->fw_recovery.err_reason = 0;

- if (ar->fw_recovery.enable)
+ if (ar->fw_recovery.hb_poll)
mod_timer(&ar->fw_recovery.hb_timer, jiffies +
msecs_to_jiffies(ar->fw_recovery.hb_poll));
}
--
1.7.0.4



Subject: [PATCH 3/4] ath6kl: Add a bit to ath6kl_dev_state for recovery cleanup state

Add a bit in ath6kl_dev_state to maintian the run time state
of firmware recovery configuration. This would help to have
user configuration in fw_recovery which will be added in
a separate patch.

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/core.h | 2 +-
drivers/net/wireless/ath/ath6kl/recovery.c | 12 +++++++-----
2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/core.h b/drivers/net/wireless/ath/ath6kl/core.h
index ac90b31..40a7b19 100644
--- a/drivers/net/wireless/ath/ath6kl/core.h
+++ b/drivers/net/wireless/ath/ath6kl/core.h
@@ -641,6 +641,7 @@ enum ath6kl_dev_state {
SKIP_SCAN,
ROAM_TBL_PEND,
FIRST_BOOT,
+ RECOVERY_CLEANUP,
};

enum ath6kl_state {
@@ -807,7 +808,6 @@ struct ath6kl {
bool wiphy_registered;

struct ath6kl_fw_recovery {
- bool enable;
struct work_struct recovery_work;
unsigned long err_reason;
unsigned long hb_poll;
diff --git a/drivers/net/wireless/ath/ath6kl/recovery.c b/drivers/net/wireless/ath/ath6kl/recovery.c
index 1a82e8b..98b6aa0 100644
--- a/drivers/net/wireless/ath/ath6kl/recovery.c
+++ b/drivers/net/wireless/ath/ath6kl/recovery.c
@@ -46,7 +46,8 @@ void ath6kl_recovery_err_notify(struct ath6kl *ar, enum ath6kl_fw_err reason)

set_bit(reason, &ar->fw_recovery.err_reason);

- if (ar->fw_recovery.enable && ar->state != ATH6KL_STATE_RECOVERY)
+ if (!test_bit(RECOVERY_CLEANUP, &ar->flag) &&
+ ar->state != ATH6KL_STATE_RECOVERY)
queue_work(ar->ath6kl_wq, &ar->fw_recovery.recovery_work);
}

@@ -61,7 +62,8 @@ static void ath6kl_recovery_hb_timer(unsigned long data)
struct ath6kl *ar = (struct ath6kl *) data;
int err;

- if (!ar->fw_recovery.enable || (ar->state == ATH6KL_STATE_RECOVERY))
+ if (test_bit(RECOVERY_CLEANUP, &ar->flag) ||
+ (ar->state == ATH6KL_STATE_RECOVERY))
return;

if (ar->fw_recovery.hb_pending)
@@ -94,7 +96,7 @@ void ath6kl_recovery_init(struct ath6kl *ar)
{
struct ath6kl_fw_recovery *recovery = &ar->fw_recovery;

- recovery->enable = true;
+ clear_bit(RECOVERY_CLEANUP, &ar->flag);
INIT_WORK(&recovery->recovery_work, ath6kl_recovery_work);
recovery->seq_num = 0;
recovery->hb_misscnt = 0;
@@ -110,7 +112,7 @@ void ath6kl_recovery_init(struct ath6kl *ar)

void ath6kl_recovery_cleanup(struct ath6kl *ar)
{
- ar->fw_recovery.enable = false;
+ set_bit(RECOVERY_CLEANUP, &ar->flag);

del_timer_sync(&ar->fw_recovery.hb_timer);
cancel_work_sync(&ar->fw_recovery.recovery_work);
@@ -133,7 +135,7 @@ void ath6kl_recovery_suspend(struct ath6kl *ar)

void ath6kl_recovery_resume(struct ath6kl *ar)
{
- ar->fw_recovery.enable = true;
+ clear_bit(RECOVERY_CLEANUP, &ar->flag);

if (!ar->fw_recovery.hb_poll)
return;
--
1.7.0.4


Subject: [PATCH 2/4] ath6kl: Remove unnecessary recovery state check in ath6kl_recovery_hb_timer()

Checking for recovery state just before re-arming hb_timer is not
necessary, this should be done at the begining of the timer instead.

Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/recovery.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/recovery.c b/drivers/net/wireless/ath/ath6kl/recovery.c
index c30df50..1a82e8b 100644
--- a/drivers/net/wireless/ath/ath6kl/recovery.c
+++ b/drivers/net/wireless/ath/ath6kl/recovery.c
@@ -61,7 +61,7 @@ static void ath6kl_recovery_hb_timer(unsigned long data)
struct ath6kl *ar = (struct ath6kl *) data;
int err;

- if (!ar->fw_recovery.enable)
+ if (!ar->fw_recovery.enable || (ar->state == ATH6KL_STATE_RECOVERY))
return;

if (ar->fw_recovery.hb_pending)
@@ -86,9 +86,6 @@ static void ath6kl_recovery_hb_timer(unsigned long data)
ath6kl_warn("Failed to send hb challenge request, err:%d\n",
err);

- if ((ar->state == ATH6KL_STATE_RECOVERY) || !ar->fw_recovery.enable)
- return;
-
mod_timer(&ar->fw_recovery.hb_timer, jiffies +
msecs_to_jiffies(ar->fw_recovery.hb_poll));
}
--
1.7.0.4


2012-09-21 16:35:38

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/4] ath6kl: Fix bug in scheduling hb_timer

On 09/03/2012 10:19 AM, Vasanthakumar Thiagarajan wrote:
> hb_timer should be scheduled only when hb_poll is non-zero.
> But in ath6kl_recovery_work() the timer is scheduled based
> on fw_recovery.enable instead which is wrong.
>
> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>

Thanks, all four applied.

Kalle

Subject: [PATCH 4/4] ath6kl: Make fw error recovery configurable

Add a modparam to configure recovery. Recovery
from firmware error is disabled by default to debug
the actual issue further. To recovery from error,
modprobe ath6kl_core recovery_enable=1.

Reported-by: Jin Navy <[email protected]>
Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/core.c | 12 ++++++++++--
drivers/net/wireless/ath/ath6kl/core.h | 1 +
drivers/net/wireless/ath/ath6kl/recovery.c | 12 ++++++++++++
3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/core.c b/drivers/net/wireless/ath/ath6kl/core.c
index fd5dd3a..4b46adb 100644
--- a/drivers/net/wireless/ath/ath6kl/core.c
+++ b/drivers/net/wireless/ath/ath6kl/core.c
@@ -33,6 +33,7 @@ static unsigned int wow_mode;
static unsigned int uart_debug;
static unsigned int ath6kl_p2p;
static unsigned int testmode;
+static unsigned int recovery_enable;
static unsigned int heart_beat_poll;

module_param(debug_mask, uint, 0644);
@@ -41,9 +42,12 @@ module_param(wow_mode, uint, 0644);
module_param(uart_debug, uint, 0644);
module_param(ath6kl_p2p, uint, 0644);
module_param(testmode, uint, 0644);
+module_param(recovery_enable, uint, 0644);
module_param(heart_beat_poll, uint, 0644);
-MODULE_PARM_DESC(heart_beat_poll, "Enable fw error detection periodic" \
- "polling. This also specifies the polling interval in msecs");
+MODULE_PARM_DESC(recovery_enable, "Enable recovery from firmware error");
+MODULE_PARM_DESC(heart_beat_poll, "Enable fw error detection periodic" \
+ "polling. This also specifies the polling interval in" \
+ "msecs. Set reocvery_enable for this to be effective");

void ath6kl_core_tx_complete(struct ath6kl *ar, struct sk_buff *skb)
{
@@ -206,6 +210,10 @@ int ath6kl_core_init(struct ath6kl *ar, enum ath6kl_htc_type htc_type)
ath6kl_dbg(ATH6KL_DBG_TRC, "%s: name=%s dev=0x%p, ar=0x%p\n",
__func__, wdev->netdev->name, wdev->netdev, ar);

+ ar->fw_recovery.enable = !!recovery_enable;
+ if (!ar->fw_recovery.enable)
+ return ret;
+
if (heart_beat_poll &&
test_bit(ATH6KL_FW_CAPABILITY_HEART_BEAT_POLL,
ar->fw_capabilities))
diff --git a/drivers/net/wireless/ath/ath6kl/core.h b/drivers/net/wireless/ath/ath6kl/core.h
index 40a7b19..3b2dfc1 100644
--- a/drivers/net/wireless/ath/ath6kl/core.h
+++ b/drivers/net/wireless/ath/ath6kl/core.h
@@ -815,6 +815,7 @@ struct ath6kl {
u32 seq_num;
bool hb_pending;
u8 hb_misscnt;
+ bool enable;
} fw_recovery;

#ifdef CONFIG_ATH6KL_DEBUG
diff --git a/drivers/net/wireless/ath/ath6kl/recovery.c b/drivers/net/wireless/ath/ath6kl/recovery.c
index 98b6aa0..3a8d5e9 100644
--- a/drivers/net/wireless/ath/ath6kl/recovery.c
+++ b/drivers/net/wireless/ath/ath6kl/recovery.c
@@ -41,6 +41,9 @@ static void ath6kl_recovery_work(struct work_struct *work)

void ath6kl_recovery_err_notify(struct ath6kl *ar, enum ath6kl_fw_err reason)
{
+ if (!ar->fw_recovery.enable)
+ return;
+
ath6kl_dbg(ATH6KL_DBG_RECOVERY, "Fw error detected, reason:%d\n",
reason);

@@ -112,6 +115,9 @@ void ath6kl_recovery_init(struct ath6kl *ar)

void ath6kl_recovery_cleanup(struct ath6kl *ar)
{
+ if (!ar->fw_recovery.enable)
+ return;
+
set_bit(RECOVERY_CLEANUP, &ar->flag);

del_timer_sync(&ar->fw_recovery.hb_timer);
@@ -120,6 +126,9 @@ void ath6kl_recovery_cleanup(struct ath6kl *ar)

void ath6kl_recovery_suspend(struct ath6kl *ar)
{
+ if (!ar->fw_recovery.enable)
+ return;
+
ath6kl_recovery_cleanup(ar);

if (!ar->fw_recovery.err_reason)
@@ -135,6 +144,9 @@ void ath6kl_recovery_suspend(struct ath6kl *ar)

void ath6kl_recovery_resume(struct ath6kl *ar)
{
+ if (!ar->fw_recovery.enable)
+ return;
+
clear_bit(RECOVERY_CLEANUP, &ar->flag);

if (!ar->fw_recovery.hb_poll)
--
1.7.0.4