[ath9k-devel] [PATCH 0/3] ath10k: hardware recovery

[ath9k-devel] [PATCH 0/3] ath10k: hardware recovery

[Michal Kazior]

There's one issue I've noticed. When associated
to WPA network tx is discarded by the firmware
after recovery (no idea why, yet). This can be
fixed in a follow up patch perhaps.

Note: this is not based on master branch. It is
based on my latest 'ath10k: fixes' and 'ath10k:
device setup refactor'. Split for easier review.

Michal Kazior (3):
  ath10k: implement device recovery
  ath10k: implement fw crash simulation command
  ath10k: create debugfs interface to trigger fw crash

 drivers/net/wireless/ath/ath10k/core.c  |   30 ++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/core.h  |    3 +++
 drivers/net/wireless/ath/ath10k/debug.c |   39 +++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/htc.c   |    3 +++
 drivers/net/wireless/ath/ath10k/mac.c   |   32 +++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/pci.c   |    2 ++
 drivers/net/wireless/ath/ath10k/wmi.c   |   26 +++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi.h   |   19 +++++++++++++++
 8 files changed, 152 insertions(+), 2 deletions(-)

-- 
1.7.9.5

[ath9k-devel] [PATCH 1/3] ath10k: implement device recovery

[Michal Kazior]

Restart the hardware if FW crashes.

If FW crashes during recovery we leave the
hardware in a "wedged" state to avoid recursive
recoveries.

When in "wedged" state userspace may bring
interfaces down (to issue stop()) and then bring
one interface (to issue start()) to reload
hardware manually.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.c |   30 ++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/core.h |    3 +++
 drivers/net/wireless/ath/ath10k/htc.c  |    3 +++
 drivers/net/wireless/ath/ath10k/mac.c  |   32 ++++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/pci.c  |    2 ++
 drivers/net/wireless/ath/ath10k/wmi.c  |    7 +++++++
 6 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 94230fb..9188fb7 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -476,6 +476,34 @@ static int ath10k_init_hw_params(struct ath10k *ar)
 	return 0;
 }
 
+static void ath10k_core_restart(struct work_struct *work)
+{
+	struct ath10k *ar = container_of(work, struct ath10k, restart_work);
+
+	mutex_lock(&ar->conf_mutex);
+	spin_lock_bh(&ar->data_lock);
+
+	if (ar->state == ATH10K_STATE_RESTARTING) {
+		ath10k_err("crashed while restarting. will not attempt to restart the device anymore\n");
+		ar->state = ATH10K_STATE_WEDGED;
+		goto skip;
+	}
+
+	ath10k_info("attempting to restart the device\n");
+	ar->state = ATH10K_STATE_RESTARTING;
+
+	if (ar->scan.in_progress) {
+		del_timer(&ar->scan.timeout);
+		ar->scan.in_progress = false;
+		ieee80211_scan_completed(ar->hw, true);
+	}
+	ieee80211_restart_hw(ar->hw);
+
+skip:
+	spin_unlock_bh(&ar->data_lock);
+	mutex_unlock(&ar->conf_mutex);
+}
+
 struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
 				  const struct ath10k_hif_ops *hif_ops)
 {
@@ -519,6 +547,8 @@ struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
 
 	init_waitqueue_head(&ar->event_queue);
 
+	INIT_WORK(&ar->restart_work, ath10k_core_restart);
+
 	return ar;
 
 err_wq:
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index ed672f3..ef233c0 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -250,6 +250,7 @@ struct ath10k_debug {
 enum ath10k_state {
 	ATH10K_STATE_OFF = 0,
 	ATH10K_STATE_RESTARTING,
+	ATH10K_STATE_WEDGED,
 	ATH10K_STATE_ON
 };
 
@@ -356,6 +357,8 @@ struct ath10k {
 
 	enum ath10k_state state;
 
+	struct work_struct restart_work;
+
 #ifdef CONFIG_ATH10K_DEBUGFS
 	struct ath10k_debug debug;
 #endif
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 7d5a366..72e072c 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -246,6 +246,9 @@ int ath10k_htc_send(struct ath10k_htc *htc,
 {
 	struct ath10k_htc_ep *ep = &htc->endpoint[eid];
 
+	if (htc->ar->state == ATH10K_STATE_WEDGED)
+		return -ECOMM;
+
 	if (eid >= ATH10K_HTC_EP_COUNT) {
 		ath10k_warn("Invalid endpoint id: %d\n", eid);
 		return -ENOENT;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index c64e7be..6c97319 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -1806,6 +1806,7 @@ static void ath10k_stop(struct ieee80211_hw *hw)
 	mutex_unlock(&ar->conf_mutex);
 
 	cancel_work_sync(&ar->offchan_tx_work);
+	cancel_work_sync(&ar->restart_work);
 }
 
 static void ath10k_config_ps(struct ath10k *ar)
@@ -2793,6 +2794,7 @@ static int ath10k_set_frag_threshold(struct ieee80211_hw *hw, u32 value)
 static void ath10k_flush(struct ieee80211_hw *hw, u32 queues, bool drop)
 {
 	struct ath10k *ar = hw->priv;
+	bool skip;
 	int ret;
 
 	/* mac80211 doesn't care if we really xmit queued frames or not
@@ -2802,17 +2804,26 @@ static void ath10k_flush(struct ieee80211_hw *hw, u32 queues, bool drop)
 
 	mutex_lock(&ar->conf_mutex);
 
+	if (ar->state == ATH10K_STATE_WEDGED)
+		goto skip;
+
 	ret = wait_event_timeout(ar->htt.empty_tx_wq, ({
 			bool empty;
+
 			spin_lock_bh(&ar->htt.tx_lock);
 			empty = bitmap_empty(ar->htt.used_msdu_ids,
 					     ar->htt.max_num_pending_tx);
 			spin_unlock_bh(&ar->htt.tx_lock);
-			(empty);
+
+			skip = (ar->state == ATH10K_STATE_WEDGED);
+
+			(empty || skip);
 		}), ATH10K_FLUSH_TIMEOUT_HZ);
-	if (ret <= 0)
+
+	if (ret <= 0 || skip)
 		ath10k_warn("tx not flushed\n");
 
+skip:
 	mutex_unlock(&ar->conf_mutex);
 }
 
@@ -2886,6 +2897,22 @@ static int ath10k_resume(struct ieee80211_hw *hw)
 }
 #endif
 
+static void ath10k_restart_complete(struct ieee80211_hw *hw)
+{
+	struct ath10k *ar = hw->priv;
+
+	mutex_lock(&ar->conf_mutex);
+
+	if (ar->state == ATH10K_STATE_WEDGED) {
+		ath10k_err("device is not responding properly\n");
+	} else {
+		ar->state = ATH10K_STATE_ON;
+		ath10k_info("device successfully recovered\n");
+	}
+
+	mutex_unlock(&ar->conf_mutex);
+}
+
 static const struct ieee80211_ops ath10k_ops = {
 	.tx				= ath10k_tx,
 	.start				= ath10k_start,
@@ -2906,6 +2933,7 @@ static const struct ieee80211_ops ath10k_ops = {
 	.set_frag_threshold		= ath10k_set_frag_threshold,
 	.flush				= ath10k_flush,
 	.tx_last_beacon			= ath10k_tx_last_beacon,
+	.restart_complete		= ath10k_restart_complete,
 #ifdef CONFIG_PM
 	.suspend			= ath10k_suspend,
 	.resume				= ath10k_resume,
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index e9a6572..91790f4 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -720,6 +720,8 @@ static void ath10k_pci_hif_dump_area(struct ath10k *ar)
 			   reg_dump_values[i + 1],
 			   reg_dump_values[i + 2],
 			   reg_dump_values[i + 3]);
+
+	ieee80211_queue_work(ar->hw, &ar->restart_work);
 }
 
 static void ath10k_pci_hif_send_complete_check(struct ath10k *ar, u8 pipe,
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index b7e7e45..0d25cd7 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -27,6 +27,13 @@ void ath10k_wmi_flush_tx(struct ath10k *ar)
 {
 	int ret;
 
+	lockdep_assert_held(&ar->conf_mutex);
+
+	if (ar->state == ATH10K_STATE_WEDGED) {
+		ath10k_warn("wmi flush skipped - device is wedged anyway\n");
+		return;
+	}
+
 	ret = wait_event_timeout(ar->wmi.wq,
 				 atomic_read(&ar->wmi.pending_tx_count) == 0,
 				 5*HZ);
-- 
1.7.9.5

[ath9k-devel] [PATCH 1/3] ath10k: implement device recovery

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> Restart the hardware if FW crashes.
>
> If FW crashes during recovery we leave the
> hardware in a "wedged" state to avoid recursive
> recoveries.
>
> When in "wedged" state userspace may bring
> interfaces down (to issue stop()) and then bring
> one interface (to issue start()) to reload
> hardware manually.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -250,6 +250,7 @@ struct ath10k_debug {
>  enum ath10k_state {
>  	ATH10K_STATE_OFF = 0,
>  	ATH10K_STATE_RESTARTING,
> +	ATH10K_STATE_WEDGED,
>  	ATH10K_STATE_ON
>  };

It would be good to add small comment describing the new state, it might
not be obvious from the name.

[...]

> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -1806,6 +1806,7 @@ static void ath10k_stop(struct ieee80211_hw *hw)
>  	mutex_unlock(&ar->conf_mutex);
>  
>  	cancel_work_sync(&ar->offchan_tx_work);
> +	cancel_work_sync(&ar->restart_work);

The logic for this struct work is a bit scattered. It's initialised in
core.c, stopped in mac.c and pushed to queue from pci.c. To keep things
simple it would be best if we can have all that code in core.c. Here's
one idea:

 * rename current function to ath10k_core_restart_work()

 * create ath10k_core_restart() which just queues the work struct.

 * call cancel_work_sync() from a suitable function in core, do we have
   such a function?



-- 
Kalle Valo

[ath9k-devel] [PATCH 2/3] ath10k: implement fw crash simulation command

[Michal Kazior]

This can be useful to test FW crash handling.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/wmi.c |   19 +++++++++++++++++++
 drivers/net/wireless/ath/ath10k/wmi.h |   19 +++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 0d25cd7..5e42460 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -2092,3 +2092,22 @@ int ath10k_wmi_request_stats(struct ath10k *ar, enum wmi_stats_id stats_id)
 	ath10k_dbg(ATH10K_DBG_WMI, "wmi request stats %d\n", (int)stats_id);
 	return ath10k_wmi_cmd_send(ar, skb, WMI_REQUEST_STATS_CMDID);
 }
+
+int ath10k_wmi_force_fw_hang(struct ath10k *ar,
+			     enum wmi_force_fw_hang_type type, u32 delay_ms)
+{
+	struct wmi_force_fw_hang_cmd *cmd;
+	struct sk_buff *skb;
+
+	skb = ath10k_wmi_alloc_skb(sizeof(*cmd));
+	if (!skb)
+		return -ENOMEM;
+
+	cmd = (struct wmi_force_fw_hang_cmd *)skb->data;
+	cmd->type = __cpu_to_le32(type);
+	cmd->delay_ms = __cpu_to_le32(delay_ms);
+
+	ath10k_dbg(ATH10K_DBG_WMI, "wmi force fw hang %d delay %d\n",
+		   type, delay_ms);
+	return ath10k_wmi_cmd_send(ar, skb, WMI_FORCE_FW_HANG_CMDID);
+}
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 9555f5a..da3b2bc 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -416,6 +416,7 @@ enum wmi_cmd_id {
 	WMI_PDEV_FTM_INTG_CMDID,
 	WMI_VDEV_SET_KEEPALIVE_CMDID,
 	WMI_VDEV_GET_KEEPALIVE_CMDID,
+	WMI_FORCE_FW_HANG_CMDID,
 
 	/* GPIO Configuration */
 	WMI_GPIO_CONFIG_CMDID = WMI_CMD_GRP(WMI_GRP_GPIO),
@@ -2972,6 +2973,22 @@ struct wmi_sta_keepalive_cmd {
 	struct wmi_sta_keepalive_arp_resp arp_resp;
 } __packed;
 
+enum wmi_force_fw_hang_type {
+	WMI_FORCE_FW_HANG_ASSERT = 1,
+	WMI_FORCE_FW_HANG_NO_DETECT,
+	WMI_FORCE_FW_HANG_CTRL_EP_FULL,
+	WMI_FORCE_FW_HANG_EMPTY_POINT,
+	WMI_FORCE_FW_HANG_STACK_OVERFLOW,
+	WMI_FORCE_FW_HANG_INFINITE_LOOP,
+};
+
+#define WMI_FORCE_FW_HANG_RANDOM_TIME 0xFFFFFFFF
+
+struct wmi_force_fw_hang_cmd {
+	__le32 type;
+	__le32 delay_ms;
+} __packed;
+
 #define ATH10K_RTS_MAX		2347
 #define ATH10K_FRAGMT_THRESHOLD_MIN	540
 #define ATH10K_FRAGMT_THRESHOLD_MAX	2346
@@ -3048,5 +3065,7 @@ int ath10k_wmi_beacon_send(struct ath10k *ar, const struct wmi_bcn_tx_arg *arg);
 int ath10k_wmi_pdev_set_wmm_params(struct ath10k *ar,
 			const struct wmi_pdev_set_wmm_params_arg *arg);
 int ath10k_wmi_request_stats(struct ath10k *ar, enum wmi_stats_id stats_id);
+int ath10k_wmi_force_fw_hang(struct ath10k *ar,
+			     enum wmi_force_fw_hang_type type, u32 delay_ms);
 
 #endif /* _WMI_H_ */
-- 
1.7.9.5

[ath9k-devel] [PATCH 3/3] ath10k: create debugfs interface to trigger fw crash

[Michal Kazior]

This can be useful for testing.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/debug.c |   39 +++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 499034b..7ab7f1e 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -443,6 +443,42 @@ static const struct file_operations fops_fw_stats = {
 	.llseek = default_llseek,
 };
 
+static ssize_t ath10k_read_simulate_fw_crash(struct file *file,
+					     char __user *user_buf,
+					     size_t count, loff_t *ppos)
+{
+	const char buf[] = "To simulate firmware crash write anything to this"
+			   " file.\nThis will force firmware to report a crash"
+			   " to the host system.\n";
+	return simple_read_from_buffer(user_buf, count, ppos, buf, strlen(buf));
+}
+
+static ssize_t ath10k_write_simulate_fw_crash(struct file *file,
+					      const char __user *user_buf,
+					      size_t count, loff_t *ppos)
+{
+	struct ath10k *ar = file->private_data;
+	int ret;
+
+	ath10k_info("simulating firmware crash\n");
+
+	mutex_lock(&ar->conf_mutex);
+	ret = ath10k_wmi_force_fw_hang(ar, WMI_FORCE_FW_HANG_ASSERT, 0);
+	if (ret)
+		ath10k_warn("failed to force fw hang (%d)\n", ret);
+	mutex_unlock(&ar->conf_mutex);
+
+	return count;
+}
+
+static const struct file_operations fops_simulate_fw_crash = {
+	.read = ath10k_read_simulate_fw_crash,
+	.write = ath10k_write_simulate_fw_crash,
+	.open = simple_open,
+	.owner = THIS_MODULE,
+	.llseek = default_llseek,
+};
+
 int ath10k_debug_create(struct ath10k *ar)
 {
 	ar->debug.debugfs_phy = debugfs_create_dir("ath10k",
@@ -459,6 +495,9 @@ int ath10k_debug_create(struct ath10k *ar)
 	debugfs_create_file("wmi_services", S_IRUSR, ar->debug.debugfs_phy, ar,
 			    &fops_wmi_services);
 
+	debugfs_create_file("simulate_fw_crash", S_IRUSR, ar->debug.debugfs_phy,
+			    ar, &fops_simulate_fw_crash);
+
 	return 0;
 }
 #endif /* CONFIG_ATH10K_DEBUGFS */
-- 
1.7.9.5

[ath9k-devel] [PATCH 3/3] ath10k: create debugfs interface to trigger fw crash

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> This can be useful for testing.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

This is handy.

> +static ssize_t ath10k_read_simulate_fw_crash(struct file *file,
> +					     char __user *user_buf,
> +					     size_t count, loff_t *ppos)
> +{
> +	const char buf[] = "To simulate firmware crash write anything to this"
> +			   " file.\nThis will force firmware to report a crash"
> +			   " to the host system.\n";
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, strlen(buf));
> +}

But I'm not sure if just writing something to the file is a good idea.
Should it have some sort of protection, for example that user needs to
write a keyword or something?

-- 
Kalle Valo

[ath9k-devel] [PATCH 3/3] ath10k: create debugfs interface to trigger fw crash

[Michal Kazior]

On 14/06/13 14:34, Kalle Valo wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> This can be useful for testing.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> This is handy.
>
>> +static ssize_t ath10k_read_simulate_fw_crash(struct file *file,
>> +					     char __user *user_buf,
>> +					     size_t count, loff_t *ppos)
>> +{
>> +	const char buf[] = "To simulate firmware crash write anything to this"
>> +			   " file.\nThis will force firmware to report a crash"
>> +			   " to the host system.\n";
>> +	return simple_read_from_buffer(user_buf, count, ppos, buf, strlen(buf));
>> +}
>
> But I'm not sure if just writing something to the file is a good idea.
> Should it have some sort of protection, for example that user needs to
> write a keyword or something?

There are a few types of firmware failures that can be triggered. And 
there's also a delay that can be specified. So in theory we could accept 
parameters.

I wanted to keep things simple though. I didn't need anything more fancy 
than this to test firmware crashes/recovery.


-- Pozdrawiam / Best regards, Michal Kazior.

[ath9k-devel] [PATCH 3/3] ath10k: create debugfs interface to trigger fw crash

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 14/06/13 14:34, Kalle Valo wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> This can be useful for testing.
>>>
>>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>>
>> This is handy.
>>
>>> +static ssize_t ath10k_read_simulate_fw_crash(struct file *file,
>>> +					     char __user *user_buf,
>>> +					     size_t count, loff_t *ppos)
>>> +{
>>> +	const char buf[] = "To simulate firmware crash write anything to this"
>>> +			   " file.\nThis will force firmware to report a crash"
>>> +			   " to the host system.\n";
>>> +	return simple_read_from_buffer(user_buf, count, ppos, buf, strlen(buf));
>>> +}
>>
>> But I'm not sure if just writing something to the file is a good idea.
>> Should it have some sort of protection, for example that user needs to
>> write a keyword or something?
>
> There are a few types of firmware failures that can be triggered. And
> there's also a delay that can be specified. So in theory we could
> accept parameters.
>
> I wanted to keep things simple though. I didn't need anything more
> fancy than this to test firmware crashes/recovery.

Sorry, I wasn't clear above. I was just worried that a user might
accidentally crash the firmware through this interface by writing to a
wrong file. Not very likely scenario, but can happen anyway. Having some
sort of extra check (like a keyword) would avoid that case.

-- 
Kalle Valo