LinuxLists.cc - [PATCH v3 0/3] Enable low power mode when WLAN is not active

2022-11-21 11:06:23

Subject: [PATCH v3 0/3] Enable low power mode when WLAN is not active

2022-11-21 11:06:33

Subject: [PATCH v3 1/3] ath11k: Fix double free issue during SRNG deinit

Currently struct ath11k_hal::srng_config pointer is not assigned
to NULL after freeing the memory in ath11k_hal_srng_deinit().
This could lead to double free issue in a scenario where
ath11k_hal_srng_deinit() is invoked back to back.

In the current code, although the chances are very low, the above
said scenario could happen when hardware recovery has failed and
then there is another FW assert where ath11k_hal_srng_deinit() is
invoked once again as part of recovery. Addressing this issue is
important when low power mode support is enabled in the driver
(will be added by a future patch) where this scenario is likely.

Fix this by assigning the struct ath11k_hal::srng_config pointer
to NULL after freeing the memory.

Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1
Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.16

Signed-off-by: Manikanta Pubbisetty <[email protected]>
---
drivers/net/wireless/ath/ath11k/hal.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/ath/ath11k/hal.c b/drivers/net/wireless/ath/ath11k/hal.c
index 2fd224480d45..e92c741526f8 100644
--- a/drivers/net/wireless/ath/ath11k/hal.c
+++ b/drivers/net/wireless/ath/ath11k/hal.c
@@ -1319,6 +1319,7 @@ void ath11k_hal_srng_deinit(struct ath11k_base *ab)
ath11k_hal_free_cont_rdp(ab);
ath11k_hal_free_cont_wrp(ab);
kfree(hal->srng_config);
+ hal->srng_config = NULL;
}
EXPORT_SYMBOL(ath11k_hal_srng_deinit);

--
2.38.0

2022-11-21 11:06:38

by Manikanta Pubbisetty

[permalink] [raw]

Subject: [PATCH v3 2/3] ath11k: Move hardware initialization logic to start()

Currently during recovery, hardware is re-initialized as part of
ath11k_core_reconfigure_on_crash(). In order to enable low power
mode support in the driver, it is required to move the hardware
re-initialization logic to ath11k_ops.start() hook.

Since ath11k_ops.start() hook is called during WiFi ON/resume and
also during hardware recovery, it is better to defer the hardware
initialization to ath11k_ops.start() in the case of hardware recovery.
This will help ensure that there is only path for the initialization
of the hardware across different scenarios. A future patch will add
the support of initializing the hardware from start() hook in
WiFi ON/resume cases as well.

Commit 38194f3a605e ("ath11k: add synchronization operation between
reconfigure of mac80211 and ath11k_base") introduced a similar change that
applies just to QCA6390/WCN6855 to defer the initialization of the hardware
during recovery by using wait logic. This is no more needed and therefore
remove it.

Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1
Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.16

Signed-off-by: Manikanta Pubbisetty <[email protected]>
---
drivers/net/wireless/ath/ath11k/core.c | 83 +++++++++++---------------
drivers/net/wireless/ath/ath11k/core.h | 6 +-
drivers/net/wireless/ath/ath11k/mac.c | 29 +++------
drivers/net/wireless/ath/ath11k/qmi.c | 3 +-
4 files changed, 44 insertions(+), 77 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c
index edf78df9b12f..d36e193b2db9 100644
--- a/drivers/net/wireless/ath/ath11k/core.c
+++ b/drivers/net/wireless/ath/ath11k/core.c
@@ -1559,10 +1559,8 @@ int ath11k_core_qmi_firmware_ready(struct ath11k_base *ab)
return ret;
}

-static int ath11k_core_reconfigure_on_crash(struct ath11k_base *ab)
+static void ath11k_core_reconfigure_on_crash(struct ath11k_base *ab)
{
- int ret;
-
mutex_lock(&ab->core_lock);
ath11k_thermal_unregister(ab);
ath11k_hif_irq_disable(ab);
@@ -1574,27 +1572,8 @@ static int ath11k_core_reconfigure_on_crash(struct ath11k_base *ab)
mutex_unlock(&ab->core_lock);

ath11k_dp_free(ab);
- ath11k_hal_srng_deinit(ab);

ab->free_vdev_map = (1LL << (ab->num_radios * TARGET_NUM_VDEVS(ab))) - 1;
-
- ret = ath11k_hal_srng_init(ab);
- if (ret)
- return ret;
-
- clear_bit(ATH11K_FLAG_CRASH_FLUSH, &ab->dev_flags);
-
- ret = ath11k_core_qmi_firmware_ready(ab);
- if (ret)
- goto err_hal_srng_deinit;
-
- clear_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags);
-
- return 0;
-
-err_hal_srng_deinit:
- ath11k_hal_srng_deinit(ab);
- return ret;
}

void ath11k_core_halt(struct ath11k *ar)
@@ -1738,19 +1717,9 @@ static void ath11k_core_post_reconfigure_recovery(struct ath11k_base *ab)
static void ath11k_core_restart(struct work_struct *work)
{
struct ath11k_base *ab = container_of(work, struct ath11k_base, restart_work);
- int ret;

- ret = ath11k_core_reconfigure_on_crash(ab);
- if (ret) {
- ath11k_err(ab, "failed to reconfigure driver on crash recovery\n");
- return;
- }
-
- if (ab->is_reset)
- complete_all(&ab->reconfigure_complete);
-
- if (!ab->is_reset)
- ath11k_core_post_reconfigure_recovery(ab);
+ ath11k_core_reconfigure_on_crash(ab);
+ ath11k_core_post_reconfigure_recovery(ab);
}

static void ath11k_core_reset(struct work_struct *work)
@@ -1804,18 +1773,6 @@ static void ath11k_core_reset(struct work_struct *work)

ab->is_reset = true;
atomic_set(&ab->recovery_count, 0);
- reinit_completion(&ab->recovery_start);
- atomic_set(&ab->recovery_start_count, 0);
-
- ath11k_core_pre_reconfigure_recovery(ab);
-
- reinit_completion(&ab->reconfigure_complete);
- ath11k_core_post_reconfigure_recovery(ab);
-
- ath11k_dbg(ab, ATH11K_DBG_BOOT, "waiting recovery start...\n");
-
- time_left = wait_for_completion_timeout(&ab->recovery_start,
- ATH11K_RECOVER_START_TIMEOUT_HZ);

ath11k_hif_power_down(ab);
ath11k_hif_power_up(ab);
@@ -1923,8 +1880,6 @@ struct ath11k_base *ath11k_core_alloc(struct device *dev, size_t priv_size,
spin_lock_init(&ab->base_lock);
mutex_init(&ab->vdev_id_11d_lock);
init_completion(&ab->reset_complete);
- init_completion(&ab->reconfigure_complete);
- init_completion(&ab->recovery_start);

INIT_LIST_HEAD(&ab->peers);
init_waitqueue_head(&ab->peer_mapping_wq);
@@ -1950,5 +1905,37 @@ struct ath11k_base *ath11k_core_alloc(struct device *dev, size_t priv_size,
}
EXPORT_SYMBOL(ath11k_core_alloc);

+int ath11k_core_start_device(struct ath11k_base *ab)
+{
+ int ret;
+
+ if (!test_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags))
+ return 0;
+
+ ath11k_hal_srng_deinit(ab);
+
+ ret = ath11k_hal_srng_init(ab);
+ if (ret) {
+ ath11k_err(ab, "failed to init srng: %d\n", ret);
+ return ret;
+ }
+
+ clear_bit(ATH11K_FLAG_CRASH_FLUSH, &ab->dev_flags);
+
+ ret = ath11k_core_qmi_firmware_ready(ab);
+ if (ret) {
+ ath11k_err(ab, "failed to init core: %d\n", ret);
+ goto err_hal_srng_deinit;
+ }
+
+ clear_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags);
+
+ return 0;
+
+err_hal_srng_deinit:
+ ath11k_hal_srng_deinit(ab);
+ return ret;
+}
+
MODULE_DESCRIPTION("Core module for Qualcomm Atheros 802.11ax wireless LAN cards.");
MODULE_LICENSE("Dual BSD/GPL");
diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
index 22460b0abf03..9b55ac94b36c 100644
--- a/drivers/net/wireless/ath/ath11k/core.h
+++ b/drivers/net/wireless/ath/ath11k/core.h
@@ -60,8 +60,6 @@ extern unsigned int ath11k_frame_mode;
#define ATH11K_RESET_MAX_FAIL_COUNT_FIRST 3
#define ATH11K_RESET_MAX_FAIL_COUNT_FINAL 5
#define ATH11K_RESET_FAIL_TIMEOUT_HZ (20 * HZ)
-#define ATH11K_RECONFIGURE_TIMEOUT_HZ (10 * HZ)
-#define ATH11K_RECOVER_START_TIMEOUT_HZ (20 * HZ)

enum ath11k_supported_bw {
ATH11K_BW_20 = 0,
@@ -926,11 +924,8 @@ struct ath11k_base {
struct work_struct reset_work;
atomic_t reset_count;
atomic_t recovery_count;
- atomic_t recovery_start_count;
bool is_reset;
struct completion reset_complete;
- struct completion reconfigure_complete;
- struct completion recovery_start;
/* continuous recovery fail count */
atomic_t fail_cont_count;
unsigned long reset_fail_timeout;
@@ -1160,6 +1155,7 @@ void ath11k_core_halt(struct ath11k *ar);
int ath11k_core_resume(struct ath11k_base *ab);
int ath11k_core_suspend(struct ath11k_base *ab);
void ath11k_core_pre_reconfigure_recovery(struct ath11k_base *ab);
+int ath11k_core_start_device(struct ath11k_base *ab);

const struct firmware *ath11k_core_firmware_request(struct ath11k_base *ab,
const char *filename);
diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index 9e923ecb0891..c21ebf0578dc 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -5788,27 +5788,6 @@ static int ath11k_mac_config_mon_status_default(struct ath11k *ar, bool enable)
return ret;
}

-static void ath11k_mac_wait_reconfigure(struct ath11k_base *ab)
-{
- int recovery_start_count;
-
- if (!ab->is_reset)
- return;
-
- recovery_start_count = atomic_inc_return(&ab->recovery_start_count);
- ath11k_dbg(ab, ATH11K_DBG_MAC, "recovery start count %d\n", recovery_start_count);
-
- if (recovery_start_count == ab->num_radios) {
- complete(&ab->recovery_start);
- ath11k_dbg(ab, ATH11K_DBG_MAC, "recovery started success\n");
- }
-
- ath11k_dbg(ab, ATH11K_DBG_MAC, "waiting reconfigure...\n");
-
- wait_for_completion_timeout(&ab->reconfigure_complete,
- ATH11K_RECONFIGURE_TIMEOUT_HZ);
-}
-
static int ath11k_mac_op_start(struct ieee80211_hw *hw)
{
struct ath11k *ar = hw->priv;
@@ -5817,6 +5796,13 @@ static int ath11k_mac_op_start(struct ieee80211_hw *hw)
int ret;

ath11k_mac_drain_tx(ar);
+
+ ret = ath11k_core_start_device(ab);
+ if (ret) {
+ ath11k_err(ab, "failed to start device : %d\n", ret);
+ return ret;
+ }
+
mutex_lock(&ar->conf_mutex);

switch (ar->state) {
@@ -5825,7 +5811,6 @@ static int ath11k_mac_op_start(struct ieee80211_hw *hw)
break;
case ATH11K_STATE_RESTARTING:
ar->state = ATH11K_STATE_RESTARTED;
- ath11k_mac_wait_reconfigure(ab);
break;
case ATH11K_STATE_RESTARTED:
case ATH11K_STATE_WEDGED:
diff --git a/drivers/net/wireless/ath/ath11k/qmi.c b/drivers/net/wireless/ath/ath11k/qmi.c
index ab923e24b0a9..2e651b0fec8a 100644
--- a/drivers/net/wireless/ath/ath11k/qmi.c
+++ b/drivers/net/wireless/ath/ath11k/qmi.c
@@ -3165,8 +3165,7 @@ static void ath11k_qmi_driver_event_work(struct work_struct *work)
set_bit(ATH11K_FLAG_CRASH_FLUSH, &ab->dev_flags);
set_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags);

- if (!ab->is_reset)
- ath11k_core_pre_reconfigure_recovery(ab);
+ ath11k_core_pre_reconfigure_recovery(ab);
break;
case ATH11K_QMI_EVENT_REQUEST_MEM:
ret = ath11k_qmi_event_mem_request(qmi);
--
2.38.0

2022-11-21 11:06:42

by Manikanta Pubbisetty

[permalink] [raw]

Subject: [PATCH v3 3/3] ath11k: Enable low power mode when WLAN is not active

Currently, WLAN chip is powered once during driver probe and is kept
ON (powered) always even when WLAN is not active; keeping the chip
powered ON all the time will consume extra power which is not
desirable for a battery operated device. Same is the case with non-WoW
suspend, chip will never be put into low power mode when the system is
suspended resulting in higher battery drain.

As per the recommendation, sending a PDEV suspend WMI command followed
by a QMI MODE OFF command will cease all WLAN activity and put the device
in low power mode. When WLAN interfaces are brought up, sending a QMI
MISSION MODE command would be sufficient to bring the chip out of low
power. This is a better approach than doing hif_power_down()/hif_power_up()
for every WiFi ON/OFF sequence since the turnaround time for entry/exit of
low power mode is much less. Overhead is just the time taken for sending
QMI MODE OFF & QMI MISSION MODE commands instead of going through the
entire chip boot & QMI init sequence.

Currently the changes are applicable only for WCN6750. This can be
extended to other targets with a future patch.

Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1
Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.16

Signed-off-by: Manikanta Pubbisetty <[email protected]>
---
drivers/net/wireless/ath/ath11k/ahb.c | 45 +++++
drivers/net/wireless/ath/ath11k/core.c | 225 ++++++++++++++++++++-----
drivers/net/wireless/ath/ath11k/core.h | 4 +
drivers/net/wireless/ath/ath11k/hif.h | 11 ++
drivers/net/wireless/ath/ath11k/mac.c | 8 +-
drivers/net/wireless/ath/ath11k/pci.c | 26 +++
6 files changed, 275 insertions(+), 44 deletions(-)

diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c
index d34a4d6325b2..5fe375014ecb 100644
--- a/drivers/net/wireless/ath/ath11k/ahb.c
+++ b/drivers/net/wireless/ath/ath11k/ahb.c
@@ -191,6 +191,47 @@ static const struct ath11k_pci_ops ath11k_ahb_pci_ops_wcn6750 = {
.window_read32 = ath11k_ahb_window_read32_wcn6750,
};

+static int ath11k_ahb_core_start_wcn6750(struct ath11k_base *ab)
+{
+ /* Initialize the hardware/firmware only for the first PDEV
+ * or during hardware recovery.
+ */
+ if (!test_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags) &&
+ ath11k_core_any_pdevs_on(ab))
+ return 0;
+
+ return ath11k_core_start_device(ab);
+}
+
+static void ath11k_ahb_core_stop_wcn6750(struct ath11k_base *ab)
+{
+ return ath11k_core_stop_device(ab);
+}
+
+static int ath11k_ahb_core_start_ipq8074(struct ath11k_base *ab)
+{
+ /* TODO: Currently initializing the hardware/firmware only
+ * during hardware recovery. Support to shutdown/turn-on
+ * the hardware during Wi-Fi OFF/ON will be added later.
+ */
+ if (!test_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags))
+ return 0;
+
+ return ath11k_core_start_device(ab);
+}
+
+static void ath11k_ahb_core_stop_ipq8074(struct ath11k_base *ab)
+{
+ /* TODO: Currently stopping the hardware/firmware only
+ * during driver unload. Support to shutdown/turn-on
+ * the hardware during Wi-Fi OFF/ON will be added later.
+ */
+ if (!test_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags))
+ return;
+
+ return ath11k_core_stop_device(ab);
+}
+
static inline u32 ath11k_ahb_read32(struct ath11k_base *ab, u32 offset)
{
return ioread32(ab->mem + offset);
@@ -778,6 +819,8 @@ static const struct ath11k_hif_ops ath11k_ahb_hif_ops_ipq8074 = {
.map_service_to_pipe = ath11k_ahb_map_service_to_pipe,
.power_down = ath11k_ahb_power_down,
.power_up = ath11k_ahb_power_up,
+ .core_start = ath11k_ahb_core_start_ipq8074,
+ .core_stop = ath11k_ahb_core_stop_ipq8074,
};

static const struct ath11k_hif_ops ath11k_ahb_hif_ops_wcn6750 = {
@@ -797,6 +840,8 @@ static const struct ath11k_hif_ops ath11k_ahb_hif_ops_wcn6750 = {
.resume = ath11k_ahb_hif_resume,
.ce_irq_enable = ath11k_pci_enable_ce_irqs_except_wake_irq,
.ce_irq_disable = ath11k_pci_disable_ce_irqs_except_wake_irq,
+ .core_start = ath11k_ahb_core_start_wcn6750,
+ .core_stop = ath11k_ahb_core_stop_wcn6750,
};

static int ath11k_core_get_rproc(struct ath11k_base *ab)
diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c
index d36e193b2db9..f64bf42048a3 100644
--- a/drivers/net/wireless/ath/ath11k/core.c
+++ b/drivers/net/wireless/ath/ath11k/core.c
@@ -1299,7 +1299,6 @@ static int ath11k_core_soc_create(struct ath11k_base *ab)
static void ath11k_core_soc_destroy(struct ath11k_base *ab)
{
ath11k_debugfs_soc_destroy(ab);
- ath11k_dp_free(ab);
ath11k_reg_free(ab);
ath11k_qmi_deinit_service(ab);
}
@@ -1320,31 +1319,14 @@ static int ath11k_core_pdev_create(struct ath11k_base *ab)
goto err_pdev_debug;
}

- ret = ath11k_mac_register(ab);
- if (ret) {
- ath11k_err(ab, "failed register the radio with mac80211: %d\n", ret);
- goto err_dp_pdev_free;
- }
-
- ret = ath11k_thermal_register(ab);
- if (ret) {
- ath11k_err(ab, "could not register thermal device: %d\n",
- ret);
- goto err_mac_unregister;
- }
-
ret = ath11k_spectral_init(ab);
if (ret) {
ath11k_err(ab, "failed to init spectral %d\n", ret);
- goto err_thermal_unregister;
+ goto err_dp_pdev_free;
}

return 0;

-err_thermal_unregister:
- ath11k_thermal_unregister(ab);
-err_mac_unregister:
- ath11k_mac_unregister(ab);
err_dp_pdev_free:
ath11k_dp_pdev_free(ab);
err_pdev_debug:
@@ -1355,11 +1337,8 @@ static int ath11k_core_pdev_create(struct ath11k_base *ab)

static void ath11k_core_pdev_destroy(struct ath11k_base *ab)
{
- ath11k_spectral_deinit(ab);
ath11k_thermal_unregister(ab);
ath11k_mac_unregister(ab);
- ath11k_hif_irq_disable(ab);
- ath11k_dp_pdev_free(ab);
ath11k_debugfs_pdev_destroy(ab);
}

@@ -1491,26 +1470,20 @@ static int ath11k_core_start_firmware(struct ath11k_base *ab,
return ret;
}

-int ath11k_core_qmi_firmware_ready(struct ath11k_base *ab)
+static int ath11k_core_setup_resources(struct ath11k_base *ab)
{
int ret;

- ret = ath11k_core_start_firmware(ab, ATH11K_FIRMWARE_MODE_NORMAL);
- if (ret) {
- ath11k_err(ab, "failed to start firmware: %d\n", ret);
- return ret;
- }
-
ret = ath11k_ce_init_pipes(ab);
if (ret) {
ath11k_err(ab, "failed to initialize CE: %d\n", ret);
- goto err_firmware_stop;
+ return ret;
}

ret = ath11k_dp_alloc(ab);
if (ret) {
ath11k_err(ab, "failed to init DP: %d\n", ret);
- goto err_firmware_stop;
+ return ret;
}

switch (ath11k_crypto_mode) {
@@ -1524,17 +1497,47 @@ int ath11k_core_qmi_firmware_ready(struct ath11k_base *ab)
break;
default:
ath11k_info(ab, "invalid crypto_mode: %d\n", ath11k_crypto_mode);
- return -EINVAL;
+ ret = -EINVAL;
+ goto err_dp_free;
}

if (ath11k_frame_mode == ATH11K_HW_TXRX_RAW)
set_bit(ATH11K_FLAG_RAW_MODE, &ab->dev_flags);

+ return 0;
+
+err_dp_free:
+ ath11k_dp_free(ab);
+
+ return ret;
+}
+
+static void ath11k_core_free_resources(struct ath11k_base *ab)
+{
+ ath11k_dp_free(ab);
+}
+
+static int ath11k_core_setup_device(struct ath11k_base *ab)
+{
+ int ret;
+
+ ret = ath11k_core_start_firmware(ab, ATH11K_FIRMWARE_MODE_NORMAL);
+ if (ret) {
+ ath11k_err(ab, "failed to start firmware: %d\n", ret);
+ return ret;
+ }
+
+ ret = ath11k_core_setup_resources(ab);
+ if (ret) {
+ ath11k_err(ab, "failed to setup resources: %d\n", ret);
+ goto err_firmware_stop;
+ }
+
mutex_lock(&ab->core_lock);
ret = ath11k_core_start(ab);
if (ret) {
ath11k_err(ab, "failed to start core: %d\n", ret);
- goto err_dp_free;
+ goto err_free_resources;
}

ret = ath11k_core_pdev_create(ab);
@@ -1542,7 +1545,10 @@ int ath11k_core_qmi_firmware_ready(struct ath11k_base *ab)
ath11k_err(ab, "failed to create pdev core: %d\n", ret);
goto err_core_stop;
}
+
ath11k_hif_irq_enable(ab);
+ ath11k_hif_core_stop(ab);
+
mutex_unlock(&ab->core_lock);

return 0;
@@ -1550,8 +1556,8 @@ int ath11k_core_qmi_firmware_ready(struct ath11k_base *ab)
err_core_stop:
ath11k_core_stop(ab);
ath11k_mac_destroy(ab);
-err_dp_free:
- ath11k_dp_free(ab);
+err_free_resources:
+ ath11k_core_free_resources(ab);
mutex_unlock(&ab->core_lock);
err_firmware_stop:
ath11k_qmi_firmware_stop(ab);
@@ -1559,10 +1565,43 @@ int ath11k_core_qmi_firmware_ready(struct ath11k_base *ab)
return ret;
}

+int ath11k_core_qmi_firmware_ready(struct ath11k_base *ab)
+{
+ int ret;
+
+ ret = ath11k_core_setup_device(ab);
+ if (ret) {
+ ath11k_err(ab, "failed to setup device: %d\n", ret);
+ return ret;
+ }
+
+ ret = ath11k_mac_register(ab);
+ if (ret) {
+ ath11k_err(ab, "failed register the radio with mac80211: %d\n", ret);
+ goto err_free_device;
+ }
+
+ ret = ath11k_thermal_register(ab);
+ if (ret) {
+ ath11k_err(ab, "could not register thermal device: %d\n",
+ ret);
+ goto err_mac_unregister;
+ }
+
+ return 0;
+
+err_mac_unregister:
+ ath11k_mac_unregister(ab);
+err_free_device:
+ ath11k_core_stop_device(ab);
+ ath11k_mac_destroy(ab);
+
+ return ret;
+}
+
static void ath11k_core_reconfigure_on_crash(struct ath11k_base *ab)
{
mutex_lock(&ab->core_lock);
- ath11k_thermal_unregister(ab);
ath11k_hif_irq_disable(ab);
ath11k_dp_pdev_free(ab);
ath11k_spectral_deinit(ab);
@@ -1837,7 +1876,6 @@ void ath11k_core_deinit(struct ath11k_base *ab)
mutex_lock(&ab->core_lock);

ath11k_core_pdev_destroy(ab);
- ath11k_core_stop(ab);

mutex_unlock(&ab->core_lock);

@@ -1905,37 +1943,140 @@ struct ath11k_base *ath11k_core_alloc(struct device *dev, size_t priv_size,
}
EXPORT_SYMBOL(ath11k_core_alloc);

+static int ath11k_core_suspend_target(struct ath11k_base *ab, u32 suspend_opt)
+{
+ struct ath11k *ar;
+ struct ath11k_pdev *pdev;
+ unsigned long time_left;
+ int ret;
+ int i;
+
+ for (i = 0; i < ab->num_radios; i++) {
+ pdev = &ab->pdevs[i];
+ ar = pdev->ar;
+
+ reinit_completion(&ab->htc_suspend);
+
+ ret = ath11k_wmi_pdev_suspend(ar, suspend_opt, pdev->pdev_id);
+ if (ret) {
+ ath11k_warn(ab, "could not suspend target (%d)\n", ret);
+ return ret;
+ }
+
+ time_left = wait_for_completion_timeout(&ab->htc_suspend, 3 * HZ);
+
+ if (!time_left) {
+ ath11k_warn(ab, "suspend timed out - target pause event never came\n");
+ return -ETIMEDOUT;
+ }
+ }
+
+ return 0;
+}
+
+void ath11k_core_stop_device(struct ath11k_base *ab)
+{
+ if (ab->core_stopped)
+ return;
+
+ ath11k_spectral_deinit(ab);
+ ath11k_core_suspend_target(ab, WMI_PDEV_SUSPEND_AND_DISABLE_INTR);
+ ath11k_hif_irq_disable(ab);
+ ath11k_hif_stop(ab);
+
+ if (!test_bit(ATH11K_FLAG_CRASH_FLUSH, &ab->dev_flags))
+ ath11k_qmi_firmware_stop(ab);
+
+ ath11k_wmi_detach(ab);
+ ath11k_dp_pdev_reo_cleanup(ab);
+ ath11k_dp_pdev_free(ab);
+ ath11k_dp_free(ab);
+
+ ab->core_stopped = true;
+}
+EXPORT_SYMBOL(ath11k_core_stop_device);
+
+int ath11k_core_any_pdevs_on(struct ath11k_base *ab)
+{
+ struct ath11k_pdev *pdev;
+ struct ath11k *ar;
+ int i;
+
+ for (i = 0; i < ab->num_radios; i++) {
+ pdev = &ab->pdevs[i];
+ ar = pdev->ar;
+ if (!ar)
+ continue;
+
+ if (ar->state == ATH11K_STATE_ON)
+ return true;
+ }
+
+ return false;
+}
+EXPORT_SYMBOL(ath11k_core_any_pdevs_on);
+
int ath11k_core_start_device(struct ath11k_base *ab)
{
int ret;

- if (!test_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags))
- return 0;
+ mutex_lock(&ab->core_lock);

ath11k_hal_srng_deinit(ab);

ret = ath11k_hal_srng_init(ab);
if (ret) {
ath11k_err(ab, "failed to init srng: %d\n", ret);
- return ret;
+ goto err_unlock;
}

clear_bit(ATH11K_FLAG_CRASH_FLUSH, &ab->dev_flags);

- ret = ath11k_core_qmi_firmware_ready(ab);
+ ret = ath11k_core_start_firmware(ab, ATH11K_FIRMWARE_MODE_NORMAL);
if (ret) {
- ath11k_err(ab, "failed to init core: %d\n", ret);
+ ath11k_err(ab, "failed to start firmware: %d\n", ret);
goto err_hal_srng_deinit;
}

+ ret = ath11k_core_setup_resources(ab);
+ if (ret) {
+ ath11k_err(ab, "failed to setup resources: %d\n", ret);
+ goto err_firmware_stop;
+ }
+
+ ret = ath11k_core_start(ab);
+ if (ret) {
+ ath11k_err(ab, "failed to start core: %d\n", ret);
+ goto err_free_resources;
+ }
+
+ ret = ath11k_core_pdev_create(ab);
+ if (ret) {
+ ath11k_err(ab, "failed to create pdev core: %d\n", ret);
+ goto err_core_stop;
+ }
+
+ ath11k_hif_irq_enable(ab);
clear_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags);

+ mutex_unlock(&ab->core_lock);
+
+ ab->core_stopped = false;
return 0;

+err_core_stop:
+ ath11k_core_stop(ab);
+err_free_resources:
+ ath11k_core_free_resources(ab);
+err_firmware_stop:
+ ath11k_qmi_firmware_stop(ab);
err_hal_srng_deinit:
ath11k_hal_srng_deinit(ab);
+err_unlock:
+ mutex_unlock(&ab->core_lock);
return ret;
}
+EXPORT_SYMBOL(ath11k_core_start_device);

MODULE_DESCRIPTION("Core module for Qualcomm Atheros 802.11ax wireless LAN cards.");
MODULE_LICENSE("Dual BSD/GPL");
diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h
index 9b55ac94b36c..338f6f71baa0 100644
--- a/drivers/net/wireless/ath/ath11k/core.h
+++ b/drivers/net/wireless/ath/ath11k/core.h
@@ -964,6 +964,8 @@ struct ath11k_base {
const struct ath11k_pci_ops *ops;
} pci;

+ bool core_stopped;
+
/* must be last */
u8 drv_priv[] __aligned(sizeof(void *));
};
@@ -1156,6 +1158,8 @@ int ath11k_core_resume(struct ath11k_base *ab);
int ath11k_core_suspend(struct ath11k_base *ab);
void ath11k_core_pre_reconfigure_recovery(struct ath11k_base *ab);
int ath11k_core_start_device(struct ath11k_base *ab);
+void ath11k_core_stop_device(struct ath11k_base *ab);
+int ath11k_core_any_pdevs_on(struct ath11k_base *ab);

const struct firmware *ath11k_core_firmware_request(struct ath11k_base *ab,
const char *filename);
diff --git a/drivers/net/wireless/ath/ath11k/hif.h b/drivers/net/wireless/ath/ath11k/hif.h
index 659b80d2abd4..395b3dbf79b8 100644
--- a/drivers/net/wireless/ath/ath11k/hif.h
+++ b/drivers/net/wireless/ath/ath11k/hif.h
@@ -30,6 +30,8 @@ struct ath11k_hif_ops {
void (*ce_irq_enable)(struct ath11k_base *ab);
void (*ce_irq_disable)(struct ath11k_base *ab);
void (*get_ce_msi_idx)(struct ath11k_base *ab, u32 ce_id, u32 *msi_idx);
+ int (*core_start)(struct ath11k_base *ab);
+ void (*core_stop)(struct ath11k_base *ab);
};

static inline void ath11k_hif_ce_irq_enable(struct ath11k_base *ab)
@@ -145,4 +147,13 @@ static inline void ath11k_get_ce_msi_idx(struct ath11k_base *ab, u32 ce_id,
*msi_data_idx = ce_id;
}

+static inline int ath11k_hif_core_start(struct ath11k_base *ab)
+{
+ return ab->hif.ops->core_start(ab);
+}
+
+static inline void ath11k_hif_core_stop(struct ath11k_base *ab)
+{
+ return ab->hif.ops->core_stop(ab);
+}
#endif /* _HIF_H_ */
diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c
index c21ebf0578dc..31b794ea52ad 100644
--- a/drivers/net/wireless/ath/ath11k/mac.c
+++ b/drivers/net/wireless/ath/ath11k/mac.c
@@ -5797,9 +5797,9 @@ static int ath11k_mac_op_start(struct ieee80211_hw *hw)

ath11k_mac_drain_tx(ar);

- ret = ath11k_core_start_device(ab);
+ ret = ath11k_hif_core_start(ab);
if (ret) {
- ath11k_err(ab, "failed to start device : %d\n", ret);
+ ath11k_err(ab, "failed to start core : %d\n", ret);
return ret;
}

@@ -5960,6 +5960,10 @@ static void ath11k_mac_op_stop(struct ieee80211_hw *hw)
synchronize_rcu();

atomic_set(&ar->num_pending_mgmt_tx, 0);
+
+ /* If all PDEVs on the SoC are down, then power down the device */
+ if (!ath11k_core_any_pdevs_on(ar->ab))
+ ath11k_hif_core_stop(ar->ab);
}

static void
diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
index 99cf3357c66e..be350ed671b7 100644
--- a/drivers/net/wireless/ath/ath11k/pci.c
+++ b/drivers/net/wireless/ath/ath11k/pci.c
@@ -680,6 +680,30 @@ static int ath11k_pci_start(struct ath11k_base *ab)
return 0;
}

+static int ath11k_pci_core_start(struct ath11k_base *ab)
+{
+ /* TODO: Currently initializing the hardware/firmware only
+ * during hardware recovery. Support to shutdown/turn-on
+ * the hardware during Wi-Fi OFF/ON will be added later.
+ */
+ if (!test_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags))
+ return 0;
+
+ return ath11k_core_start_device(ab);
+}
+
+static void ath11k_pci_core_stop(struct ath11k_base *ab)
+{
+ /* TODO: Currently stopping the hardware/firmware only
+ * during driver unload. Support to shutdown/turn-on
+ * the hardware during Wi-Fi OFF/ON will be added later.
+ */
+ if (!test_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags))
+ return;
+
+ return ath11k_core_stop_device(ab);
+}
+
static const struct ath11k_hif_ops ath11k_pci_hif_ops = {
.start = ath11k_pci_start,
.stop = ath11k_pcic_stop,
@@ -698,6 +722,8 @@ static const struct ath11k_hif_ops ath11k_pci_hif_ops = {
.ce_irq_enable = ath11k_pci_hif_ce_irq_enable,
.ce_irq_disable = ath11k_pci_hif_ce_irq_disable,
.get_ce_msi_idx = ath11k_pcic_get_ce_msi_idx,
+ .core_start = ath11k_pci_core_start,
+ .core_stop = ath11k_pci_core_stop,
};

static void ath11k_pci_read_hw_version(struct ath11k_base *ab, u32 *major, u32 *minor)
--
2.38.0

2022-11-21 16:22:23

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH v3 0/3] Enable low power mode when WLAN is not active

2022-11-22 09:12:24

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH v3 0/3] Enable low power mode when WLAN is not active

Manikanta Pubbisetty <[email protected]> writes:

> Currently, WLAN chip is powered once during driver probe and is kept
> ON (powered) always even when WLAN is not active; keeping the chip
> powered ON all the time will consume extra power which is not
> desirable for battery operated devices. Same is the case with non-WoW
> suspend, chip will not be put into low power mode when the system is
> suspended resulting in higher battery drain.
>
> Send QMI MODE OFF command to firmware during WiFi OFF to put device
> into low power mode.
>
> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.16
>
> Manikanta Pubbisetty (3):
> ath11k: Fix double free issue during SRNG deinit
> ath11k: Move hardware initialization logic to start()
> ath11k: Enable low power mode when WLAN is not active
> ---
> V3:
> - Removed patch "ath11k: Fix failed to parse regulatory event print" as it is not needed anymore
> - Fixed a potential deadlock scenario reported by lockdep around ab->core_lock with V2 changes
> - Fixed other minor issues that were found during code review
> - Spelling corrections in the commit messages

I still see a crash, immediately after the first rmmod:

Nov 22 11:05:47 nuc2 [ 139.378719] rmmod ath11k_pci
Nov 22 11:05:48 nuc2 [ 139.892395] general protection fault, probably for non-canonical address 0xdffffc000000003e: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN
Nov 22 11:05:48 nuc2 [ 139.892453] KASAN: null-ptr-deref in range [0x00000000000001f0-0x00000000000001f7]

Really odd that you don't see it. Unfortunately not able to debug this
further right now.

This is with:

wcn6855 hw2.0 WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.9

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

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

2022-11-23 16:11:05

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH v3 0/3] Enable low power mode when WLAN is not active

Kalle Valo <[email protected]> writes:

> Manikanta Pubbisetty <[email protected]> writes:
>
>> Currently, WLAN chip is powered once during driver probe and is kept
>> ON (powered) always even when WLAN is not active; keeping the chip
>> powered ON all the time will consume extra power which is not
>> desirable for battery operated devices. Same is the case with non-WoW
>> suspend, chip will not be put into low power mode when the system is
>> suspended resulting in higher battery drain.
>>
>> Send QMI MODE OFF command to firmware during WiFi OFF to put device
>> into low power mode.
>>
>> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1
>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.16
>>
>> Manikanta Pubbisetty (3):
>> ath11k: Fix double free issue during SRNG deinit
>> ath11k: Move hardware initialization logic to start()
>> ath11k: Enable low power mode when WLAN is not active
>> ---
>> V3:
>> - Removed patch "ath11k: Fix failed to parse regulatory event print" as it is not needed anymore
>> - Fixed a potential deadlock scenario reported by lockdep around ab->core_lock with V2 changes
>> - Fixed other minor issues that were found during code review
>> - Spelling corrections in the commit messages
>
> I still see a crash, immediately after the first rmmod:
>
> Nov 22 11:05:47 nuc2 [ 139.378719] rmmod ath11k_pci
> Nov 22 11:05:48 nuc2 [ 139.892395] general protection fault, probably
> for non-canonical address 0xdffffc000000003e: 0000 [#1] PREEMPT SMP
> DEBUG_PAGEALLOC KASAN
> Nov 22 11:05:48 nuc2 [ 139.892453] KASAN: null-ptr-deref in range
> [0x00000000000001f0-0x00000000000001f7]
>
> Really odd that you don't see it. Unfortunately not able to debug this
> further right now.
>
> This is with:
>
> wcn6855 hw2.0 WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.9

A bit more information how I see the crash. So first I have all modules
loaded:

$ lsmod
Module Size Used by
ath11k_pci 57344 0
ath11k 2015232 1 ath11k_pci
mac80211 3284992 1 ath11k
libarc4 16384 1 mac80211
cfg80211 2494464 2 ath11k,mac80211
qmi_helpers 57344 1 ath11k
qrtr_mhi 20480 0
mhi 217088 2 ath11k_pci,qrtr_mhi
qrtr 98304 5 qrtr_mhi
nvme 122880 3
nvme_core 299008 5 nvme
$

Then I just remove ath11k_pci module and boom:

$ sudo rmmod ath11k_pci

[ 153.658409] general protection fault, probably for non-canonical address 0xdffffc000000003e: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN

This happens every time, there doesn't seem to be any randomness on the
behaviour.

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

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

2022-11-25 04:34:42

by Manikanta Pubbisetty

[permalink] [raw]

Subject: Re: [PATCH v3 0/3] Enable low power mode when WLAN is not active

On 11/23/2022 9:35 PM, Kalle Valo wrote:
> Kalle Valo <[email protected]> writes:
>
>> Manikanta Pubbisetty <[email protected]> writes:
>>
>>> Currently, WLAN chip is powered once during driver probe and is kept
>>> ON (powered) always even when WLAN is not active; keeping the chip
>>> powered ON all the time will consume extra power which is not
>>> desirable for battery operated devices. Same is the case with non-WoW
>>> suspend, chip will not be put into low power mode when the system is
>>> suspended resulting in higher battery drain.
>>>
>>> Send QMI MODE OFF command to firmware during WiFi OFF to put device
>>> into low power mode.
>>>
>>> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1
>>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.16
>>>
>>> Manikanta Pubbisetty (3):
>>> ath11k: Fix double free issue during SRNG deinit
>>> ath11k: Move hardware initialization logic to start()
>>> ath11k: Enable low power mode when WLAN is not active
>>> ---
>>> V3:
>>> - Removed patch "ath11k: Fix failed to parse regulatory event print" as it is not needed anymore
>>> - Fixed a potential deadlock scenario reported by lockdep around ab->core_lock with V2 changes
>>> - Fixed other minor issues that were found during code review
>>> - Spelling corrections in the commit messages
>>
>> I still see a crash, immediately after the first rmmod:
>>
>> Nov 22 11:05:47 nuc2 [ 139.378719] rmmod ath11k_pci
>> Nov 22 11:05:48 nuc2 [ 139.892395] general protection fault, probably
>> for non-canonical address 0xdffffc000000003e: 0000 [#1] PREEMPT SMP
>> DEBUG_PAGEALLOC KASAN
>> Nov 22 11:05:48 nuc2 [ 139.892453] KASAN: null-ptr-deref in range
>> [0x00000000000001f0-0x00000000000001f7]
>>
>> Really odd that you don't see it. Unfortunately not able to debug this
>> further right now.
>>
>> This is with:
>>
>> wcn6855 hw2.0 WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.9
>
> A bit more information how I see the crash. So first I have all modules
> loaded:
>
> $ lsmod
> Module Size Used by
> ath11k_pci 57344 0
> ath11k 2015232 1 ath11k_pci
> mac80211 3284992 1 ath11k
> libarc4 16384 1 mac80211
> cfg80211 2494464 2 ath11k,mac80211
> qmi_helpers 57344 1 ath11k
> qrtr_mhi 20480 0
> mhi 217088 2 ath11k_pci,qrtr_mhi
> qrtr 98304 5 qrtr_mhi
> nvme 122880 3
> nvme_core 299008 5 nvme
> $
>
> Then I just remove ath11k_pci module and boom:
>
> $ sudo rmmod ath11k_pci
>
> [ 153.658409] general protection fault, probably for non-canonical address 0xdffffc000000003e: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN
>
> This happens every time, there doesn't seem to be any randomness on the
> behaviour.
>

Thanks for the help Kalle, this is exactly what I was doing in my tests.
Unfortunately, I'm not able to reproduce the problem. I have also tried
with the exact firmware that you have pointed out. Let me see if I'm
missing anything.

Thanks,
Manikanta

2022-12-08 08:00:02

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH v3 0/3] Enable low power mode when WLAN is not active

Manikanta Pubbisetty <[email protected]> writes:

> On 11/23/2022 9:35 PM, Kalle Valo wrote:
>
>> Kalle Valo <[email protected]> writes:
>>
>>> Manikanta Pubbisetty <[email protected]> writes:
>>>
>>>> Currently, WLAN chip is powered once during driver probe and is kept
>>>> ON (powered) always even when WLAN is not active; keeping the chip
>>>> powered ON all the time will consume extra power which is not
>>>> desirable for battery operated devices. Same is the case with non-WoW
>>>> suspend, chip will not be put into low power mode when the system is
>>>> suspended resulting in higher battery drain.
>>>>
>>>> Send QMI MODE OFF command to firmware during WiFi OFF to put device
>>>> into low power mode.
>>>>
>>>> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1
>>>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.16
>>>>
>>>> Manikanta Pubbisetty (3):
>>>> ath11k: Fix double free issue during SRNG deinit
>>>> ath11k: Move hardware initialization logic to start()
>>>> ath11k: Enable low power mode when WLAN is not active
>>>> ---
>>>> V3:
>>>> - Removed patch "ath11k: Fix failed to parse regulatory event print" as it is not needed anymore
>>>> - Fixed a potential deadlock scenario reported by lockdep around ab->core_lock with V2 changes
>>>> - Fixed other minor issues that were found during code review
>>>> - Spelling corrections in the commit messages
>>>
>>> I still see a crash, immediately after the first rmmod:
>>>
>>> Nov 22 11:05:47 nuc2 [ 139.378719] rmmod ath11k_pci
>>> Nov 22 11:05:48 nuc2 [ 139.892395] general protection fault, probably
>>> for non-canonical address 0xdffffc000000003e: 0000 [#1] PREEMPT SMP
>>> DEBUG_PAGEALLOC KASAN
>>> Nov 22 11:05:48 nuc2 [ 139.892453] KASAN: null-ptr-deref in range
>>> [0x00000000000001f0-0x00000000000001f7]
>>>
>>> Really odd that you don't see it. Unfortunately not able to debug this
>>> further right now.
>>>
>>> This is with:
>>>
>>> wcn6855 hw2.0 WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.9
>>
>> A bit more information how I see the crash. So first I have all modules
>> loaded:
>>
>> $ lsmod
>> Module Size Used by
>> ath11k_pci 57344 0
>> ath11k 2015232 1 ath11k_pci
>> mac80211 3284992 1 ath11k
>> libarc4 16384 1 mac80211
>> cfg80211 2494464 2 ath11k,mac80211
>> qmi_helpers 57344 1 ath11k
>> qrtr_mhi 20480 0
>> mhi 217088 2 ath11k_pci,qrtr_mhi
>> qrtr 98304 5 qrtr_mhi
>> nvme 122880 3
>> nvme_core 299008 5 nvme
>> $
>>
>> Then I just remove ath11k_pci module and boom:
>>
>> $ sudo rmmod ath11k_pci
>>
>> [ 153.658409] general protection fault, probably for non-canonical
>> address 0xdffffc000000003e: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
>> KASAN
>>
>> This happens every time, there doesn't seem to be any randomness on the
>> behaviour.
>>
>
> Thanks for the help Kalle, this is exactly what I was doing in my
> tests. Unfortunately, I'm not able to reproduce the problem. I have
> also tried with the exact firmware that you have pointed out. Let me
> see if I'm missing anything.

I tested this more and patch 3 seems to be the one causing the crash. I
didn't see this when patch 1-2 were applied.

The crash happens in ath11k_dp_process_rxdma_err() in this line:

srng = &ab->hal.srng_list[err_ring->ring_id];

ab looks sane to me (0xffff88814c960000) but err_ring is set to 0x200.
Does this help?

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

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

2022-12-08 11:23:44

by Manikanta Pubbisetty

[permalink] [raw]

Subject: Re: [PATCH v3 0/3] Enable low power mode when WLAN is not active

On 12/8/2022 1:24 PM, Kalle Valo wrote:
> Manikanta Pubbisetty <[email protected]> writes:
>
>> On 11/23/2022 9:35 PM, Kalle Valo wrote:
>>
>>> Kalle Valo <[email protected]> writes:
>>>
>>>> Manikanta Pubbisetty <[email protected]> writes:
>>>>
>>>>> Currently, WLAN chip is powered once during driver probe and is kept
>>>>> ON (powered) always even when WLAN is not active; keeping the chip
>>>>> powered ON all the time will consume extra power which is not
>>>>> desirable for battery operated devices. Same is the case with non-WoW
>>>>> suspend, chip will not be put into low power mode when the system is
>>>>> suspended resulting in higher battery drain.
>>>>>
>>>>> Send QMI MODE OFF command to firmware during WiFi OFF to put device
>>>>> into low power mode.
>>>>>
>>>>> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00887-QCAMSLSWPLZ-1
>>>>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.16
>>>>>
>>>>> Manikanta Pubbisetty (3):
>>>>> ath11k: Fix double free issue during SRNG deinit
>>>>> ath11k: Move hardware initialization logic to start()
>>>>> ath11k: Enable low power mode when WLAN is not active
>>>>> ---
>>>>> V3:
>>>>> - Removed patch "ath11k: Fix failed to parse regulatory event print" as it is not needed anymore
>>>>> - Fixed a potential deadlock scenario reported by lockdep around ab->core_lock with V2 changes
>>>>> - Fixed other minor issues that were found during code review
>>>>> - Spelling corrections in the commit messages
>>>>
>>>> I still see a crash, immediately after the first rmmod:
>>>>
>>>> Nov 22 11:05:47 nuc2 [ 139.378719] rmmod ath11k_pci
>>>> Nov 22 11:05:48 nuc2 [ 139.892395] general protection fault, probably
>>>> for non-canonical address 0xdffffc000000003e: 0000 [#1] PREEMPT SMP
>>>> DEBUG_PAGEALLOC KASAN
>>>> Nov 22 11:05:48 nuc2 [ 139.892453] KASAN: null-ptr-deref in range
>>>> [0x00000000000001f0-0x00000000000001f7]
>>>>
>>>> Really odd that you don't see it. Unfortunately not able to debug this
>>>> further right now.
>>>>
>>>> This is with:
>>>>
>>>> wcn6855 hw2.0 WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.9
>>>
>>> A bit more information how I see the crash. So first I have all modules
>>> loaded:
>>>
>>> $ lsmod
>>> Module Size Used by
>>> ath11k_pci 57344 0
>>> ath11k 2015232 1 ath11k_pci
>>> mac80211 3284992 1 ath11k
>>> libarc4 16384 1 mac80211
>>> cfg80211 2494464 2 ath11k,mac80211
>>> qmi_helpers 57344 1 ath11k
>>> qrtr_mhi 20480 0
>>> mhi 217088 2 ath11k_pci,qrtr_mhi
>>> qrtr 98304 5 qrtr_mhi
>>> nvme 122880 3
>>> nvme_core 299008 5 nvme
>>> $
>>>
>>> Then I just remove ath11k_pci module and boom:
>>>
>>> $ sudo rmmod ath11k_pci
>>>
>>> [ 153.658409] general protection fault, probably for non-canonical
>>> address 0xdffffc000000003e: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
>>> KASAN
>>>
>>> This happens every time, there doesn't seem to be any randomness on the
>>> behaviour.
>>>
>>
>> Thanks for the help Kalle, this is exactly what I was doing in my
>> tests. Unfortunately, I'm not able to reproduce the problem. I have
>> also tried with the exact firmware that you have pointed out. Let me
>> see if I'm missing anything.
>
> I tested this more and patch 3 seems to be the one causing the crash. I
> didn't see this when patch 1-2 were applied.
>
> The crash happens in ath11k_dp_process_rxdma_err() in this line:
>
> srng = &ab->hal.srng_list[err_ring->ring_id];
>
> ab looks sane to me (0xffff88814c960000) but err_ring is set to 0x200.
> Does this help?
>

Thanks for your time and analysis on the bug.

From the callstack() that you have shared earlier, it looks like
ath11k_dp_process_rxdma_err() is called from dp_service_srngs which is a
napi poll callback.

To me it looks like the napi handler of the driver is getting called
after the srng resources have been de-initialized during rmmod.

I have closed examined the changes in patch 3 (I'm still doing it). so
far, I could not find anything that could trigger this kind of a
problem. Since NAPI is disabled upon rmmod, NAPI poll should not be called.

It's quite not clear if I'm missing something.

Thanks,
Manikanta