* [PATCH 0/2] ath11k: add support for get_txpower mac ops @ 2022-06-02 5:14 ` Aditya Kumar Singh 0 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh @ 2022-06-02 5:14 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, Aditya Kumar Singh Currently, driver does not support get_txpower mac ops because of which cfg80211 returns vif->bss_conf.txpower to user space. bss_conf.txpower gets its value from ieee80211_channel->max_reg_power. However, the final txpower is dependent on few other parameters apart from max regulatory supported power. It is the firmware which knows about all these parameters and considers the minimum for each packet transmission. Firmware reports the final tx power in firmware pdev stats which falls under fw_stats. But currently, fw_stats is under debugfs. Add support for get_txpower mac ops to get the tx power from firmware leveraging fw_stats and return it accordingly. Also, move fw_stats out of debugfs so that get_txpower mac ops can function properly even when debugfs is disabled. Aditya Kumar Singh (2): ath11k: move firmware stats out of debugfs ath11k: add get_txpower mac ops drivers/net/wireless/ath/ath11k/core.c | 46 ++++++++ drivers/net/wireless/ath/ath11k/core.h | 12 +- drivers/net/wireless/ath/ath11k/debugfs.c | 131 +++++----------------- drivers/net/wireless/ath/ath11k/debugfs.h | 6 +- drivers/net/wireless/ath/ath11k/mac.c | 91 +++++++++++++++ drivers/net/wireless/ath/ath11k/wmi.c | 48 +++++++- 6 files changed, 227 insertions(+), 107 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 0/2] ath11k: add support for get_txpower mac ops @ 2022-06-02 5:14 ` Aditya Kumar Singh 0 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh @ 2022-06-02 5:14 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, Aditya Kumar Singh Currently, driver does not support get_txpower mac ops because of which cfg80211 returns vif->bss_conf.txpower to user space. bss_conf.txpower gets its value from ieee80211_channel->max_reg_power. However, the final txpower is dependent on few other parameters apart from max regulatory supported power. It is the firmware which knows about all these parameters and considers the minimum for each packet transmission. Firmware reports the final tx power in firmware pdev stats which falls under fw_stats. But currently, fw_stats is under debugfs. Add support for get_txpower mac ops to get the tx power from firmware leveraging fw_stats and return it accordingly. Also, move fw_stats out of debugfs so that get_txpower mac ops can function properly even when debugfs is disabled. Aditya Kumar Singh (2): ath11k: move firmware stats out of debugfs ath11k: add get_txpower mac ops drivers/net/wireless/ath/ath11k/core.c | 46 ++++++++ drivers/net/wireless/ath/ath11k/core.h | 12 +- drivers/net/wireless/ath/ath11k/debugfs.c | 131 +++++----------------- drivers/net/wireless/ath/ath11k/debugfs.h | 6 +- drivers/net/wireless/ath/ath11k/mac.c | 91 +++++++++++++++ drivers/net/wireless/ath/ath11k/wmi.c | 48 +++++++- 6 files changed, 227 insertions(+), 107 deletions(-) -- 2.17.1 -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/2] ath11k: move firmware stats out of debugfs 2022-06-02 5:14 ` Aditya Kumar Singh @ 2022-06-02 5:14 ` Aditya Kumar Singh -1 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh @ 2022-06-02 5:14 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, Aditya Kumar Singh Currently, firmware stats, comprising pdev, vdev and beacon stats are part of debugfs. In firmware pdev stats, firmware reports the final Tx power used to transmit each packet. If driver wants to know the final Tx power being used at firmware level, it can leverage from firmware pdev stats. Move firmware stats out of debugfs context in order to leverage the final Tx power reported in it even when debugfs is disabled. Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> --- drivers/net/wireless/ath/ath11k/core.c | 46 ++++++++ drivers/net/wireless/ath/ath11k/core.h | 12 +- drivers/net/wireless/ath/ath11k/debugfs.c | 131 +++++----------------- drivers/net/wireless/ath/ath11k/debugfs.h | 6 +- drivers/net/wireless/ath/ath11k/wmi.c | 48 +++++++- 5 files changed, 136 insertions(+), 107 deletions(-) diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c index ca6fadd64b43..07a299a2e213 100644 --- a/drivers/net/wireless/ath/ath11k/core.c +++ b/drivers/net/wireless/ath/ath11k/core.c @@ -570,6 +570,52 @@ static inline struct ath11k_pdev *ath11k_core_get_single_pdev(struct ath11k_base return &ab->pdevs[0]; } +void ath11k_fw_stats_pdevs_free(struct list_head *head) +{ + struct ath11k_fw_stats_pdev *i, *tmp; + + list_for_each_entry_safe(i, tmp, head, list) { + list_del(&i->list); + kfree(i); + } +} + +void ath11k_fw_stats_vdevs_free(struct list_head *head) +{ + struct ath11k_fw_stats_vdev *i, *tmp; + + list_for_each_entry_safe(i, tmp, head, list) { + list_del(&i->list); + kfree(i); + } +} + +void ath11k_fw_stats_bcn_free(struct list_head *head) +{ + struct ath11k_fw_stats_bcn *i, *tmp; + + list_for_each_entry_safe(i, tmp, head, list) { + list_del(&i->list); + kfree(i); + } +} + +void ath11k_fw_stats_init(struct ath11k *ar) +{ + INIT_LIST_HEAD(&ar->fw_stats.pdevs); + INIT_LIST_HEAD(&ar->fw_stats.vdevs); + INIT_LIST_HEAD(&ar->fw_stats.bcn); + + init_completion(&ar->fw_stats_complete); +} + +void ath11k_fw_stats_free(struct ath11k_fw_stats *stats) +{ + ath11k_fw_stats_pdevs_free(&stats->pdevs); + ath11k_fw_stats_vdevs_free(&stats->vdevs); + ath11k_fw_stats_bcn_free(&stats->bcn); +} + int ath11k_core_suspend(struct ath11k_base *ab) { int ret; diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h index 65d54e9c15d9..672701f40a6b 100644 --- a/drivers/net/wireless/ath/ath11k/core.h +++ b/drivers/net/wireless/ath/ath11k/core.h @@ -545,9 +545,6 @@ struct ath11k_debug { struct dentry *debugfs_pdev; struct ath11k_dbg_htt_stats htt_stats; u32 extd_tx_stats; - struct ath11k_fw_stats fw_stats; - struct completion fw_stats_complete; - bool fw_stats_done; u32 extd_rx_stats; u32 pktlog_filter; u32 pktlog_mode; @@ -712,6 +709,9 @@ struct ath11k { u8 twt_enabled; bool nlo_enabled; u8 alpha2[REG_ALPHA2_LEN + 1]; + struct ath11k_fw_stats fw_stats; + struct completion fw_stats_complete; + bool fw_stats_done; }; struct ath11k_band_cap { @@ -1117,6 +1117,12 @@ struct ath11k_fw_stats_bcn { u32 tx_bcn_outage_cnt; }; +void ath11k_fw_stats_init(struct ath11k *ar); +void ath11k_fw_stats_pdevs_free(struct list_head *head); +void ath11k_fw_stats_vdevs_free(struct list_head *head); +void ath11k_fw_stats_bcn_free(struct list_head *head); +void ath11k_fw_stats_free(struct ath11k_fw_stats *stats); + extern const struct ce_pipe_config ath11k_target_ce_config_wlan_ipq8074[]; extern const struct service_to_pipe ath11k_target_service_to_ce_map_wlan_ipq8074[]; extern const struct service_to_pipe ath11k_target_service_to_ce_map_wlan_ipq6018[]; diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c index 0d4843ef9dd1..e4a2fd3da481 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs.c +++ b/drivers/net/wireless/ath/ath11k/debugfs.c @@ -93,91 +93,36 @@ void ath11k_debugfs_add_dbring_entry(struct ath11k *ar, spin_unlock_bh(&dbr_data->lock); } -static void ath11k_fw_stats_pdevs_free(struct list_head *head) -{ - struct ath11k_fw_stats_pdev *i, *tmp; - - list_for_each_entry_safe(i, tmp, head, list) { - list_del(&i->list); - kfree(i); - } -} - -static void ath11k_fw_stats_vdevs_free(struct list_head *head) -{ - struct ath11k_fw_stats_vdev *i, *tmp; - - list_for_each_entry_safe(i, tmp, head, list) { - list_del(&i->list); - kfree(i); - } -} - -static void ath11k_fw_stats_bcn_free(struct list_head *head) -{ - struct ath11k_fw_stats_bcn *i, *tmp; - - list_for_each_entry_safe(i, tmp, head, list) { - list_del(&i->list); - kfree(i); - } -} static void ath11k_debugfs_fw_stats_reset(struct ath11k *ar) { spin_lock_bh(&ar->data_lock); - ar->debug.fw_stats_done = false; - ath11k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs); - ath11k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs); + ar->fw_stats_done = false; + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); + ath11k_fw_stats_vdevs_free(&ar->fw_stats.vdevs); spin_unlock_bh(&ar->data_lock); } -void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb) +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats *stats) { - struct ath11k_fw_stats stats = {}; - struct ath11k *ar; + struct ath11k_base *ab = ar->ab; struct ath11k_pdev *pdev; bool is_end; static unsigned int num_vdev, num_bcn; size_t total_vdevs_started = 0; - int i, ret; - - INIT_LIST_HEAD(&stats.pdevs); - INIT_LIST_HEAD(&stats.vdevs); - INIT_LIST_HEAD(&stats.bcn); - - ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); - if (ret) { - ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); - goto free; - } - - rcu_read_lock(); - ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); - if (!ar) { - rcu_read_unlock(); - ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", - stats.pdev_id, ret); - goto free; - } - - spin_lock_bh(&ar->data_lock); + int i; - if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { - list_splice_tail_init(&stats.pdevs, &ar->debug.fw_stats.pdevs); - ar->debug.fw_stats_done = true; - goto complete; - } + /* WMI_REQUEST_PDEV_STAT request has been already processed */ - if (stats.stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { - ar->debug.fw_stats_done = true; - goto complete; + if (stats->stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { + ar->fw_stats_done = true; + return; } - if (stats.stats_id == WMI_REQUEST_VDEV_STAT) { - if (list_empty(&stats.vdevs)) { + if (stats->stats_id == WMI_REQUEST_VDEV_STAT) { + if (list_empty(&stats->vdevs)) { ath11k_warn(ab, "empty vdev stats"); - goto complete; + return; } /* FW sends all the active VDEV stats irrespective of PDEV, * hence limit until the count of all VDEVs started @@ -190,43 +135,34 @@ void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb is_end = ((++num_vdev) == total_vdevs_started); - list_splice_tail_init(&stats.vdevs, - &ar->debug.fw_stats.vdevs); + list_splice_tail_init(&stats->vdevs, + &ar->fw_stats.vdevs); if (is_end) { - ar->debug.fw_stats_done = true; + ar->fw_stats_done = true; num_vdev = 0; } - goto complete; + return; } - if (stats.stats_id == WMI_REQUEST_BCN_STAT) { - if (list_empty(&stats.bcn)) { + if (stats->stats_id == WMI_REQUEST_BCN_STAT) { + if (list_empty(&stats->bcn)) { ath11k_warn(ab, "empty bcn stats"); - goto complete; + return; } /* Mark end until we reached the count of all started VDEVs * within the PDEV */ is_end = ((++num_bcn) == ar->num_started_vdevs); - list_splice_tail_init(&stats.bcn, - &ar->debug.fw_stats.bcn); + list_splice_tail_init(&stats->bcn, + &ar->fw_stats.bcn); if (is_end) { - ar->debug.fw_stats_done = true; + ar->fw_stats_done = true; num_bcn = 0; } } -complete: - complete(&ar->debug.fw_stats_complete); - rcu_read_unlock(); - spin_unlock_bh(&ar->data_lock); - -free: - ath11k_fw_stats_pdevs_free(&stats.pdevs); - ath11k_fw_stats_vdevs_free(&stats.vdevs); - ath11k_fw_stats_bcn_free(&stats.bcn); } static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, @@ -247,7 +183,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, ath11k_debugfs_fw_stats_reset(ar); - reinit_completion(&ar->debug.fw_stats_complete); + reinit_completion(&ar->fw_stats_complete); ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); @@ -258,7 +194,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, } time_left = - wait_for_completion_timeout(&ar->debug.fw_stats_complete, + wait_for_completion_timeout(&ar->fw_stats_complete, 1 * HZ); if (!time_left) return -ETIMEDOUT; @@ -268,7 +204,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, break; spin_lock_bh(&ar->data_lock); - if (ar->debug.fw_stats_done) { + if (ar->fw_stats_done) { spin_unlock_bh(&ar->data_lock); break; } @@ -340,7 +276,7 @@ static int ath11k_open_pdev_stats(struct inode *inode, struct file *file) goto err_free; } - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, buf); file->private_data = buf; @@ -412,7 +348,7 @@ static int ath11k_open_vdev_stats(struct inode *inode, struct file *file) goto err_free; } - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, buf); file->private_data = buf; @@ -490,14 +426,14 @@ static int ath11k_open_bcn_stats(struct inode *inode, struct file *file) } } - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, buf); /* since beacon stats request is looped for all active VDEVs, saved fw * stats is not freed for each request until done for all active VDEVs */ spin_lock_bh(&ar->data_lock); - ath11k_fw_stats_bcn_free(&ar->debug.fw_stats.bcn); + ath11k_fw_stats_bcn_free(&ar->fw_stats.bcn); spin_unlock_bh(&ar->data_lock); file->private_data = buf; @@ -1055,7 +991,7 @@ void ath11k_debugfs_fw_stats_init(struct ath11k *ar) struct dentry *fwstats_dir = debugfs_create_dir("fw_stats", ar->debug.debugfs_pdev); - ar->debug.fw_stats.debugfs_fwstats = fwstats_dir; + ar->fw_stats.debugfs_fwstats = fwstats_dir; /* all stats debugfs files created are under "fw_stats" directory * created per PDEV @@ -1067,11 +1003,6 @@ void ath11k_debugfs_fw_stats_init(struct ath11k *ar) debugfs_create_file("beacon_stats", 0600, fwstats_dir, ar, &fops_bcn_stats); - INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs); - INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs); - INIT_LIST_HEAD(&ar->debug.fw_stats.bcn); - - init_completion(&ar->debug.fw_stats_complete); } static ssize_t ath11k_write_pktlog_filter(struct file *file, diff --git a/drivers/net/wireless/ath/ath11k/debugfs.h b/drivers/net/wireless/ath/ath11k/debugfs.h index 8fc125a71943..e45dc874ff23 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs.h +++ b/drivers/net/wireless/ath/ath11k/debugfs.h @@ -271,7 +271,7 @@ int ath11k_debugfs_pdev_create(struct ath11k_base *ab); void ath11k_debugfs_pdev_destroy(struct ath11k_base *ab); int ath11k_debugfs_register(struct ath11k *ar); void ath11k_debugfs_unregister(struct ath11k *ar); -void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb); +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats *stats); void ath11k_debugfs_fw_stats_init(struct ath11k *ar); int ath11k_debugfs_get_fw_stats(struct ath11k *ar, u32 pdev_id, @@ -352,8 +352,8 @@ static inline void ath11k_debugfs_unregister(struct ath11k *ar) { } -static inline void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, - struct sk_buff *skb) +static inline void ath11k_debugfs_fw_stats_process(struct ath11k *ar, + struct ath11k_fw_stats *stats) { } diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c index 84d1c7054013..9c7a0a438b12 100644 --- a/drivers/net/wireless/ath/ath11k/wmi.c +++ b/drivers/net/wireless/ath/ath11k/wmi.c @@ -7412,7 +7412,53 @@ static void ath11k_peer_assoc_conf_event(struct ath11k_base *ab, struct sk_buff static void ath11k_update_stats_event(struct ath11k_base *ab, struct sk_buff *skb) { - ath11k_debugfs_fw_stats_process(ab, skb); + struct ath11k_fw_stats stats = {}; + struct ath11k *ar; + int ret; + + INIT_LIST_HEAD(&stats.pdevs); + INIT_LIST_HEAD(&stats.vdevs); + INIT_LIST_HEAD(&stats.bcn); + + ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); + if (ret) { + ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); + goto free; + } + + rcu_read_lock(); + ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); + if (!ar) { + rcu_read_unlock(); + ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", + stats.pdev_id, ret); + goto free; + } + + spin_lock_bh(&ar->data_lock); + + /* WMI_REQUEST_PDEV_STAT can be requested via .get_txpower mac ops or via + * debugfs fw stats. Therefore, processing it separately. + */ + if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { + list_splice_tail_init(&stats.pdevs, &ar->fw_stats.pdevs); + ar->fw_stats_done = true; + goto complete; + } + + /* WMI_REQUEST_VDEV_STAT, WMI_REQUEST_BCN_STAT and WMI_REQUEST_RSSI_PER_CHAIN_STAT + * are currently requested only via debugfs fw stats. Hence, processing these + * in debugfs context + */ + ath11k_debugfs_fw_stats_process(ar, &stats); + +complete: + complete(&ar->fw_stats_complete); + rcu_read_unlock(); + spin_unlock_bh(&ar->data_lock); + +free: + ath11k_fw_stats_free(&stats); } /* PDEV_CTL_FAILSAFE_CHECK_EVENT is received from FW when the frequency scanned -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 1/2] ath11k: move firmware stats out of debugfs @ 2022-06-02 5:14 ` Aditya Kumar Singh 0 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh @ 2022-06-02 5:14 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, Aditya Kumar Singh Currently, firmware stats, comprising pdev, vdev and beacon stats are part of debugfs. In firmware pdev stats, firmware reports the final Tx power used to transmit each packet. If driver wants to know the final Tx power being used at firmware level, it can leverage from firmware pdev stats. Move firmware stats out of debugfs context in order to leverage the final Tx power reported in it even when debugfs is disabled. Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> --- drivers/net/wireless/ath/ath11k/core.c | 46 ++++++++ drivers/net/wireless/ath/ath11k/core.h | 12 +- drivers/net/wireless/ath/ath11k/debugfs.c | 131 +++++----------------- drivers/net/wireless/ath/ath11k/debugfs.h | 6 +- drivers/net/wireless/ath/ath11k/wmi.c | 48 +++++++- 5 files changed, 136 insertions(+), 107 deletions(-) diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c index ca6fadd64b43..07a299a2e213 100644 --- a/drivers/net/wireless/ath/ath11k/core.c +++ b/drivers/net/wireless/ath/ath11k/core.c @@ -570,6 +570,52 @@ static inline struct ath11k_pdev *ath11k_core_get_single_pdev(struct ath11k_base return &ab->pdevs[0]; } +void ath11k_fw_stats_pdevs_free(struct list_head *head) +{ + struct ath11k_fw_stats_pdev *i, *tmp; + + list_for_each_entry_safe(i, tmp, head, list) { + list_del(&i->list); + kfree(i); + } +} + +void ath11k_fw_stats_vdevs_free(struct list_head *head) +{ + struct ath11k_fw_stats_vdev *i, *tmp; + + list_for_each_entry_safe(i, tmp, head, list) { + list_del(&i->list); + kfree(i); + } +} + +void ath11k_fw_stats_bcn_free(struct list_head *head) +{ + struct ath11k_fw_stats_bcn *i, *tmp; + + list_for_each_entry_safe(i, tmp, head, list) { + list_del(&i->list); + kfree(i); + } +} + +void ath11k_fw_stats_init(struct ath11k *ar) +{ + INIT_LIST_HEAD(&ar->fw_stats.pdevs); + INIT_LIST_HEAD(&ar->fw_stats.vdevs); + INIT_LIST_HEAD(&ar->fw_stats.bcn); + + init_completion(&ar->fw_stats_complete); +} + +void ath11k_fw_stats_free(struct ath11k_fw_stats *stats) +{ + ath11k_fw_stats_pdevs_free(&stats->pdevs); + ath11k_fw_stats_vdevs_free(&stats->vdevs); + ath11k_fw_stats_bcn_free(&stats->bcn); +} + int ath11k_core_suspend(struct ath11k_base *ab) { int ret; diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h index 65d54e9c15d9..672701f40a6b 100644 --- a/drivers/net/wireless/ath/ath11k/core.h +++ b/drivers/net/wireless/ath/ath11k/core.h @@ -545,9 +545,6 @@ struct ath11k_debug { struct dentry *debugfs_pdev; struct ath11k_dbg_htt_stats htt_stats; u32 extd_tx_stats; - struct ath11k_fw_stats fw_stats; - struct completion fw_stats_complete; - bool fw_stats_done; u32 extd_rx_stats; u32 pktlog_filter; u32 pktlog_mode; @@ -712,6 +709,9 @@ struct ath11k { u8 twt_enabled; bool nlo_enabled; u8 alpha2[REG_ALPHA2_LEN + 1]; + struct ath11k_fw_stats fw_stats; + struct completion fw_stats_complete; + bool fw_stats_done; }; struct ath11k_band_cap { @@ -1117,6 +1117,12 @@ struct ath11k_fw_stats_bcn { u32 tx_bcn_outage_cnt; }; +void ath11k_fw_stats_init(struct ath11k *ar); +void ath11k_fw_stats_pdevs_free(struct list_head *head); +void ath11k_fw_stats_vdevs_free(struct list_head *head); +void ath11k_fw_stats_bcn_free(struct list_head *head); +void ath11k_fw_stats_free(struct ath11k_fw_stats *stats); + extern const struct ce_pipe_config ath11k_target_ce_config_wlan_ipq8074[]; extern const struct service_to_pipe ath11k_target_service_to_ce_map_wlan_ipq8074[]; extern const struct service_to_pipe ath11k_target_service_to_ce_map_wlan_ipq6018[]; diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c index 0d4843ef9dd1..e4a2fd3da481 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs.c +++ b/drivers/net/wireless/ath/ath11k/debugfs.c @@ -93,91 +93,36 @@ void ath11k_debugfs_add_dbring_entry(struct ath11k *ar, spin_unlock_bh(&dbr_data->lock); } -static void ath11k_fw_stats_pdevs_free(struct list_head *head) -{ - struct ath11k_fw_stats_pdev *i, *tmp; - - list_for_each_entry_safe(i, tmp, head, list) { - list_del(&i->list); - kfree(i); - } -} - -static void ath11k_fw_stats_vdevs_free(struct list_head *head) -{ - struct ath11k_fw_stats_vdev *i, *tmp; - - list_for_each_entry_safe(i, tmp, head, list) { - list_del(&i->list); - kfree(i); - } -} - -static void ath11k_fw_stats_bcn_free(struct list_head *head) -{ - struct ath11k_fw_stats_bcn *i, *tmp; - - list_for_each_entry_safe(i, tmp, head, list) { - list_del(&i->list); - kfree(i); - } -} static void ath11k_debugfs_fw_stats_reset(struct ath11k *ar) { spin_lock_bh(&ar->data_lock); - ar->debug.fw_stats_done = false; - ath11k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs); - ath11k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs); + ar->fw_stats_done = false; + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); + ath11k_fw_stats_vdevs_free(&ar->fw_stats.vdevs); spin_unlock_bh(&ar->data_lock); } -void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb) +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats *stats) { - struct ath11k_fw_stats stats = {}; - struct ath11k *ar; + struct ath11k_base *ab = ar->ab; struct ath11k_pdev *pdev; bool is_end; static unsigned int num_vdev, num_bcn; size_t total_vdevs_started = 0; - int i, ret; - - INIT_LIST_HEAD(&stats.pdevs); - INIT_LIST_HEAD(&stats.vdevs); - INIT_LIST_HEAD(&stats.bcn); - - ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); - if (ret) { - ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); - goto free; - } - - rcu_read_lock(); - ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); - if (!ar) { - rcu_read_unlock(); - ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", - stats.pdev_id, ret); - goto free; - } - - spin_lock_bh(&ar->data_lock); + int i; - if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { - list_splice_tail_init(&stats.pdevs, &ar->debug.fw_stats.pdevs); - ar->debug.fw_stats_done = true; - goto complete; - } + /* WMI_REQUEST_PDEV_STAT request has been already processed */ - if (stats.stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { - ar->debug.fw_stats_done = true; - goto complete; + if (stats->stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { + ar->fw_stats_done = true; + return; } - if (stats.stats_id == WMI_REQUEST_VDEV_STAT) { - if (list_empty(&stats.vdevs)) { + if (stats->stats_id == WMI_REQUEST_VDEV_STAT) { + if (list_empty(&stats->vdevs)) { ath11k_warn(ab, "empty vdev stats"); - goto complete; + return; } /* FW sends all the active VDEV stats irrespective of PDEV, * hence limit until the count of all VDEVs started @@ -190,43 +135,34 @@ void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb is_end = ((++num_vdev) == total_vdevs_started); - list_splice_tail_init(&stats.vdevs, - &ar->debug.fw_stats.vdevs); + list_splice_tail_init(&stats->vdevs, + &ar->fw_stats.vdevs); if (is_end) { - ar->debug.fw_stats_done = true; + ar->fw_stats_done = true; num_vdev = 0; } - goto complete; + return; } - if (stats.stats_id == WMI_REQUEST_BCN_STAT) { - if (list_empty(&stats.bcn)) { + if (stats->stats_id == WMI_REQUEST_BCN_STAT) { + if (list_empty(&stats->bcn)) { ath11k_warn(ab, "empty bcn stats"); - goto complete; + return; } /* Mark end until we reached the count of all started VDEVs * within the PDEV */ is_end = ((++num_bcn) == ar->num_started_vdevs); - list_splice_tail_init(&stats.bcn, - &ar->debug.fw_stats.bcn); + list_splice_tail_init(&stats->bcn, + &ar->fw_stats.bcn); if (is_end) { - ar->debug.fw_stats_done = true; + ar->fw_stats_done = true; num_bcn = 0; } } -complete: - complete(&ar->debug.fw_stats_complete); - rcu_read_unlock(); - spin_unlock_bh(&ar->data_lock); - -free: - ath11k_fw_stats_pdevs_free(&stats.pdevs); - ath11k_fw_stats_vdevs_free(&stats.vdevs); - ath11k_fw_stats_bcn_free(&stats.bcn); } static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, @@ -247,7 +183,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, ath11k_debugfs_fw_stats_reset(ar); - reinit_completion(&ar->debug.fw_stats_complete); + reinit_completion(&ar->fw_stats_complete); ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); @@ -258,7 +194,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, } time_left = - wait_for_completion_timeout(&ar->debug.fw_stats_complete, + wait_for_completion_timeout(&ar->fw_stats_complete, 1 * HZ); if (!time_left) return -ETIMEDOUT; @@ -268,7 +204,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, break; spin_lock_bh(&ar->data_lock); - if (ar->debug.fw_stats_done) { + if (ar->fw_stats_done) { spin_unlock_bh(&ar->data_lock); break; } @@ -340,7 +276,7 @@ static int ath11k_open_pdev_stats(struct inode *inode, struct file *file) goto err_free; } - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, buf); file->private_data = buf; @@ -412,7 +348,7 @@ static int ath11k_open_vdev_stats(struct inode *inode, struct file *file) goto err_free; } - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, buf); file->private_data = buf; @@ -490,14 +426,14 @@ static int ath11k_open_bcn_stats(struct inode *inode, struct file *file) } } - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, buf); /* since beacon stats request is looped for all active VDEVs, saved fw * stats is not freed for each request until done for all active VDEVs */ spin_lock_bh(&ar->data_lock); - ath11k_fw_stats_bcn_free(&ar->debug.fw_stats.bcn); + ath11k_fw_stats_bcn_free(&ar->fw_stats.bcn); spin_unlock_bh(&ar->data_lock); file->private_data = buf; @@ -1055,7 +991,7 @@ void ath11k_debugfs_fw_stats_init(struct ath11k *ar) struct dentry *fwstats_dir = debugfs_create_dir("fw_stats", ar->debug.debugfs_pdev); - ar->debug.fw_stats.debugfs_fwstats = fwstats_dir; + ar->fw_stats.debugfs_fwstats = fwstats_dir; /* all stats debugfs files created are under "fw_stats" directory * created per PDEV @@ -1067,11 +1003,6 @@ void ath11k_debugfs_fw_stats_init(struct ath11k *ar) debugfs_create_file("beacon_stats", 0600, fwstats_dir, ar, &fops_bcn_stats); - INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs); - INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs); - INIT_LIST_HEAD(&ar->debug.fw_stats.bcn); - - init_completion(&ar->debug.fw_stats_complete); } static ssize_t ath11k_write_pktlog_filter(struct file *file, diff --git a/drivers/net/wireless/ath/ath11k/debugfs.h b/drivers/net/wireless/ath/ath11k/debugfs.h index 8fc125a71943..e45dc874ff23 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs.h +++ b/drivers/net/wireless/ath/ath11k/debugfs.h @@ -271,7 +271,7 @@ int ath11k_debugfs_pdev_create(struct ath11k_base *ab); void ath11k_debugfs_pdev_destroy(struct ath11k_base *ab); int ath11k_debugfs_register(struct ath11k *ar); void ath11k_debugfs_unregister(struct ath11k *ar); -void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb); +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats *stats); void ath11k_debugfs_fw_stats_init(struct ath11k *ar); int ath11k_debugfs_get_fw_stats(struct ath11k *ar, u32 pdev_id, @@ -352,8 +352,8 @@ static inline void ath11k_debugfs_unregister(struct ath11k *ar) { } -static inline void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, - struct sk_buff *skb) +static inline void ath11k_debugfs_fw_stats_process(struct ath11k *ar, + struct ath11k_fw_stats *stats) { } diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c index 84d1c7054013..9c7a0a438b12 100644 --- a/drivers/net/wireless/ath/ath11k/wmi.c +++ b/drivers/net/wireless/ath/ath11k/wmi.c @@ -7412,7 +7412,53 @@ static void ath11k_peer_assoc_conf_event(struct ath11k_base *ab, struct sk_buff static void ath11k_update_stats_event(struct ath11k_base *ab, struct sk_buff *skb) { - ath11k_debugfs_fw_stats_process(ab, skb); + struct ath11k_fw_stats stats = {}; + struct ath11k *ar; + int ret; + + INIT_LIST_HEAD(&stats.pdevs); + INIT_LIST_HEAD(&stats.vdevs); + INIT_LIST_HEAD(&stats.bcn); + + ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); + if (ret) { + ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); + goto free; + } + + rcu_read_lock(); + ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); + if (!ar) { + rcu_read_unlock(); + ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", + stats.pdev_id, ret); + goto free; + } + + spin_lock_bh(&ar->data_lock); + + /* WMI_REQUEST_PDEV_STAT can be requested via .get_txpower mac ops or via + * debugfs fw stats. Therefore, processing it separately. + */ + if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { + list_splice_tail_init(&stats.pdevs, &ar->fw_stats.pdevs); + ar->fw_stats_done = true; + goto complete; + } + + /* WMI_REQUEST_VDEV_STAT, WMI_REQUEST_BCN_STAT and WMI_REQUEST_RSSI_PER_CHAIN_STAT + * are currently requested only via debugfs fw stats. Hence, processing these + * in debugfs context + */ + ath11k_debugfs_fw_stats_process(ar, &stats); + +complete: + complete(&ar->fw_stats_complete); + rcu_read_unlock(); + spin_unlock_bh(&ar->data_lock); + +free: + ath11k_fw_stats_free(&stats); } /* PDEV_CTL_FAILSAFE_CHECK_EVENT is received from FW when the frequency scanned -- 2.17.1 -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] ath11k: move firmware stats out of debugfs 2022-06-02 5:14 ` Aditya Kumar Singh @ 2022-06-02 14:33 ` Jeff Johnson -1 siblings, 0 replies; 20+ messages in thread From: Jeff Johnson @ 2022-06-02 14:33 UTC (permalink / raw) To: Aditya Kumar Singh, ath11k; +Cc: linux-wireless On 6/1/2022 10:14 PM, Aditya Kumar Singh wrote: > Currently, firmware stats, comprising pdev, vdev and beacon stats are > part of debugfs. In firmware pdev stats, firmware reports the final > Tx power used to transmit each packet. If driver wants to know the > final Tx power being used at firmware level, it can leverage from > firmware pdev stats. > > Move firmware stats out of debugfs context in order to leverage > the final Tx power reported in it even when debugfs is disabled. > > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > --- > drivers/net/wireless/ath/ath11k/core.c | 46 ++++++++ > drivers/net/wireless/ath/ath11k/core.h | 12 +- > drivers/net/wireless/ath/ath11k/debugfs.c | 131 +++++----------------- > drivers/net/wireless/ath/ath11k/debugfs.h | 6 +- > drivers/net/wireless/ath/ath11k/wmi.c | 48 +++++++- > 5 files changed, 136 insertions(+), 107 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c > index ca6fadd64b43..07a299a2e213 100644 > --- a/drivers/net/wireless/ath/ath11k/core.c > +++ b/drivers/net/wireless/ath/ath11k/core.c > @@ -570,6 +570,52 @@ static inline struct ath11k_pdev *ath11k_core_get_single_pdev(struct ath11k_base > return &ab->pdevs[0]; > } > > +void ath11k_fw_stats_pdevs_free(struct list_head *head) > +{ > + struct ath11k_fw_stats_pdev *i, *tmp; > + > + list_for_each_entry_safe(i, tmp, head, list) { > + list_del(&i->list); > + kfree(i); > + } > +} > + > +void ath11k_fw_stats_vdevs_free(struct list_head *head) > +{ > + struct ath11k_fw_stats_vdev *i, *tmp; > + > + list_for_each_entry_safe(i, tmp, head, list) { > + list_del(&i->list); > + kfree(i); > + } > +} > + > +void ath11k_fw_stats_bcn_free(struct list_head *head) > +{ > + struct ath11k_fw_stats_bcn *i, *tmp; > + > + list_for_each_entry_safe(i, tmp, head, list) { > + list_del(&i->list); > + kfree(i); > + } > +} > + > +void ath11k_fw_stats_init(struct ath11k *ar) > +{ > + INIT_LIST_HEAD(&ar->fw_stats.pdevs); > + INIT_LIST_HEAD(&ar->fw_stats.vdevs); > + INIT_LIST_HEAD(&ar->fw_stats.bcn); > + > + init_completion(&ar->fw_stats_complete); > +} > + > +void ath11k_fw_stats_free(struct ath11k_fw_stats *stats) > +{ > + ath11k_fw_stats_pdevs_free(&stats->pdevs); > + ath11k_fw_stats_vdevs_free(&stats->vdevs); > + ath11k_fw_stats_bcn_free(&stats->bcn); > +} > + > int ath11k_core_suspend(struct ath11k_base *ab) > { > int ret; > diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h > index 65d54e9c15d9..672701f40a6b 100644 > --- a/drivers/net/wireless/ath/ath11k/core.h > +++ b/drivers/net/wireless/ath/ath11k/core.h > @@ -545,9 +545,6 @@ struct ath11k_debug { > struct dentry *debugfs_pdev; > struct ath11k_dbg_htt_stats htt_stats; > u32 extd_tx_stats; > - struct ath11k_fw_stats fw_stats; > - struct completion fw_stats_complete; > - bool fw_stats_done; > u32 extd_rx_stats; > u32 pktlog_filter; > u32 pktlog_mode; > @@ -712,6 +709,9 @@ struct ath11k { > u8 twt_enabled; > bool nlo_enabled; > u8 alpha2[REG_ALPHA2_LEN + 1]; > + struct ath11k_fw_stats fw_stats; > + struct completion fw_stats_complete; > + bool fw_stats_done; > }; > > struct ath11k_band_cap { > @@ -1117,6 +1117,12 @@ struct ath11k_fw_stats_bcn { > u32 tx_bcn_outage_cnt; > }; > > +void ath11k_fw_stats_init(struct ath11k *ar); > +void ath11k_fw_stats_pdevs_free(struct list_head *head); > +void ath11k_fw_stats_vdevs_free(struct list_head *head); > +void ath11k_fw_stats_bcn_free(struct list_head *head); > +void ath11k_fw_stats_free(struct ath11k_fw_stats *stats); > + > extern const struct ce_pipe_config ath11k_target_ce_config_wlan_ipq8074[]; > extern const struct service_to_pipe ath11k_target_service_to_ce_map_wlan_ipq8074[]; > extern const struct service_to_pipe ath11k_target_service_to_ce_map_wlan_ipq6018[]; > diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c > index 0d4843ef9dd1..e4a2fd3da481 100644 > --- a/drivers/net/wireless/ath/ath11k/debugfs.c > +++ b/drivers/net/wireless/ath/ath11k/debugfs.c > @@ -93,91 +93,36 @@ void ath11k_debugfs_add_dbring_entry(struct ath11k *ar, > spin_unlock_bh(&dbr_data->lock); > } > > -static void ath11k_fw_stats_pdevs_free(struct list_head *head) > -{ > - struct ath11k_fw_stats_pdev *i, *tmp; > - > - list_for_each_entry_safe(i, tmp, head, list) { > - list_del(&i->list); > - kfree(i); > - } > -} > - > -static void ath11k_fw_stats_vdevs_free(struct list_head *head) > -{ > - struct ath11k_fw_stats_vdev *i, *tmp; > - > - list_for_each_entry_safe(i, tmp, head, list) { > - list_del(&i->list); > - kfree(i); > - } > -} > - > -static void ath11k_fw_stats_bcn_free(struct list_head *head) > -{ > - struct ath11k_fw_stats_bcn *i, *tmp; > - > - list_for_each_entry_safe(i, tmp, head, list) { > - list_del(&i->list); > - kfree(i); > - } > -} > > static void ath11k_debugfs_fw_stats_reset(struct ath11k *ar) > { > spin_lock_bh(&ar->data_lock); > - ar->debug.fw_stats_done = false; > - ath11k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs); > - ath11k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs); > + ar->fw_stats_done = false; > + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); > + ath11k_fw_stats_vdevs_free(&ar->fw_stats.vdevs); > spin_unlock_bh(&ar->data_lock); > } > > -void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb) > +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats *stats) > { > - struct ath11k_fw_stats stats = {}; > - struct ath11k *ar; > + struct ath11k_base *ab = ar->ab; > struct ath11k_pdev *pdev; > bool is_end; > static unsigned int num_vdev, num_bcn; > size_t total_vdevs_started = 0; > - int i, ret; > - > - INIT_LIST_HEAD(&stats.pdevs); > - INIT_LIST_HEAD(&stats.vdevs); > - INIT_LIST_HEAD(&stats.bcn); > - > - ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); > - if (ret) { > - ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); > - goto free; > - } > - > - rcu_read_lock(); > - ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); > - if (!ar) { > - rcu_read_unlock(); > - ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", > - stats.pdev_id, ret); > - goto free; > - } > - > - spin_lock_bh(&ar->data_lock); > + int i; > > - if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { > - list_splice_tail_init(&stats.pdevs, &ar->debug.fw_stats.pdevs); > - ar->debug.fw_stats_done = true; > - goto complete; > - } > + /* WMI_REQUEST_PDEV_STAT request has been already processed */ > > - if (stats.stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { > - ar->debug.fw_stats_done = true; > - goto complete; > + if (stats->stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { > + ar->fw_stats_done = true; > + return; > } > > - if (stats.stats_id == WMI_REQUEST_VDEV_STAT) { > - if (list_empty(&stats.vdevs)) { > + if (stats->stats_id == WMI_REQUEST_VDEV_STAT) { > + if (list_empty(&stats->vdevs)) { > ath11k_warn(ab, "empty vdev stats"); > - goto complete; > + return; > } > /* FW sends all the active VDEV stats irrespective of PDEV, > * hence limit until the count of all VDEVs started > @@ -190,43 +135,34 @@ void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb > > is_end = ((++num_vdev) == total_vdevs_started); > > - list_splice_tail_init(&stats.vdevs, > - &ar->debug.fw_stats.vdevs); > + list_splice_tail_init(&stats->vdevs, > + &ar->fw_stats.vdevs); > > if (is_end) { > - ar->debug.fw_stats_done = true; > + ar->fw_stats_done = true; > num_vdev = 0; > } > - goto complete; > + return; > } > > - if (stats.stats_id == WMI_REQUEST_BCN_STAT) { > - if (list_empty(&stats.bcn)) { > + if (stats->stats_id == WMI_REQUEST_BCN_STAT) { > + if (list_empty(&stats->bcn)) { > ath11k_warn(ab, "empty bcn stats"); > - goto complete; > + return; > } > /* Mark end until we reached the count of all started VDEVs > * within the PDEV > */ > is_end = ((++num_bcn) == ar->num_started_vdevs); > > - list_splice_tail_init(&stats.bcn, > - &ar->debug.fw_stats.bcn); > + list_splice_tail_init(&stats->bcn, > + &ar->fw_stats.bcn); > > if (is_end) { > - ar->debug.fw_stats_done = true; > + ar->fw_stats_done = true; > num_bcn = 0; > } > } > -complete: > - complete(&ar->debug.fw_stats_complete); > - rcu_read_unlock(); > - spin_unlock_bh(&ar->data_lock); > - > -free: > - ath11k_fw_stats_pdevs_free(&stats.pdevs); > - ath11k_fw_stats_vdevs_free(&stats.vdevs); > - ath11k_fw_stats_bcn_free(&stats.bcn); > } > > static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, > @@ -247,7 +183,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, > > ath11k_debugfs_fw_stats_reset(ar); > > - reinit_completion(&ar->debug.fw_stats_complete); > + reinit_completion(&ar->fw_stats_complete); > > ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); > > @@ -258,7 +194,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, > } > > time_left = > - wait_for_completion_timeout(&ar->debug.fw_stats_complete, > + wait_for_completion_timeout(&ar->fw_stats_complete, this is a case where imo the existing code was not compliant with the coding standard (descendant was not indented from the parent) so I'd definitely want to either indent this or combine it with the prior line > 1 * HZ); > if (!time_left) > return -ETIMEDOUT; > @@ -268,7 +204,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, > break; > > spin_lock_bh(&ar->data_lock); > - if (ar->debug.fw_stats_done) { > + if (ar->fw_stats_done) { > spin_unlock_bh(&ar->data_lock); > break; > } > @@ -340,7 +276,7 @@ static int ath11k_open_pdev_stats(struct inode *inode, struct file *file) > goto err_free; > } > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > buf); there are multiple places in this patch where, due to the fact you are removing 6 characters from an existing line, a descendant may now fit on the prior line without exceeding 80 columns. consider removing the line break in those cases. i wasn't going to mention these bikeshed comments but I see something in the 2nd patch that imo should be addressed, so you'll probably want to upload a v2 and this would be a good cleanup to apply as well > > file->private_data = buf; > @@ -412,7 +348,7 @@ static int ath11k_open_vdev_stats(struct inode *inode, struct file *file) > goto err_free; > } > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > buf); > > file->private_data = buf; > @@ -490,14 +426,14 @@ static int ath11k_open_bcn_stats(struct inode *inode, struct file *file) > } > } > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > buf); > > /* since beacon stats request is looped for all active VDEVs, saved fw > * stats is not freed for each request until done for all active VDEVs > */ > spin_lock_bh(&ar->data_lock); > - ath11k_fw_stats_bcn_free(&ar->debug.fw_stats.bcn); > + ath11k_fw_stats_bcn_free(&ar->fw_stats.bcn); > spin_unlock_bh(&ar->data_lock); > > file->private_data = buf; > @@ -1055,7 +991,7 @@ void ath11k_debugfs_fw_stats_init(struct ath11k *ar) > struct dentry *fwstats_dir = debugfs_create_dir("fw_stats", > ar->debug.debugfs_pdev); > > - ar->debug.fw_stats.debugfs_fwstats = fwstats_dir; > + ar->fw_stats.debugfs_fwstats = fwstats_dir; > > /* all stats debugfs files created are under "fw_stats" directory > * created per PDEV > @@ -1067,11 +1003,6 @@ void ath11k_debugfs_fw_stats_init(struct ath11k *ar) > debugfs_create_file("beacon_stats", 0600, fwstats_dir, ar, > &fops_bcn_stats); > > - INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs); > - INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs); > - INIT_LIST_HEAD(&ar->debug.fw_stats.bcn); > - > - init_completion(&ar->debug.fw_stats_complete); > } > > static ssize_t ath11k_write_pktlog_filter(struct file *file, > diff --git a/drivers/net/wireless/ath/ath11k/debugfs.h b/drivers/net/wireless/ath/ath11k/debugfs.h > index 8fc125a71943..e45dc874ff23 100644 > --- a/drivers/net/wireless/ath/ath11k/debugfs.h > +++ b/drivers/net/wireless/ath/ath11k/debugfs.h > @@ -271,7 +271,7 @@ int ath11k_debugfs_pdev_create(struct ath11k_base *ab); > void ath11k_debugfs_pdev_destroy(struct ath11k_base *ab); > int ath11k_debugfs_register(struct ath11k *ar); > void ath11k_debugfs_unregister(struct ath11k *ar); > -void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb); > +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats *stats); > > void ath11k_debugfs_fw_stats_init(struct ath11k *ar); > int ath11k_debugfs_get_fw_stats(struct ath11k *ar, u32 pdev_id, > @@ -352,8 +352,8 @@ static inline void ath11k_debugfs_unregister(struct ath11k *ar) > { > } > > -static inline void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, > - struct sk_buff *skb) > +static inline void ath11k_debugfs_fw_stats_process(struct ath11k *ar, > + struct ath11k_fw_stats *stats) > { > } > > diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c > index 84d1c7054013..9c7a0a438b12 100644 > --- a/drivers/net/wireless/ath/ath11k/wmi.c > +++ b/drivers/net/wireless/ath/ath11k/wmi.c > @@ -7412,7 +7412,53 @@ static void ath11k_peer_assoc_conf_event(struct ath11k_base *ab, struct sk_buff > > static void ath11k_update_stats_event(struct ath11k_base *ab, struct sk_buff *skb) > { > - ath11k_debugfs_fw_stats_process(ab, skb); > + struct ath11k_fw_stats stats = {}; > + struct ath11k *ar; > + int ret; > + > + INIT_LIST_HEAD(&stats.pdevs); > + INIT_LIST_HEAD(&stats.vdevs); > + INIT_LIST_HEAD(&stats.bcn); > + > + ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); > + if (ret) { > + ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); > + goto free; > + } > + > + rcu_read_lock(); > + ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); > + if (!ar) { > + rcu_read_unlock(); > + ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", > + stats.pdev_id, ret); > + goto free; > + } > + > + spin_lock_bh(&ar->data_lock); > + > + /* WMI_REQUEST_PDEV_STAT can be requested via .get_txpower mac ops or via > + * debugfs fw stats. Therefore, processing it separately. > + */ > + if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { > + list_splice_tail_init(&stats.pdevs, &ar->fw_stats.pdevs); > + ar->fw_stats_done = true; > + goto complete; > + } > + > + /* WMI_REQUEST_VDEV_STAT, WMI_REQUEST_BCN_STAT and WMI_REQUEST_RSSI_PER_CHAIN_STAT > + * are currently requested only via debugfs fw stats. Hence, processing these > + * in debugfs context > + */ > + ath11k_debugfs_fw_stats_process(ar, &stats); > + > +complete: > + complete(&ar->fw_stats_complete); > + rcu_read_unlock(); > + spin_unlock_bh(&ar->data_lock); > + > +free: > + ath11k_fw_stats_free(&stats); > } > > /* PDEV_CTL_FAILSAFE_CHECK_EVENT is received from FW when the frequency scanned ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] ath11k: move firmware stats out of debugfs @ 2022-06-02 14:33 ` Jeff Johnson 0 siblings, 0 replies; 20+ messages in thread From: Jeff Johnson @ 2022-06-02 14:33 UTC (permalink / raw) To: Aditya Kumar Singh, ath11k; +Cc: linux-wireless On 6/1/2022 10:14 PM, Aditya Kumar Singh wrote: > Currently, firmware stats, comprising pdev, vdev and beacon stats are > part of debugfs. In firmware pdev stats, firmware reports the final > Tx power used to transmit each packet. If driver wants to know the > final Tx power being used at firmware level, it can leverage from > firmware pdev stats. > > Move firmware stats out of debugfs context in order to leverage > the final Tx power reported in it even when debugfs is disabled. > > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > --- > drivers/net/wireless/ath/ath11k/core.c | 46 ++++++++ > drivers/net/wireless/ath/ath11k/core.h | 12 +- > drivers/net/wireless/ath/ath11k/debugfs.c | 131 +++++----------------- > drivers/net/wireless/ath/ath11k/debugfs.h | 6 +- > drivers/net/wireless/ath/ath11k/wmi.c | 48 +++++++- > 5 files changed, 136 insertions(+), 107 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c > index ca6fadd64b43..07a299a2e213 100644 > --- a/drivers/net/wireless/ath/ath11k/core.c > +++ b/drivers/net/wireless/ath/ath11k/core.c > @@ -570,6 +570,52 @@ static inline struct ath11k_pdev *ath11k_core_get_single_pdev(struct ath11k_base > return &ab->pdevs[0]; > } > > +void ath11k_fw_stats_pdevs_free(struct list_head *head) > +{ > + struct ath11k_fw_stats_pdev *i, *tmp; > + > + list_for_each_entry_safe(i, tmp, head, list) { > + list_del(&i->list); > + kfree(i); > + } > +} > + > +void ath11k_fw_stats_vdevs_free(struct list_head *head) > +{ > + struct ath11k_fw_stats_vdev *i, *tmp; > + > + list_for_each_entry_safe(i, tmp, head, list) { > + list_del(&i->list); > + kfree(i); > + } > +} > + > +void ath11k_fw_stats_bcn_free(struct list_head *head) > +{ > + struct ath11k_fw_stats_bcn *i, *tmp; > + > + list_for_each_entry_safe(i, tmp, head, list) { > + list_del(&i->list); > + kfree(i); > + } > +} > + > +void ath11k_fw_stats_init(struct ath11k *ar) > +{ > + INIT_LIST_HEAD(&ar->fw_stats.pdevs); > + INIT_LIST_HEAD(&ar->fw_stats.vdevs); > + INIT_LIST_HEAD(&ar->fw_stats.bcn); > + > + init_completion(&ar->fw_stats_complete); > +} > + > +void ath11k_fw_stats_free(struct ath11k_fw_stats *stats) > +{ > + ath11k_fw_stats_pdevs_free(&stats->pdevs); > + ath11k_fw_stats_vdevs_free(&stats->vdevs); > + ath11k_fw_stats_bcn_free(&stats->bcn); > +} > + > int ath11k_core_suspend(struct ath11k_base *ab) > { > int ret; > diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h > index 65d54e9c15d9..672701f40a6b 100644 > --- a/drivers/net/wireless/ath/ath11k/core.h > +++ b/drivers/net/wireless/ath/ath11k/core.h > @@ -545,9 +545,6 @@ struct ath11k_debug { > struct dentry *debugfs_pdev; > struct ath11k_dbg_htt_stats htt_stats; > u32 extd_tx_stats; > - struct ath11k_fw_stats fw_stats; > - struct completion fw_stats_complete; > - bool fw_stats_done; > u32 extd_rx_stats; > u32 pktlog_filter; > u32 pktlog_mode; > @@ -712,6 +709,9 @@ struct ath11k { > u8 twt_enabled; > bool nlo_enabled; > u8 alpha2[REG_ALPHA2_LEN + 1]; > + struct ath11k_fw_stats fw_stats; > + struct completion fw_stats_complete; > + bool fw_stats_done; > }; > > struct ath11k_band_cap { > @@ -1117,6 +1117,12 @@ struct ath11k_fw_stats_bcn { > u32 tx_bcn_outage_cnt; > }; > > +void ath11k_fw_stats_init(struct ath11k *ar); > +void ath11k_fw_stats_pdevs_free(struct list_head *head); > +void ath11k_fw_stats_vdevs_free(struct list_head *head); > +void ath11k_fw_stats_bcn_free(struct list_head *head); > +void ath11k_fw_stats_free(struct ath11k_fw_stats *stats); > + > extern const struct ce_pipe_config ath11k_target_ce_config_wlan_ipq8074[]; > extern const struct service_to_pipe ath11k_target_service_to_ce_map_wlan_ipq8074[]; > extern const struct service_to_pipe ath11k_target_service_to_ce_map_wlan_ipq6018[]; > diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c > index 0d4843ef9dd1..e4a2fd3da481 100644 > --- a/drivers/net/wireless/ath/ath11k/debugfs.c > +++ b/drivers/net/wireless/ath/ath11k/debugfs.c > @@ -93,91 +93,36 @@ void ath11k_debugfs_add_dbring_entry(struct ath11k *ar, > spin_unlock_bh(&dbr_data->lock); > } > > -static void ath11k_fw_stats_pdevs_free(struct list_head *head) > -{ > - struct ath11k_fw_stats_pdev *i, *tmp; > - > - list_for_each_entry_safe(i, tmp, head, list) { > - list_del(&i->list); > - kfree(i); > - } > -} > - > -static void ath11k_fw_stats_vdevs_free(struct list_head *head) > -{ > - struct ath11k_fw_stats_vdev *i, *tmp; > - > - list_for_each_entry_safe(i, tmp, head, list) { > - list_del(&i->list); > - kfree(i); > - } > -} > - > -static void ath11k_fw_stats_bcn_free(struct list_head *head) > -{ > - struct ath11k_fw_stats_bcn *i, *tmp; > - > - list_for_each_entry_safe(i, tmp, head, list) { > - list_del(&i->list); > - kfree(i); > - } > -} > > static void ath11k_debugfs_fw_stats_reset(struct ath11k *ar) > { > spin_lock_bh(&ar->data_lock); > - ar->debug.fw_stats_done = false; > - ath11k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs); > - ath11k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs); > + ar->fw_stats_done = false; > + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); > + ath11k_fw_stats_vdevs_free(&ar->fw_stats.vdevs); > spin_unlock_bh(&ar->data_lock); > } > > -void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb) > +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats *stats) > { > - struct ath11k_fw_stats stats = {}; > - struct ath11k *ar; > + struct ath11k_base *ab = ar->ab; > struct ath11k_pdev *pdev; > bool is_end; > static unsigned int num_vdev, num_bcn; > size_t total_vdevs_started = 0; > - int i, ret; > - > - INIT_LIST_HEAD(&stats.pdevs); > - INIT_LIST_HEAD(&stats.vdevs); > - INIT_LIST_HEAD(&stats.bcn); > - > - ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); > - if (ret) { > - ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); > - goto free; > - } > - > - rcu_read_lock(); > - ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); > - if (!ar) { > - rcu_read_unlock(); > - ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", > - stats.pdev_id, ret); > - goto free; > - } > - > - spin_lock_bh(&ar->data_lock); > + int i; > > - if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { > - list_splice_tail_init(&stats.pdevs, &ar->debug.fw_stats.pdevs); > - ar->debug.fw_stats_done = true; > - goto complete; > - } > + /* WMI_REQUEST_PDEV_STAT request has been already processed */ > > - if (stats.stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { > - ar->debug.fw_stats_done = true; > - goto complete; > + if (stats->stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { > + ar->fw_stats_done = true; > + return; > } > > - if (stats.stats_id == WMI_REQUEST_VDEV_STAT) { > - if (list_empty(&stats.vdevs)) { > + if (stats->stats_id == WMI_REQUEST_VDEV_STAT) { > + if (list_empty(&stats->vdevs)) { > ath11k_warn(ab, "empty vdev stats"); > - goto complete; > + return; > } > /* FW sends all the active VDEV stats irrespective of PDEV, > * hence limit until the count of all VDEVs started > @@ -190,43 +135,34 @@ void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb > > is_end = ((++num_vdev) == total_vdevs_started); > > - list_splice_tail_init(&stats.vdevs, > - &ar->debug.fw_stats.vdevs); > + list_splice_tail_init(&stats->vdevs, > + &ar->fw_stats.vdevs); > > if (is_end) { > - ar->debug.fw_stats_done = true; > + ar->fw_stats_done = true; > num_vdev = 0; > } > - goto complete; > + return; > } > > - if (stats.stats_id == WMI_REQUEST_BCN_STAT) { > - if (list_empty(&stats.bcn)) { > + if (stats->stats_id == WMI_REQUEST_BCN_STAT) { > + if (list_empty(&stats->bcn)) { > ath11k_warn(ab, "empty bcn stats"); > - goto complete; > + return; > } > /* Mark end until we reached the count of all started VDEVs > * within the PDEV > */ > is_end = ((++num_bcn) == ar->num_started_vdevs); > > - list_splice_tail_init(&stats.bcn, > - &ar->debug.fw_stats.bcn); > + list_splice_tail_init(&stats->bcn, > + &ar->fw_stats.bcn); > > if (is_end) { > - ar->debug.fw_stats_done = true; > + ar->fw_stats_done = true; > num_bcn = 0; > } > } > -complete: > - complete(&ar->debug.fw_stats_complete); > - rcu_read_unlock(); > - spin_unlock_bh(&ar->data_lock); > - > -free: > - ath11k_fw_stats_pdevs_free(&stats.pdevs); > - ath11k_fw_stats_vdevs_free(&stats.vdevs); > - ath11k_fw_stats_bcn_free(&stats.bcn); > } > > static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, > @@ -247,7 +183,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, > > ath11k_debugfs_fw_stats_reset(ar); > > - reinit_completion(&ar->debug.fw_stats_complete); > + reinit_completion(&ar->fw_stats_complete); > > ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); > > @@ -258,7 +194,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, > } > > time_left = > - wait_for_completion_timeout(&ar->debug.fw_stats_complete, > + wait_for_completion_timeout(&ar->fw_stats_complete, this is a case where imo the existing code was not compliant with the coding standard (descendant was not indented from the parent) so I'd definitely want to either indent this or combine it with the prior line > 1 * HZ); > if (!time_left) > return -ETIMEDOUT; > @@ -268,7 +204,7 @@ static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, > break; > > spin_lock_bh(&ar->data_lock); > - if (ar->debug.fw_stats_done) { > + if (ar->fw_stats_done) { > spin_unlock_bh(&ar->data_lock); > break; > } > @@ -340,7 +276,7 @@ static int ath11k_open_pdev_stats(struct inode *inode, struct file *file) > goto err_free; > } > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > buf); there are multiple places in this patch where, due to the fact you are removing 6 characters from an existing line, a descendant may now fit on the prior line without exceeding 80 columns. consider removing the line break in those cases. i wasn't going to mention these bikeshed comments but I see something in the 2nd patch that imo should be addressed, so you'll probably want to upload a v2 and this would be a good cleanup to apply as well > > file->private_data = buf; > @@ -412,7 +348,7 @@ static int ath11k_open_vdev_stats(struct inode *inode, struct file *file) > goto err_free; > } > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > buf); > > file->private_data = buf; > @@ -490,14 +426,14 @@ static int ath11k_open_bcn_stats(struct inode *inode, struct file *file) > } > } > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, req_param.stats_id, > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > buf); > > /* since beacon stats request is looped for all active VDEVs, saved fw > * stats is not freed for each request until done for all active VDEVs > */ > spin_lock_bh(&ar->data_lock); > - ath11k_fw_stats_bcn_free(&ar->debug.fw_stats.bcn); > + ath11k_fw_stats_bcn_free(&ar->fw_stats.bcn); > spin_unlock_bh(&ar->data_lock); > > file->private_data = buf; > @@ -1055,7 +991,7 @@ void ath11k_debugfs_fw_stats_init(struct ath11k *ar) > struct dentry *fwstats_dir = debugfs_create_dir("fw_stats", > ar->debug.debugfs_pdev); > > - ar->debug.fw_stats.debugfs_fwstats = fwstats_dir; > + ar->fw_stats.debugfs_fwstats = fwstats_dir; > > /* all stats debugfs files created are under "fw_stats" directory > * created per PDEV > @@ -1067,11 +1003,6 @@ void ath11k_debugfs_fw_stats_init(struct ath11k *ar) > debugfs_create_file("beacon_stats", 0600, fwstats_dir, ar, > &fops_bcn_stats); > > - INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs); > - INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs); > - INIT_LIST_HEAD(&ar->debug.fw_stats.bcn); > - > - init_completion(&ar->debug.fw_stats_complete); > } > > static ssize_t ath11k_write_pktlog_filter(struct file *file, > diff --git a/drivers/net/wireless/ath/ath11k/debugfs.h b/drivers/net/wireless/ath/ath11k/debugfs.h > index 8fc125a71943..e45dc874ff23 100644 > --- a/drivers/net/wireless/ath/ath11k/debugfs.h > +++ b/drivers/net/wireless/ath/ath11k/debugfs.h > @@ -271,7 +271,7 @@ int ath11k_debugfs_pdev_create(struct ath11k_base *ab); > void ath11k_debugfs_pdev_destroy(struct ath11k_base *ab); > int ath11k_debugfs_register(struct ath11k *ar); > void ath11k_debugfs_unregister(struct ath11k *ar); > -void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff *skb); > +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct ath11k_fw_stats *stats); > > void ath11k_debugfs_fw_stats_init(struct ath11k *ar); > int ath11k_debugfs_get_fw_stats(struct ath11k *ar, u32 pdev_id, > @@ -352,8 +352,8 @@ static inline void ath11k_debugfs_unregister(struct ath11k *ar) > { > } > > -static inline void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, > - struct sk_buff *skb) > +static inline void ath11k_debugfs_fw_stats_process(struct ath11k *ar, > + struct ath11k_fw_stats *stats) > { > } > > diff --git a/drivers/net/wireless/ath/ath11k/wmi.c b/drivers/net/wireless/ath/ath11k/wmi.c > index 84d1c7054013..9c7a0a438b12 100644 > --- a/drivers/net/wireless/ath/ath11k/wmi.c > +++ b/drivers/net/wireless/ath/ath11k/wmi.c > @@ -7412,7 +7412,53 @@ static void ath11k_peer_assoc_conf_event(struct ath11k_base *ab, struct sk_buff > > static void ath11k_update_stats_event(struct ath11k_base *ab, struct sk_buff *skb) > { > - ath11k_debugfs_fw_stats_process(ab, skb); > + struct ath11k_fw_stats stats = {}; > + struct ath11k *ar; > + int ret; > + > + INIT_LIST_HEAD(&stats.pdevs); > + INIT_LIST_HEAD(&stats.vdevs); > + INIT_LIST_HEAD(&stats.bcn); > + > + ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); > + if (ret) { > + ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); > + goto free; > + } > + > + rcu_read_lock(); > + ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); > + if (!ar) { > + rcu_read_unlock(); > + ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", > + stats.pdev_id, ret); > + goto free; > + } > + > + spin_lock_bh(&ar->data_lock); > + > + /* WMI_REQUEST_PDEV_STAT can be requested via .get_txpower mac ops or via > + * debugfs fw stats. Therefore, processing it separately. > + */ > + if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { > + list_splice_tail_init(&stats.pdevs, &ar->fw_stats.pdevs); > + ar->fw_stats_done = true; > + goto complete; > + } > + > + /* WMI_REQUEST_VDEV_STAT, WMI_REQUEST_BCN_STAT and WMI_REQUEST_RSSI_PER_CHAIN_STAT > + * are currently requested only via debugfs fw stats. Hence, processing these > + * in debugfs context > + */ > + ath11k_debugfs_fw_stats_process(ar, &stats); > + > +complete: > + complete(&ar->fw_stats_complete); > + rcu_read_unlock(); > + spin_unlock_bh(&ar->data_lock); > + > +free: > + ath11k_fw_stats_free(&stats); > } > > /* PDEV_CTL_FAILSAFE_CHECK_EVENT is received from FW when the frequency scanned -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/2] ath11k: move firmware stats out of debugfs 2022-06-02 14:33 ` Jeff Johnson @ 2022-06-03 4:23 ` Aditya Kumar Singh (QUIC) -1 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh (QUIC) @ 2022-06-03 4:23 UTC (permalink / raw) To: Jeff Johnson (QUIC); +Cc: linux-wireless, Aditya Kumar Singh (QUIC), ath11k > -----Original Message----- > From: Jeff Johnson (QUIC) <quic_jjohnson@quicinc.com> > Sent: Thursday, June 2, 2022 20:03 > To: Aditya Kumar Singh (QUIC) <quic_adisi@quicinc.com>; > ath11k@lists.infradead.org > Cc: linux-wireless@vger.kernel.org > Subject: Re: [PATCH 1/2] ath11k: move firmware stats out of debugfs > > On 6/1/2022 10:14 PM, Aditya Kumar Singh wrote: > > Currently, firmware stats, comprising pdev, vdev and beacon stats are > > part of debugfs. In firmware pdev stats, firmware reports the final Tx > > power used to transmit each packet. If driver wants to know the final > > Tx power being used at firmware level, it can leverage from firmware > > pdev stats. > > > > Move firmware stats out of debugfs context in order to leverage the > > final Tx power reported in it even when debugfs is disabled. > > > > Tested-on: IPQ8074 hw2.0 AHB > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: QCN9074 hw1.0 PCI > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: WCN6855 hw2.0 PCI > > WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > > --- > > drivers/net/wireless/ath/ath11k/core.c | 46 ++++++++ > > drivers/net/wireless/ath/ath11k/core.h | 12 +- > > drivers/net/wireless/ath/ath11k/debugfs.c | 131 +++++----------------- > > drivers/net/wireless/ath/ath11k/debugfs.h | 6 +- > > drivers/net/wireless/ath/ath11k/wmi.c | 48 +++++++- > > 5 files changed, 136 insertions(+), 107 deletions(-) > > > > diff --git a/drivers/net/wireless/ath/ath11k/core.c > > b/drivers/net/wireless/ath/ath11k/core.c > > index ca6fadd64b43..07a299a2e213 100644 > > --- a/drivers/net/wireless/ath/ath11k/core.c > > +++ b/drivers/net/wireless/ath/ath11k/core.c > > @@ -570,6 +570,52 @@ static inline struct ath11k_pdev > *ath11k_core_get_single_pdev(struct ath11k_base > > return &ab->pdevs[0]; > > } > > > > +void ath11k_fw_stats_pdevs_free(struct list_head *head) { > > + struct ath11k_fw_stats_pdev *i, *tmp; > > + > > + list_for_each_entry_safe(i, tmp, head, list) { > > + list_del(&i->list); > > + kfree(i); > > + } > > +} > > + > > +void ath11k_fw_stats_vdevs_free(struct list_head *head) { > > + struct ath11k_fw_stats_vdev *i, *tmp; > > + > > + list_for_each_entry_safe(i, tmp, head, list) { > > + list_del(&i->list); > > + kfree(i); > > + } > > +} > > + > > +void ath11k_fw_stats_bcn_free(struct list_head *head) { > > + struct ath11k_fw_stats_bcn *i, *tmp; > > + > > + list_for_each_entry_safe(i, tmp, head, list) { > > + list_del(&i->list); > > + kfree(i); > > + } > > +} > > + > > +void ath11k_fw_stats_init(struct ath11k *ar) { > > + INIT_LIST_HEAD(&ar->fw_stats.pdevs); > > + INIT_LIST_HEAD(&ar->fw_stats.vdevs); > > + INIT_LIST_HEAD(&ar->fw_stats.bcn); > > + > > + init_completion(&ar->fw_stats_complete); > > +} > > + > > +void ath11k_fw_stats_free(struct ath11k_fw_stats *stats) { > > + ath11k_fw_stats_pdevs_free(&stats->pdevs); > > + ath11k_fw_stats_vdevs_free(&stats->vdevs); > > + ath11k_fw_stats_bcn_free(&stats->bcn); > > +} > > + > > int ath11k_core_suspend(struct ath11k_base *ab) > > { > > int ret; > > diff --git a/drivers/net/wireless/ath/ath11k/core.h > > b/drivers/net/wireless/ath/ath11k/core.h > > index 65d54e9c15d9..672701f40a6b 100644 > > --- a/drivers/net/wireless/ath/ath11k/core.h > > +++ b/drivers/net/wireless/ath/ath11k/core.h > > @@ -545,9 +545,6 @@ struct ath11k_debug { > > struct dentry *debugfs_pdev; > > struct ath11k_dbg_htt_stats htt_stats; > > u32 extd_tx_stats; > > - struct ath11k_fw_stats fw_stats; > > - struct completion fw_stats_complete; > > - bool fw_stats_done; > > u32 extd_rx_stats; > > u32 pktlog_filter; > > u32 pktlog_mode; > > @@ -712,6 +709,9 @@ struct ath11k { > > u8 twt_enabled; > > bool nlo_enabled; > > u8 alpha2[REG_ALPHA2_LEN + 1]; > > + struct ath11k_fw_stats fw_stats; > > + struct completion fw_stats_complete; > > + bool fw_stats_done; > > }; > > > > struct ath11k_band_cap { > > @@ -1117,6 +1117,12 @@ struct ath11k_fw_stats_bcn { > > u32 tx_bcn_outage_cnt; > > }; > > > > +void ath11k_fw_stats_init(struct ath11k *ar); void > > +ath11k_fw_stats_pdevs_free(struct list_head *head); void > > +ath11k_fw_stats_vdevs_free(struct list_head *head); void > > +ath11k_fw_stats_bcn_free(struct list_head *head); void > > +ath11k_fw_stats_free(struct ath11k_fw_stats *stats); > > + > > extern const struct ce_pipe_config > ath11k_target_ce_config_wlan_ipq8074[]; > > extern const struct service_to_pipe > ath11k_target_service_to_ce_map_wlan_ipq8074[]; > > extern const struct service_to_pipe > > ath11k_target_service_to_ce_map_wlan_ipq6018[]; > > diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c > > b/drivers/net/wireless/ath/ath11k/debugfs.c > > index 0d4843ef9dd1..e4a2fd3da481 100644 > > --- a/drivers/net/wireless/ath/ath11k/debugfs.c > > +++ b/drivers/net/wireless/ath/ath11k/debugfs.c > > @@ -93,91 +93,36 @@ void ath11k_debugfs_add_dbring_entry(struct > ath11k *ar, > > spin_unlock_bh(&dbr_data->lock); > > } > > > > -static void ath11k_fw_stats_pdevs_free(struct list_head *head) -{ > > - struct ath11k_fw_stats_pdev *i, *tmp; > > - > > - list_for_each_entry_safe(i, tmp, head, list) { > > - list_del(&i->list); > > - kfree(i); > > - } > > -} > > - > > -static void ath11k_fw_stats_vdevs_free(struct list_head *head) -{ > > - struct ath11k_fw_stats_vdev *i, *tmp; > > - > > - list_for_each_entry_safe(i, tmp, head, list) { > > - list_del(&i->list); > > - kfree(i); > > - } > > -} > > - > > -static void ath11k_fw_stats_bcn_free(struct list_head *head) -{ > > - struct ath11k_fw_stats_bcn *i, *tmp; > > - > > - list_for_each_entry_safe(i, tmp, head, list) { > > - list_del(&i->list); > > - kfree(i); > > - } > > -} > > > > static void ath11k_debugfs_fw_stats_reset(struct ath11k *ar) > > { > > spin_lock_bh(&ar->data_lock); > > - ar->debug.fw_stats_done = false; > > - ath11k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs); > > - ath11k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs); > > + ar->fw_stats_done = false; > > + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); > > + ath11k_fw_stats_vdevs_free(&ar->fw_stats.vdevs); > > spin_unlock_bh(&ar->data_lock); > > } > > > > -void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct > > sk_buff *skb) > > +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct > > +ath11k_fw_stats *stats) > > { > > - struct ath11k_fw_stats stats = {}; > > - struct ath11k *ar; > > + struct ath11k_base *ab = ar->ab; > > struct ath11k_pdev *pdev; > > bool is_end; > > static unsigned int num_vdev, num_bcn; > > size_t total_vdevs_started = 0; > > - int i, ret; > > - > > - INIT_LIST_HEAD(&stats.pdevs); > > - INIT_LIST_HEAD(&stats.vdevs); > > - INIT_LIST_HEAD(&stats.bcn); > > - > > - ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); > > - if (ret) { > > - ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); > > - goto free; > > - } > > - > > - rcu_read_lock(); > > - ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); > > - if (!ar) { > > - rcu_read_unlock(); > > - ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", > > - stats.pdev_id, ret); > > - goto free; > > - } > > - > > - spin_lock_bh(&ar->data_lock); > > + int i; > > > > - if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { > > - list_splice_tail_init(&stats.pdevs, &ar- > >debug.fw_stats.pdevs); > > - ar->debug.fw_stats_done = true; > > - goto complete; > > - } > > + /* WMI_REQUEST_PDEV_STAT request has been already processed > */ > > > > - if (stats.stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { > > - ar->debug.fw_stats_done = true; > > - goto complete; > > + if (stats->stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { > > + ar->fw_stats_done = true; > > + return; > > } > > > > - if (stats.stats_id == WMI_REQUEST_VDEV_STAT) { > > - if (list_empty(&stats.vdevs)) { > > + if (stats->stats_id == WMI_REQUEST_VDEV_STAT) { > > + if (list_empty(&stats->vdevs)) { > > ath11k_warn(ab, "empty vdev stats"); > > - goto complete; > > + return; > > } > > /* FW sends all the active VDEV stats irrespective of PDEV, > > * hence limit until the count of all VDEVs started @@ -190,43 > > +135,34 @@ void ath11k_debugfs_fw_stats_process(struct ath11k_base > > *ab, struct sk_buff *skb > > > > is_end = ((++num_vdev) == total_vdevs_started); > > > > - list_splice_tail_init(&stats.vdevs, > > - &ar->debug.fw_stats.vdevs); > > + list_splice_tail_init(&stats->vdevs, > > + &ar->fw_stats.vdevs); > > > > if (is_end) { > > - ar->debug.fw_stats_done = true; > > + ar->fw_stats_done = true; > > num_vdev = 0; > > } > > - goto complete; > > + return; > > } > > > > - if (stats.stats_id == WMI_REQUEST_BCN_STAT) { > > - if (list_empty(&stats.bcn)) { > > + if (stats->stats_id == WMI_REQUEST_BCN_STAT) { > > + if (list_empty(&stats->bcn)) { > > ath11k_warn(ab, "empty bcn stats"); > > - goto complete; > > + return; > > } > > /* Mark end until we reached the count of all started VDEVs > > * within the PDEV > > */ > > is_end = ((++num_bcn) == ar->num_started_vdevs); > > > > - list_splice_tail_init(&stats.bcn, > > - &ar->debug.fw_stats.bcn); > > + list_splice_tail_init(&stats->bcn, > > + &ar->fw_stats.bcn); > > > > if (is_end) { > > - ar->debug.fw_stats_done = true; > > + ar->fw_stats_done = true; > > num_bcn = 0; > > } > > } > > -complete: > > - complete(&ar->debug.fw_stats_complete); > > - rcu_read_unlock(); > > - spin_unlock_bh(&ar->data_lock); > > - > > -free: > > - ath11k_fw_stats_pdevs_free(&stats.pdevs); > > - ath11k_fw_stats_vdevs_free(&stats.vdevs); > > - ath11k_fw_stats_bcn_free(&stats.bcn); > > } > > > > static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, @@ > > -247,7 +183,7 @@ static int ath11k_debugfs_fw_stats_request(struct > > ath11k *ar, > > > > ath11k_debugfs_fw_stats_reset(ar); > > > > - reinit_completion(&ar->debug.fw_stats_complete); > > + reinit_completion(&ar->fw_stats_complete); > > > > ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); > > > > @@ -258,7 +194,7 @@ static int ath11k_debugfs_fw_stats_request(struct > ath11k *ar, > > } > > > > time_left = > > - wait_for_completion_timeout(&ar->debug.fw_stats_complete, > > + wait_for_completion_timeout(&ar->fw_stats_complete, > > this is a case where imo the existing code was not compliant with the coding > standard (descendant was not indented from the parent) so I'd definitely > want to either indent this or combine it with the prior line > Sure, will address in v2. Thanks for pointing out. > > 1 * HZ); > > if (!time_left) > > return -ETIMEDOUT; > > @@ -268,7 +204,7 @@ static int ath11k_debugfs_fw_stats_request(struct > ath11k *ar, > > break; > > > > spin_lock_bh(&ar->data_lock); > > - if (ar->debug.fw_stats_done) { > > + if (ar->fw_stats_done) { > > spin_unlock_bh(&ar->data_lock); > > break; > > } > > @@ -340,7 +276,7 @@ static int ath11k_open_pdev_stats(struct inode > *inode, struct file *file) > > goto err_free; > > } > > > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, > req_param.stats_id, > > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > > buf); > > there are multiple places in this patch where, due to the fact you are > removing 6 characters from an existing line, a descendant may now fit on the > prior line without exceeding 80 columns. consider removing the line break in > those cases. > > i wasn't going to mention these bikeshed comments but I see something in > the 2nd patch that imo should be addressed, so you'll probably want to > upload a v2 and this would be a good cleanup to apply as well > Yes correct. Will address these v2. Thanks. > > > > file->private_data = buf; > > @@ -412,7 +348,7 @@ static int ath11k_open_vdev_stats(struct inode > *inode, struct file *file) > > goto err_free; > > } > > > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, > req_param.stats_id, > > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > > buf); > > > > file->private_data = buf; > > @@ -490,14 +426,14 @@ static int ath11k_open_bcn_stats(struct inode > *inode, struct file *file) > > } > > } > > > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, > req_param.stats_id, > > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > > buf); > > > > /* since beacon stats request is looped for all active VDEVs, saved fw > > * stats is not freed for each request until done for all active VDEVs > > */ > > spin_lock_bh(&ar->data_lock); > > - ath11k_fw_stats_bcn_free(&ar->debug.fw_stats.bcn); > > + ath11k_fw_stats_bcn_free(&ar->fw_stats.bcn); > > spin_unlock_bh(&ar->data_lock); > > > > file->private_data = buf; > > @@ -1055,7 +991,7 @@ void ath11k_debugfs_fw_stats_init(struct ath11k > *ar) > > struct dentry *fwstats_dir = debugfs_create_dir("fw_stats", > > ar- > >debug.debugfs_pdev); > > > > - ar->debug.fw_stats.debugfs_fwstats = fwstats_dir; > > + ar->fw_stats.debugfs_fwstats = fwstats_dir; > > > > /* all stats debugfs files created are under "fw_stats" directory > > * created per PDEV > > @@ -1067,11 +1003,6 @@ void ath11k_debugfs_fw_stats_init(struct > ath11k *ar) > > debugfs_create_file("beacon_stats", 0600, fwstats_dir, ar, > > &fops_bcn_stats); > > > > - INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs); > > - INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs); > > - INIT_LIST_HEAD(&ar->debug.fw_stats.bcn); > > - > > - init_completion(&ar->debug.fw_stats_complete); > > } > > > > static ssize_t ath11k_write_pktlog_filter(struct file *file, diff > > --git a/drivers/net/wireless/ath/ath11k/debugfs.h > > b/drivers/net/wireless/ath/ath11k/debugfs.h > > index 8fc125a71943..e45dc874ff23 100644 > > --- a/drivers/net/wireless/ath/ath11k/debugfs.h > > +++ b/drivers/net/wireless/ath/ath11k/debugfs.h > > @@ -271,7 +271,7 @@ int ath11k_debugfs_pdev_create(struct > ath11k_base *ab); > > void ath11k_debugfs_pdev_destroy(struct ath11k_base *ab); > > int ath11k_debugfs_register(struct ath11k *ar); > > void ath11k_debugfs_unregister(struct ath11k *ar); -void > > ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff > > *skb); > > +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct > > +ath11k_fw_stats *stats); > > > > void ath11k_debugfs_fw_stats_init(struct ath11k *ar); > > int ath11k_debugfs_get_fw_stats(struct ath11k *ar, u32 pdev_id, @@ > > -352,8 +352,8 @@ static inline void ath11k_debugfs_unregister(struct > ath11k *ar) > > { > > } > > > > -static inline void ath11k_debugfs_fw_stats_process(struct ath11k_base > *ab, > > - struct sk_buff *skb) > > +static inline void ath11k_debugfs_fw_stats_process(struct ath11k *ar, > > + struct ath11k_fw_stats > *stats) > > { > > } > > > > diff --git a/drivers/net/wireless/ath/ath11k/wmi.c > > b/drivers/net/wireless/ath/ath11k/wmi.c > > index 84d1c7054013..9c7a0a438b12 100644 > > --- a/drivers/net/wireless/ath/ath11k/wmi.c > > +++ b/drivers/net/wireless/ath/ath11k/wmi.c > > @@ -7412,7 +7412,53 @@ static void > ath11k_peer_assoc_conf_event(struct > > ath11k_base *ab, struct sk_buff > > > > static void ath11k_update_stats_event(struct ath11k_base *ab, struct > sk_buff *skb) > > { > > - ath11k_debugfs_fw_stats_process(ab, skb); > > + struct ath11k_fw_stats stats = {}; > > + struct ath11k *ar; > > + int ret; > > + > > + INIT_LIST_HEAD(&stats.pdevs); > > + INIT_LIST_HEAD(&stats.vdevs); > > + INIT_LIST_HEAD(&stats.bcn); > > + > > + ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); > > + if (ret) { > > + ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); > > + goto free; > > + } > > + > > + rcu_read_lock(); > > + ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); > > + if (!ar) { > > + rcu_read_unlock(); > > + ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", > > + stats.pdev_id, ret); > > + goto free; > > + } > > + > > + spin_lock_bh(&ar->data_lock); > > + > > + /* WMI_REQUEST_PDEV_STAT can be requested via .get_txpower > mac ops or via > > + * debugfs fw stats. Therefore, processing it separately. > > + */ > > + if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { > > + list_splice_tail_init(&stats.pdevs, &ar->fw_stats.pdevs); > > + ar->fw_stats_done = true; > > + goto complete; > > + } > > + > > + /* WMI_REQUEST_VDEV_STAT, WMI_REQUEST_BCN_STAT and > WMI_REQUEST_RSSI_PER_CHAIN_STAT > > + * are currently requested only via debugfs fw stats. Hence, > processing these > > + * in debugfs context > > + */ > > + ath11k_debugfs_fw_stats_process(ar, &stats); > > + > > +complete: > > + complete(&ar->fw_stats_complete); > > + rcu_read_unlock(); > > + spin_unlock_bh(&ar->data_lock); > > + > > +free: > > + ath11k_fw_stats_free(&stats); > > } > > > > /* PDEV_CTL_FAILSAFE_CHECK_EVENT is received from FW when the > > frequency scanned ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/2] ath11k: move firmware stats out of debugfs @ 2022-06-03 4:23 ` Aditya Kumar Singh (QUIC) 0 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh (QUIC) @ 2022-06-03 4:23 UTC (permalink / raw) To: Jeff Johnson (QUIC); +Cc: linux-wireless, Aditya Kumar Singh (QUIC), ath11k > -----Original Message----- > From: Jeff Johnson (QUIC) <quic_jjohnson@quicinc.com> > Sent: Thursday, June 2, 2022 20:03 > To: Aditya Kumar Singh (QUIC) <quic_adisi@quicinc.com>; > ath11k@lists.infradead.org > Cc: linux-wireless@vger.kernel.org > Subject: Re: [PATCH 1/2] ath11k: move firmware stats out of debugfs > > On 6/1/2022 10:14 PM, Aditya Kumar Singh wrote: > > Currently, firmware stats, comprising pdev, vdev and beacon stats are > > part of debugfs. In firmware pdev stats, firmware reports the final Tx > > power used to transmit each packet. If driver wants to know the final > > Tx power being used at firmware level, it can leverage from firmware > > pdev stats. > > > > Move firmware stats out of debugfs context in order to leverage the > > final Tx power reported in it even when debugfs is disabled. > > > > Tested-on: IPQ8074 hw2.0 AHB > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: QCN9074 hw1.0 PCI > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: WCN6855 hw2.0 PCI > > WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > > --- > > drivers/net/wireless/ath/ath11k/core.c | 46 ++++++++ > > drivers/net/wireless/ath/ath11k/core.h | 12 +- > > drivers/net/wireless/ath/ath11k/debugfs.c | 131 +++++----------------- > > drivers/net/wireless/ath/ath11k/debugfs.h | 6 +- > > drivers/net/wireless/ath/ath11k/wmi.c | 48 +++++++- > > 5 files changed, 136 insertions(+), 107 deletions(-) > > > > diff --git a/drivers/net/wireless/ath/ath11k/core.c > > b/drivers/net/wireless/ath/ath11k/core.c > > index ca6fadd64b43..07a299a2e213 100644 > > --- a/drivers/net/wireless/ath/ath11k/core.c > > +++ b/drivers/net/wireless/ath/ath11k/core.c > > @@ -570,6 +570,52 @@ static inline struct ath11k_pdev > *ath11k_core_get_single_pdev(struct ath11k_base > > return &ab->pdevs[0]; > > } > > > > +void ath11k_fw_stats_pdevs_free(struct list_head *head) { > > + struct ath11k_fw_stats_pdev *i, *tmp; > > + > > + list_for_each_entry_safe(i, tmp, head, list) { > > + list_del(&i->list); > > + kfree(i); > > + } > > +} > > + > > +void ath11k_fw_stats_vdevs_free(struct list_head *head) { > > + struct ath11k_fw_stats_vdev *i, *tmp; > > + > > + list_for_each_entry_safe(i, tmp, head, list) { > > + list_del(&i->list); > > + kfree(i); > > + } > > +} > > + > > +void ath11k_fw_stats_bcn_free(struct list_head *head) { > > + struct ath11k_fw_stats_bcn *i, *tmp; > > + > > + list_for_each_entry_safe(i, tmp, head, list) { > > + list_del(&i->list); > > + kfree(i); > > + } > > +} > > + > > +void ath11k_fw_stats_init(struct ath11k *ar) { > > + INIT_LIST_HEAD(&ar->fw_stats.pdevs); > > + INIT_LIST_HEAD(&ar->fw_stats.vdevs); > > + INIT_LIST_HEAD(&ar->fw_stats.bcn); > > + > > + init_completion(&ar->fw_stats_complete); > > +} > > + > > +void ath11k_fw_stats_free(struct ath11k_fw_stats *stats) { > > + ath11k_fw_stats_pdevs_free(&stats->pdevs); > > + ath11k_fw_stats_vdevs_free(&stats->vdevs); > > + ath11k_fw_stats_bcn_free(&stats->bcn); > > +} > > + > > int ath11k_core_suspend(struct ath11k_base *ab) > > { > > int ret; > > diff --git a/drivers/net/wireless/ath/ath11k/core.h > > b/drivers/net/wireless/ath/ath11k/core.h > > index 65d54e9c15d9..672701f40a6b 100644 > > --- a/drivers/net/wireless/ath/ath11k/core.h > > +++ b/drivers/net/wireless/ath/ath11k/core.h > > @@ -545,9 +545,6 @@ struct ath11k_debug { > > struct dentry *debugfs_pdev; > > struct ath11k_dbg_htt_stats htt_stats; > > u32 extd_tx_stats; > > - struct ath11k_fw_stats fw_stats; > > - struct completion fw_stats_complete; > > - bool fw_stats_done; > > u32 extd_rx_stats; > > u32 pktlog_filter; > > u32 pktlog_mode; > > @@ -712,6 +709,9 @@ struct ath11k { > > u8 twt_enabled; > > bool nlo_enabled; > > u8 alpha2[REG_ALPHA2_LEN + 1]; > > + struct ath11k_fw_stats fw_stats; > > + struct completion fw_stats_complete; > > + bool fw_stats_done; > > }; > > > > struct ath11k_band_cap { > > @@ -1117,6 +1117,12 @@ struct ath11k_fw_stats_bcn { > > u32 tx_bcn_outage_cnt; > > }; > > > > +void ath11k_fw_stats_init(struct ath11k *ar); void > > +ath11k_fw_stats_pdevs_free(struct list_head *head); void > > +ath11k_fw_stats_vdevs_free(struct list_head *head); void > > +ath11k_fw_stats_bcn_free(struct list_head *head); void > > +ath11k_fw_stats_free(struct ath11k_fw_stats *stats); > > + > > extern const struct ce_pipe_config > ath11k_target_ce_config_wlan_ipq8074[]; > > extern const struct service_to_pipe > ath11k_target_service_to_ce_map_wlan_ipq8074[]; > > extern const struct service_to_pipe > > ath11k_target_service_to_ce_map_wlan_ipq6018[]; > > diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c > > b/drivers/net/wireless/ath/ath11k/debugfs.c > > index 0d4843ef9dd1..e4a2fd3da481 100644 > > --- a/drivers/net/wireless/ath/ath11k/debugfs.c > > +++ b/drivers/net/wireless/ath/ath11k/debugfs.c > > @@ -93,91 +93,36 @@ void ath11k_debugfs_add_dbring_entry(struct > ath11k *ar, > > spin_unlock_bh(&dbr_data->lock); > > } > > > > -static void ath11k_fw_stats_pdevs_free(struct list_head *head) -{ > > - struct ath11k_fw_stats_pdev *i, *tmp; > > - > > - list_for_each_entry_safe(i, tmp, head, list) { > > - list_del(&i->list); > > - kfree(i); > > - } > > -} > > - > > -static void ath11k_fw_stats_vdevs_free(struct list_head *head) -{ > > - struct ath11k_fw_stats_vdev *i, *tmp; > > - > > - list_for_each_entry_safe(i, tmp, head, list) { > > - list_del(&i->list); > > - kfree(i); > > - } > > -} > > - > > -static void ath11k_fw_stats_bcn_free(struct list_head *head) -{ > > - struct ath11k_fw_stats_bcn *i, *tmp; > > - > > - list_for_each_entry_safe(i, tmp, head, list) { > > - list_del(&i->list); > > - kfree(i); > > - } > > -} > > > > static void ath11k_debugfs_fw_stats_reset(struct ath11k *ar) > > { > > spin_lock_bh(&ar->data_lock); > > - ar->debug.fw_stats_done = false; > > - ath11k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs); > > - ath11k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs); > > + ar->fw_stats_done = false; > > + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); > > + ath11k_fw_stats_vdevs_free(&ar->fw_stats.vdevs); > > spin_unlock_bh(&ar->data_lock); > > } > > > > -void ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct > > sk_buff *skb) > > +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct > > +ath11k_fw_stats *stats) > > { > > - struct ath11k_fw_stats stats = {}; > > - struct ath11k *ar; > > + struct ath11k_base *ab = ar->ab; > > struct ath11k_pdev *pdev; > > bool is_end; > > static unsigned int num_vdev, num_bcn; > > size_t total_vdevs_started = 0; > > - int i, ret; > > - > > - INIT_LIST_HEAD(&stats.pdevs); > > - INIT_LIST_HEAD(&stats.vdevs); > > - INIT_LIST_HEAD(&stats.bcn); > > - > > - ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); > > - if (ret) { > > - ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); > > - goto free; > > - } > > - > > - rcu_read_lock(); > > - ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); > > - if (!ar) { > > - rcu_read_unlock(); > > - ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", > > - stats.pdev_id, ret); > > - goto free; > > - } > > - > > - spin_lock_bh(&ar->data_lock); > > + int i; > > > > - if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { > > - list_splice_tail_init(&stats.pdevs, &ar- > >debug.fw_stats.pdevs); > > - ar->debug.fw_stats_done = true; > > - goto complete; > > - } > > + /* WMI_REQUEST_PDEV_STAT request has been already processed > */ > > > > - if (stats.stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { > > - ar->debug.fw_stats_done = true; > > - goto complete; > > + if (stats->stats_id == WMI_REQUEST_RSSI_PER_CHAIN_STAT) { > > + ar->fw_stats_done = true; > > + return; > > } > > > > - if (stats.stats_id == WMI_REQUEST_VDEV_STAT) { > > - if (list_empty(&stats.vdevs)) { > > + if (stats->stats_id == WMI_REQUEST_VDEV_STAT) { > > + if (list_empty(&stats->vdevs)) { > > ath11k_warn(ab, "empty vdev stats"); > > - goto complete; > > + return; > > } > > /* FW sends all the active VDEV stats irrespective of PDEV, > > * hence limit until the count of all VDEVs started @@ -190,43 > > +135,34 @@ void ath11k_debugfs_fw_stats_process(struct ath11k_base > > *ab, struct sk_buff *skb > > > > is_end = ((++num_vdev) == total_vdevs_started); > > > > - list_splice_tail_init(&stats.vdevs, > > - &ar->debug.fw_stats.vdevs); > > + list_splice_tail_init(&stats->vdevs, > > + &ar->fw_stats.vdevs); > > > > if (is_end) { > > - ar->debug.fw_stats_done = true; > > + ar->fw_stats_done = true; > > num_vdev = 0; > > } > > - goto complete; > > + return; > > } > > > > - if (stats.stats_id == WMI_REQUEST_BCN_STAT) { > > - if (list_empty(&stats.bcn)) { > > + if (stats->stats_id == WMI_REQUEST_BCN_STAT) { > > + if (list_empty(&stats->bcn)) { > > ath11k_warn(ab, "empty bcn stats"); > > - goto complete; > > + return; > > } > > /* Mark end until we reached the count of all started VDEVs > > * within the PDEV > > */ > > is_end = ((++num_bcn) == ar->num_started_vdevs); > > > > - list_splice_tail_init(&stats.bcn, > > - &ar->debug.fw_stats.bcn); > > + list_splice_tail_init(&stats->bcn, > > + &ar->fw_stats.bcn); > > > > if (is_end) { > > - ar->debug.fw_stats_done = true; > > + ar->fw_stats_done = true; > > num_bcn = 0; > > } > > } > > -complete: > > - complete(&ar->debug.fw_stats_complete); > > - rcu_read_unlock(); > > - spin_unlock_bh(&ar->data_lock); > > - > > -free: > > - ath11k_fw_stats_pdevs_free(&stats.pdevs); > > - ath11k_fw_stats_vdevs_free(&stats.vdevs); > > - ath11k_fw_stats_bcn_free(&stats.bcn); > > } > > > > static int ath11k_debugfs_fw_stats_request(struct ath11k *ar, @@ > > -247,7 +183,7 @@ static int ath11k_debugfs_fw_stats_request(struct > > ath11k *ar, > > > > ath11k_debugfs_fw_stats_reset(ar); > > > > - reinit_completion(&ar->debug.fw_stats_complete); > > + reinit_completion(&ar->fw_stats_complete); > > > > ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); > > > > @@ -258,7 +194,7 @@ static int ath11k_debugfs_fw_stats_request(struct > ath11k *ar, > > } > > > > time_left = > > - wait_for_completion_timeout(&ar->debug.fw_stats_complete, > > + wait_for_completion_timeout(&ar->fw_stats_complete, > > this is a case where imo the existing code was not compliant with the coding > standard (descendant was not indented from the parent) so I'd definitely > want to either indent this or combine it with the prior line > Sure, will address in v2. Thanks for pointing out. > > 1 * HZ); > > if (!time_left) > > return -ETIMEDOUT; > > @@ -268,7 +204,7 @@ static int ath11k_debugfs_fw_stats_request(struct > ath11k *ar, > > break; > > > > spin_lock_bh(&ar->data_lock); > > - if (ar->debug.fw_stats_done) { > > + if (ar->fw_stats_done) { > > spin_unlock_bh(&ar->data_lock); > > break; > > } > > @@ -340,7 +276,7 @@ static int ath11k_open_pdev_stats(struct inode > *inode, struct file *file) > > goto err_free; > > } > > > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, > req_param.stats_id, > > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > > buf); > > there are multiple places in this patch where, due to the fact you are > removing 6 characters from an existing line, a descendant may now fit on the > prior line without exceeding 80 columns. consider removing the line break in > those cases. > > i wasn't going to mention these bikeshed comments but I see something in > the 2nd patch that imo should be addressed, so you'll probably want to > upload a v2 and this would be a good cleanup to apply as well > Yes correct. Will address these v2. Thanks. > > > > file->private_data = buf; > > @@ -412,7 +348,7 @@ static int ath11k_open_vdev_stats(struct inode > *inode, struct file *file) > > goto err_free; > > } > > > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, > req_param.stats_id, > > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > > buf); > > > > file->private_data = buf; > > @@ -490,14 +426,14 @@ static int ath11k_open_bcn_stats(struct inode > *inode, struct file *file) > > } > > } > > > > - ath11k_wmi_fw_stats_fill(ar, &ar->debug.fw_stats, > req_param.stats_id, > > + ath11k_wmi_fw_stats_fill(ar, &ar->fw_stats, req_param.stats_id, > > buf); > > > > /* since beacon stats request is looped for all active VDEVs, saved fw > > * stats is not freed for each request until done for all active VDEVs > > */ > > spin_lock_bh(&ar->data_lock); > > - ath11k_fw_stats_bcn_free(&ar->debug.fw_stats.bcn); > > + ath11k_fw_stats_bcn_free(&ar->fw_stats.bcn); > > spin_unlock_bh(&ar->data_lock); > > > > file->private_data = buf; > > @@ -1055,7 +991,7 @@ void ath11k_debugfs_fw_stats_init(struct ath11k > *ar) > > struct dentry *fwstats_dir = debugfs_create_dir("fw_stats", > > ar- > >debug.debugfs_pdev); > > > > - ar->debug.fw_stats.debugfs_fwstats = fwstats_dir; > > + ar->fw_stats.debugfs_fwstats = fwstats_dir; > > > > /* all stats debugfs files created are under "fw_stats" directory > > * created per PDEV > > @@ -1067,11 +1003,6 @@ void ath11k_debugfs_fw_stats_init(struct > ath11k *ar) > > debugfs_create_file("beacon_stats", 0600, fwstats_dir, ar, > > &fops_bcn_stats); > > > > - INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs); > > - INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs); > > - INIT_LIST_HEAD(&ar->debug.fw_stats.bcn); > > - > > - init_completion(&ar->debug.fw_stats_complete); > > } > > > > static ssize_t ath11k_write_pktlog_filter(struct file *file, diff > > --git a/drivers/net/wireless/ath/ath11k/debugfs.h > > b/drivers/net/wireless/ath/ath11k/debugfs.h > > index 8fc125a71943..e45dc874ff23 100644 > > --- a/drivers/net/wireless/ath/ath11k/debugfs.h > > +++ b/drivers/net/wireless/ath/ath11k/debugfs.h > > @@ -271,7 +271,7 @@ int ath11k_debugfs_pdev_create(struct > ath11k_base *ab); > > void ath11k_debugfs_pdev_destroy(struct ath11k_base *ab); > > int ath11k_debugfs_register(struct ath11k *ar); > > void ath11k_debugfs_unregister(struct ath11k *ar); -void > > ath11k_debugfs_fw_stats_process(struct ath11k_base *ab, struct sk_buff > > *skb); > > +void ath11k_debugfs_fw_stats_process(struct ath11k *ar, struct > > +ath11k_fw_stats *stats); > > > > void ath11k_debugfs_fw_stats_init(struct ath11k *ar); > > int ath11k_debugfs_get_fw_stats(struct ath11k *ar, u32 pdev_id, @@ > > -352,8 +352,8 @@ static inline void ath11k_debugfs_unregister(struct > ath11k *ar) > > { > > } > > > > -static inline void ath11k_debugfs_fw_stats_process(struct ath11k_base > *ab, > > - struct sk_buff *skb) > > +static inline void ath11k_debugfs_fw_stats_process(struct ath11k *ar, > > + struct ath11k_fw_stats > *stats) > > { > > } > > > > diff --git a/drivers/net/wireless/ath/ath11k/wmi.c > > b/drivers/net/wireless/ath/ath11k/wmi.c > > index 84d1c7054013..9c7a0a438b12 100644 > > --- a/drivers/net/wireless/ath/ath11k/wmi.c > > +++ b/drivers/net/wireless/ath/ath11k/wmi.c > > @@ -7412,7 +7412,53 @@ static void > ath11k_peer_assoc_conf_event(struct > > ath11k_base *ab, struct sk_buff > > > > static void ath11k_update_stats_event(struct ath11k_base *ab, struct > sk_buff *skb) > > { > > - ath11k_debugfs_fw_stats_process(ab, skb); > > + struct ath11k_fw_stats stats = {}; > > + struct ath11k *ar; > > + int ret; > > + > > + INIT_LIST_HEAD(&stats.pdevs); > > + INIT_LIST_HEAD(&stats.vdevs); > > + INIT_LIST_HEAD(&stats.bcn); > > + > > + ret = ath11k_wmi_pull_fw_stats(ab, skb, &stats); > > + if (ret) { > > + ath11k_warn(ab, "failed to pull fw stats: %d\n", ret); > > + goto free; > > + } > > + > > + rcu_read_lock(); > > + ar = ath11k_mac_get_ar_by_pdev_id(ab, stats.pdev_id); > > + if (!ar) { > > + rcu_read_unlock(); > > + ath11k_warn(ab, "failed to get ar for pdev_id %d: %d\n", > > + stats.pdev_id, ret); > > + goto free; > > + } > > + > > + spin_lock_bh(&ar->data_lock); > > + > > + /* WMI_REQUEST_PDEV_STAT can be requested via .get_txpower > mac ops or via > > + * debugfs fw stats. Therefore, processing it separately. > > + */ > > + if (stats.stats_id == WMI_REQUEST_PDEV_STAT) { > > + list_splice_tail_init(&stats.pdevs, &ar->fw_stats.pdevs); > > + ar->fw_stats_done = true; > > + goto complete; > > + } > > + > > + /* WMI_REQUEST_VDEV_STAT, WMI_REQUEST_BCN_STAT and > WMI_REQUEST_RSSI_PER_CHAIN_STAT > > + * are currently requested only via debugfs fw stats. Hence, > processing these > > + * in debugfs context > > + */ > > + ath11k_debugfs_fw_stats_process(ar, &stats); > > + > > +complete: > > + complete(&ar->fw_stats_complete); > > + rcu_read_unlock(); > > + spin_unlock_bh(&ar->data_lock); > > + > > +free: > > + ath11k_fw_stats_free(&stats); > > } > > > > /* PDEV_CTL_FAILSAFE_CHECK_EVENT is received from FW when the > > frequency scanned -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] ath11k: move firmware stats out of debugfs 2022-06-03 4:23 ` Aditya Kumar Singh (QUIC) @ 2022-06-03 10:37 ` Kalle Valo -1 siblings, 0 replies; 20+ messages in thread From: Kalle Valo @ 2022-06-03 10:37 UTC (permalink / raw) To: Aditya Kumar Singh (QUIC); +Cc: Jeff Johnson (QUIC), linux-wireless, ath11k "Aditya Kumar Singh (QUIC)" <quic_adisi@quicinc.com> writes: > Sure, will address in v2. Thanks for pointing out. Please edit your quotes. You had over 500 lines of quotes in your one line reply, this makes it horrible to use patchwork: https://patchwork.kernel.org/project/linux-wireless/patch/20220602051425.9265-2-quic_adisi@quicinc.com/ -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] ath11k: move firmware stats out of debugfs @ 2022-06-03 10:37 ` Kalle Valo 0 siblings, 0 replies; 20+ messages in thread From: Kalle Valo @ 2022-06-03 10:37 UTC (permalink / raw) To: Aditya Kumar Singh (QUIC); +Cc: Jeff Johnson (QUIC), linux-wireless, ath11k "Aditya Kumar Singh (QUIC)" <quic_adisi@quicinc.com> writes: > Sure, will address in v2. Thanks for pointing out. Please edit your quotes. You had over 500 lines of quotes in your one line reply, this makes it horrible to use patchwork: https://patchwork.kernel.org/project/linux-wireless/patch/20220602051425.9265-2-quic_adisi@quicinc.com/ -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] ath11k: move firmware stats out of debugfs 2022-06-02 5:14 ` Aditya Kumar Singh @ 2022-06-03 6:50 ` Kalle Valo -1 siblings, 0 replies; 20+ messages in thread From: Kalle Valo @ 2022-06-03 6:50 UTC (permalink / raw) To: Aditya Kumar Singh; +Cc: ath11k, linux-wireless, Aditya Kumar Singh Aditya Kumar Singh <quic_adisi@quicinc.com> wrote: > Currently, firmware stats, comprising pdev, vdev and beacon stats are > part of debugfs. In firmware pdev stats, firmware reports the final > Tx power used to transmit each packet. If driver wants to know the > final Tx power being used at firmware level, it can leverage from > firmware pdev stats. > > Move firmware stats out of debugfs context in order to leverage > the final Tx power reported in it even when debugfs is disabled. > > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> This added new warnings: drivers/net/wireless/ath/ath11k/debugfs.c:94: Please don't use multiple blank lines drivers/net/wireless/ath/ath11k/debugfs.c:976: Blank lines aren't necessary before a close brace '}' I fixed them in the pending branch. -- https://patchwork.kernel.org/project/linux-wireless/patch/20220602051425.9265-2-quic_adisi@quicinc.com/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] ath11k: move firmware stats out of debugfs @ 2022-06-03 6:50 ` Kalle Valo 0 siblings, 0 replies; 20+ messages in thread From: Kalle Valo @ 2022-06-03 6:50 UTC (permalink / raw) To: Aditya Kumar Singh; +Cc: ath11k, linux-wireless, Aditya Kumar Singh Aditya Kumar Singh <quic_adisi@quicinc.com> wrote: > Currently, firmware stats, comprising pdev, vdev and beacon stats are > part of debugfs. In firmware pdev stats, firmware reports the final > Tx power used to transmit each packet. If driver wants to know the > final Tx power being used at firmware level, it can leverage from > firmware pdev stats. > > Move firmware stats out of debugfs context in order to leverage > the final Tx power reported in it even when debugfs is disabled. > > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> This added new warnings: drivers/net/wireless/ath/ath11k/debugfs.c:94: Please don't use multiple blank lines drivers/net/wireless/ath/ath11k/debugfs.c:976: Blank lines aren't necessary before a close brace '}' I fixed them in the pending branch. -- https://patchwork.kernel.org/project/linux-wireless/patch/20220602051425.9265-2-quic_adisi@quicinc.com/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/2] ath11k: move firmware stats out of debugfs 2022-06-03 6:50 ` Kalle Valo @ 2022-06-03 8:30 ` Aditya Kumar Singh (QUIC) -1 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh (QUIC) @ 2022-06-03 8:30 UTC (permalink / raw) To: Kalle Valo; +Cc: ath11k, linux-wireless, Aditya Kumar Singh (QUIC) > -----Original Message----- > From: Kalle Valo <kvalo@kernel.org> > Sent: Friday, June 3, 2022 12:21 > To: Aditya Kumar Singh (QUIC) <quic_adisi@quicinc.com> > Cc: ath11k@lists.infradead.org; linux-wireless@vger.kernel.org; Aditya > Kumar Singh (QUIC) <quic_adisi@quicinc.com> > Subject: Re: [PATCH 1/2] ath11k: move firmware stats out of debugfs > > Aditya Kumar Singh <quic_adisi@quicinc.com> wrote: > > > Currently, firmware stats, comprising pdev, vdev and beacon stats are > > part of debugfs. In firmware pdev stats, firmware reports the final Tx > > power used to transmit each packet. If driver wants to know the final > > Tx power being used at firmware level, it can leverage from firmware > > pdev stats. > > > > Move firmware stats out of debugfs context in order to leverage the > > final Tx power reported in it even when debugfs is disabled. > > > > Tested-on: IPQ8074 hw2.0 AHB > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: QCN9074 hw1.0 PCI > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: WCN6855 hw2.0 PCI > > WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > > This added new warnings: > > drivers/net/wireless/ath/ath11k/debugfs.c:94: Please don't use multiple > blank lines > drivers/net/wireless/ath/ath11k/debugfs.c:976: Blank lines aren't necessary > before a close brace '}' > I have sent [v2] before your mail. But that too will lead to these warnings hence I have rectified and sent [v3]. > I fixed them in the pending branch. Sure thanks. > > -- > https://patchwork.kernel.org/project/linux- > wireless/patch/20220602051425.9265-2-quic_adisi@quicinc.com/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingp > atches ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/2] ath11k: move firmware stats out of debugfs @ 2022-06-03 8:30 ` Aditya Kumar Singh (QUIC) 0 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh (QUIC) @ 2022-06-03 8:30 UTC (permalink / raw) To: Kalle Valo; +Cc: ath11k, linux-wireless, Aditya Kumar Singh (QUIC) > -----Original Message----- > From: Kalle Valo <kvalo@kernel.org> > Sent: Friday, June 3, 2022 12:21 > To: Aditya Kumar Singh (QUIC) <quic_adisi@quicinc.com> > Cc: ath11k@lists.infradead.org; linux-wireless@vger.kernel.org; Aditya > Kumar Singh (QUIC) <quic_adisi@quicinc.com> > Subject: Re: [PATCH 1/2] ath11k: move firmware stats out of debugfs > > Aditya Kumar Singh <quic_adisi@quicinc.com> wrote: > > > Currently, firmware stats, comprising pdev, vdev and beacon stats are > > part of debugfs. In firmware pdev stats, firmware reports the final Tx > > power used to transmit each packet. If driver wants to know the final > > Tx power being used at firmware level, it can leverage from firmware > > pdev stats. > > > > Move firmware stats out of debugfs context in order to leverage the > > final Tx power reported in it even when debugfs is disabled. > > > > Tested-on: IPQ8074 hw2.0 AHB > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: QCN9074 hw1.0 PCI > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: WCN6855 hw2.0 PCI > > WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> > > This added new warnings: > > drivers/net/wireless/ath/ath11k/debugfs.c:94: Please don't use multiple > blank lines > drivers/net/wireless/ath/ath11k/debugfs.c:976: Blank lines aren't necessary > before a close brace '}' > I have sent [v2] before your mail. But that too will lead to these warnings hence I have rectified and sent [v3]. > I fixed them in the pending branch. Sure thanks. > > -- > https://patchwork.kernel.org/project/linux- > wireless/patch/20220602051425.9265-2-quic_adisi@quicinc.com/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingp > atches -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/2] ath11k: add get_txpower mac ops 2022-06-02 5:14 ` Aditya Kumar Singh @ 2022-06-02 5:14 ` Aditya Kumar Singh -1 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh @ 2022-06-02 5:14 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, Aditya Kumar Singh Driver does not support get_txpower mac ops because of which cfg80211 returns vif->bss_conf.txpower to user space. bss_conf.txpower gets its value from ieee80211_channel->max_reg_power. However, the final txpower is dependent on few other parameters apart from max regulatory supported power. It is the firmware which knows about all these parameters and considers the minimum for each packet transmission. All ath11k firmware reports the final tx power in firmware pdev stats which falls under fw_stats. Add get_txpower mac ops to get the tx power from firmware leveraging fw_stats and return it accordingly. Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> --- drivers/net/wireless/ath/ath11k/mac.c | 91 +++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c index f11956163822..f2502ce7deac 100644 --- a/drivers/net/wireless/ath/ath11k/mac.c +++ b/drivers/net/wireless/ath/ath11k/mac.c @@ -8471,6 +8471,94 @@ static int ath11k_mac_op_remain_on_channel(struct ieee80211_hw *hw, return ret; } +static int ath11k_fw_stats_request(struct ath11k *ar, + struct stats_request_params *req_param) +{ + struct ath11k_base *ab = ar->ab; + unsigned long time_left; + int ret; + + lockdep_assert_held(&ar->conf_mutex); + + spin_lock_bh(&ar->data_lock); + ar->fw_stats_done = false; + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); + spin_unlock_bh(&ar->data_lock); + + reinit_completion(&ar->fw_stats_complete); + + ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); + if (ret) { + ath11k_warn(ab, "could not request fw stats (%d)\n", + ret); + return ret; + } + + time_left = wait_for_completion_timeout(&ar->fw_stats_complete, + 1 * HZ); + + if (!time_left) + return -ETIMEDOUT; + + return 0; +} + +static int ath11k_mac_op_get_txpower(struct ieee80211_hw *hw, + struct ieee80211_vif *vif, + int *dbm) +{ + struct ath11k *ar = hw->priv; + struct ath11k_base *ab = ar->ab; + struct stats_request_params req_param; + struct ath11k_fw_stats_pdev *pdev; + int ret; + + /* Final Tx power is minimum of Target Power, CTL power, Regulatory + * Power, PSD EIRP Power. We just know the Regulatory power from the + * regulatory rules obtained. FW knows all these power and sets the min + * of these. Hence, we request the FW pdev stats in which FW reports + * the minimum of all vdev's channel Tx power. + */ + mutex_lock(&ar->conf_mutex); + + if (ar->state != ATH11K_STATE_ON) + goto err_fallback; + + req_param.pdev_id = ar->pdev->pdev_id; + req_param.vdev_id = 0; + req_param.stats_id = WMI_REQUEST_PDEV_STAT; + + ret = ath11k_fw_stats_request(ar, &req_param); + if (ret) { + ath11k_warn(ab, "failed to request fw pdev stats: %d\n", ret); + goto err_fallback; + } + + spin_lock_bh(&ar->data_lock); + pdev = list_first_entry_or_null(&ar->fw_stats.pdevs, + struct ath11k_fw_stats_pdev, list); + if (!pdev) { + spin_unlock_bh(&ar->data_lock); + goto err_fallback; + } + + /* tx power is set as 2 units per dBm in FW. */ + *dbm = pdev->chan_tx_power / 2; + + spin_unlock_bh(&ar->data_lock); + mutex_unlock(&ar->conf_mutex); + + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower: %d from fw\n", __func__, *dbm); + return 0; + +err_fallback: + mutex_unlock(&ar->conf_mutex); + /* We didn't get txpower from FW. Hence, relying on vif->bss_conf.txpower */ + *dbm = vif->bss_conf.txpower; + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower from bss_conf\n", __func__); + return 0; +} + static const struct ieee80211_ops ath11k_ops = { .tx = ath11k_mac_op_tx, .start = ath11k_mac_op_start, @@ -8521,6 +8609,7 @@ static const struct ieee80211_ops ath11k_ops = { #if IS_ENABLED(CONFIG_IPV6) .ipv6_addr_change = ath11k_mac_op_ipv6_changed, #endif + .get_txpower = ath11k_mac_op_get_txpower, .set_sar_specs = ath11k_mac_op_set_bios_sar_specs, .remain_on_channel = ath11k_mac_op_remain_on_channel, @@ -9129,6 +9218,8 @@ int ath11k_mac_allocate(struct ath11k_base *ab) clear_bit(ATH11K_FLAG_MONITOR_VDEV_CREATED, &ar->monitor_flags); ar->vdev_id_11d_scan = ATH11K_11D_INVALID_VDEV_ID; init_completion(&ar->completed_11d_scan); + + ath11k_fw_stats_init(ar); } return 0; -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] ath11k: add get_txpower mac ops @ 2022-06-02 5:14 ` Aditya Kumar Singh 0 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh @ 2022-06-02 5:14 UTC (permalink / raw) To: ath11k; +Cc: linux-wireless, Aditya Kumar Singh Driver does not support get_txpower mac ops because of which cfg80211 returns vif->bss_conf.txpower to user space. bss_conf.txpower gets its value from ieee80211_channel->max_reg_power. However, the final txpower is dependent on few other parameters apart from max regulatory supported power. It is the firmware which knows about all these parameters and considers the minimum for each packet transmission. All ath11k firmware reports the final tx power in firmware pdev stats which falls under fw_stats. Add get_txpower mac ops to get the tx power from firmware leveraging fw_stats and return it accordingly. Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> --- drivers/net/wireless/ath/ath11k/mac.c | 91 +++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c index f11956163822..f2502ce7deac 100644 --- a/drivers/net/wireless/ath/ath11k/mac.c +++ b/drivers/net/wireless/ath/ath11k/mac.c @@ -8471,6 +8471,94 @@ static int ath11k_mac_op_remain_on_channel(struct ieee80211_hw *hw, return ret; } +static int ath11k_fw_stats_request(struct ath11k *ar, + struct stats_request_params *req_param) +{ + struct ath11k_base *ab = ar->ab; + unsigned long time_left; + int ret; + + lockdep_assert_held(&ar->conf_mutex); + + spin_lock_bh(&ar->data_lock); + ar->fw_stats_done = false; + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); + spin_unlock_bh(&ar->data_lock); + + reinit_completion(&ar->fw_stats_complete); + + ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); + if (ret) { + ath11k_warn(ab, "could not request fw stats (%d)\n", + ret); + return ret; + } + + time_left = wait_for_completion_timeout(&ar->fw_stats_complete, + 1 * HZ); + + if (!time_left) + return -ETIMEDOUT; + + return 0; +} + +static int ath11k_mac_op_get_txpower(struct ieee80211_hw *hw, + struct ieee80211_vif *vif, + int *dbm) +{ + struct ath11k *ar = hw->priv; + struct ath11k_base *ab = ar->ab; + struct stats_request_params req_param; + struct ath11k_fw_stats_pdev *pdev; + int ret; + + /* Final Tx power is minimum of Target Power, CTL power, Regulatory + * Power, PSD EIRP Power. We just know the Regulatory power from the + * regulatory rules obtained. FW knows all these power and sets the min + * of these. Hence, we request the FW pdev stats in which FW reports + * the minimum of all vdev's channel Tx power. + */ + mutex_lock(&ar->conf_mutex); + + if (ar->state != ATH11K_STATE_ON) + goto err_fallback; + + req_param.pdev_id = ar->pdev->pdev_id; + req_param.vdev_id = 0; + req_param.stats_id = WMI_REQUEST_PDEV_STAT; + + ret = ath11k_fw_stats_request(ar, &req_param); + if (ret) { + ath11k_warn(ab, "failed to request fw pdev stats: %d\n", ret); + goto err_fallback; + } + + spin_lock_bh(&ar->data_lock); + pdev = list_first_entry_or_null(&ar->fw_stats.pdevs, + struct ath11k_fw_stats_pdev, list); + if (!pdev) { + spin_unlock_bh(&ar->data_lock); + goto err_fallback; + } + + /* tx power is set as 2 units per dBm in FW. */ + *dbm = pdev->chan_tx_power / 2; + + spin_unlock_bh(&ar->data_lock); + mutex_unlock(&ar->conf_mutex); + + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower: %d from fw\n", __func__, *dbm); + return 0; + +err_fallback: + mutex_unlock(&ar->conf_mutex); + /* We didn't get txpower from FW. Hence, relying on vif->bss_conf.txpower */ + *dbm = vif->bss_conf.txpower; + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower from bss_conf\n", __func__); + return 0; +} + static const struct ieee80211_ops ath11k_ops = { .tx = ath11k_mac_op_tx, .start = ath11k_mac_op_start, @@ -8521,6 +8609,7 @@ static const struct ieee80211_ops ath11k_ops = { #if IS_ENABLED(CONFIG_IPV6) .ipv6_addr_change = ath11k_mac_op_ipv6_changed, #endif + .get_txpower = ath11k_mac_op_get_txpower, .set_sar_specs = ath11k_mac_op_set_bios_sar_specs, .remain_on_channel = ath11k_mac_op_remain_on_channel, @@ -9129,6 +9218,8 @@ int ath11k_mac_allocate(struct ath11k_base *ab) clear_bit(ATH11K_FLAG_MONITOR_VDEV_CREATED, &ar->monitor_flags); ar->vdev_id_11d_scan = ATH11K_11D_INVALID_VDEV_ID; init_completion(&ar->completed_11d_scan); + + ath11k_fw_stats_init(ar); } return 0; -- 2.17.1 -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] ath11k: add get_txpower mac ops 2022-06-02 5:14 ` Aditya Kumar Singh @ 2022-06-02 14:51 ` Jeff Johnson -1 siblings, 0 replies; 20+ messages in thread From: Jeff Johnson @ 2022-06-02 14:51 UTC (permalink / raw) To: Aditya Kumar Singh, ath11k; +Cc: linux-wireless On 6/1/2022 10:14 PM, Aditya Kumar Singh wrote: > Driver does not support get_txpower mac ops because of which > cfg80211 returns vif->bss_conf.txpower to user space. bss_conf.txpower > gets its value from ieee80211_channel->max_reg_power. However, the final > txpower is dependent on few other parameters apart from max regulatory > supported power. It is the firmware which knows about all these > parameters and considers the minimum for each packet transmission. > > All ath11k firmware reports the final tx power in firmware pdev stats > which falls under fw_stats. > > Add get_txpower mac ops to get the tx power from firmware leveraging > fw_stats and return it accordingly. > > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > --- > drivers/net/wireless/ath/ath11k/mac.c | 91 +++++++++++++++++++++++++++ > 1 file changed, 91 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c > index f11956163822..f2502ce7deac 100644 > --- a/drivers/net/wireless/ath/ath11k/mac.c > +++ b/drivers/net/wireless/ath/ath11k/mac.c > @@ -8471,6 +8471,94 @@ static int ath11k_mac_op_remain_on_channel(struct ieee80211_hw *hw, > return ret; > } > > +static int ath11k_fw_stats_request(struct ath11k *ar, > + struct stats_request_params *req_param) > +{ > + struct ath11k_base *ab = ar->ab; > + unsigned long time_left; > + int ret; > + > + lockdep_assert_held(&ar->conf_mutex); > + > + spin_lock_bh(&ar->data_lock); > + ar->fw_stats_done = false; > + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); > + spin_unlock_bh(&ar->data_lock); > + > + reinit_completion(&ar->fw_stats_complete); > + > + ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); > + if (ret) { > + ath11k_warn(ab, "could not request fw stats (%d)\n", > + ret); > + return ret; > + } > + > + time_left = wait_for_completion_timeout(&ar->fw_stats_complete, > + 1 * HZ); > + > + if (!time_left) > + return -ETIMEDOUT; > + > + return 0; > +} > + > +static int ath11k_mac_op_get_txpower(struct ieee80211_hw *hw, > + struct ieee80211_vif *vif, > + int *dbm) > +{ > + struct ath11k *ar = hw->priv; > + struct ath11k_base *ab = ar->ab; > + struct stats_request_params req_param; suggest you use an = {} initializer here. > + struct ath11k_fw_stats_pdev *pdev; > + int ret; > + > + /* Final Tx power is minimum of Target Power, CTL power, Regulatory > + * Power, PSD EIRP Power. We just know the Regulatory power from the > + * regulatory rules obtained. FW knows all these power and sets the min > + * of these. Hence, we request the FW pdev stats in which FW reports > + * the minimum of all vdev's channel Tx power. > + */ > + mutex_lock(&ar->conf_mutex); > + > + if (ar->state != ATH11K_STATE_ON) > + goto err_fallback; > + > + req_param.pdev_id = ar->pdev->pdev_id; > + req_param.vdev_id = 0; and remove this explicit setting of an unused param to 0 since it will not be needed if the entire struct is zeroed. the reason for this approach is that if, in the future, any additional fields are added to the struct, you don't want to have a situation where you forget to add code to clear the new fields, and as a result you potentially leak stack memory contents to firmware, which is a security hole. > + req_param.stats_id = WMI_REQUEST_PDEV_STAT; > + > + ret = ath11k_fw_stats_request(ar, &req_param); > + if (ret) { > + ath11k_warn(ab, "failed to request fw pdev stats: %d\n", ret); > + goto err_fallback; > + } > + > + spin_lock_bh(&ar->data_lock); > + pdev = list_first_entry_or_null(&ar->fw_stats.pdevs, > + struct ath11k_fw_stats_pdev, list); > + if (!pdev) { > + spin_unlock_bh(&ar->data_lock); > + goto err_fallback; > + } > + > + /* tx power is set as 2 units per dBm in FW. */ > + *dbm = pdev->chan_tx_power / 2; > + > + spin_unlock_bh(&ar->data_lock); > + mutex_unlock(&ar->conf_mutex); > + > + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower: %d from fw\n", __func__, *dbm); IMO this is misleading. technically pdev->chan_tx_power is the txpower from firmware, *dbm is the derived power after converting units. maybe that is splitting hairs, but when debugging issues you usually want to be very clear about what is the raw data and what is the calculated data Also follow ath11k coding style for debug messages (which follows ath10k) which does not allow colons so I'd suggest "txpower from firmware %d reported %d" > + return 0; > + > +err_fallback: > + mutex_unlock(&ar->conf_mutex); > + /* We didn't get txpower from FW. Hence, relying on vif->bss_conf.txpower */ > + *dbm = vif->bss_conf.txpower; > + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower from bss_conf\n", __func__); I'd log *dbm here as well > + return 0; > +} > + > static const struct ieee80211_ops ath11k_ops = { > .tx = ath11k_mac_op_tx, > .start = ath11k_mac_op_start, > @@ -8521,6 +8609,7 @@ static const struct ieee80211_ops ath11k_ops = { > #if IS_ENABLED(CONFIG_IPV6) > .ipv6_addr_change = ath11k_mac_op_ipv6_changed, > #endif > + .get_txpower = ath11k_mac_op_get_txpower, > > .set_sar_specs = ath11k_mac_op_set_bios_sar_specs, > .remain_on_channel = ath11k_mac_op_remain_on_channel, > @@ -9129,6 +9218,8 @@ int ath11k_mac_allocate(struct ath11k_base *ab) > clear_bit(ATH11K_FLAG_MONITOR_VDEV_CREATED, &ar->monitor_flags); > ar->vdev_id_11d_scan = ATH11K_11D_INVALID_VDEV_ID; > init_completion(&ar->completed_11d_scan); > + > + ath11k_fw_stats_init(ar); > } > > return 0; -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] ath11k: add get_txpower mac ops @ 2022-06-02 14:51 ` Jeff Johnson 0 siblings, 0 replies; 20+ messages in thread From: Jeff Johnson @ 2022-06-02 14:51 UTC (permalink / raw) To: Aditya Kumar Singh, ath11k; +Cc: linux-wireless On 6/1/2022 10:14 PM, Aditya Kumar Singh wrote: > Driver does not support get_txpower mac ops because of which > cfg80211 returns vif->bss_conf.txpower to user space. bss_conf.txpower > gets its value from ieee80211_channel->max_reg_power. However, the final > txpower is dependent on few other parameters apart from max regulatory > supported power. It is the firmware which knows about all these > parameters and considers the minimum for each packet transmission. > > All ath11k firmware reports the final tx power in firmware pdev stats > which falls under fw_stats. > > Add get_txpower mac ops to get the tx power from firmware leveraging > fw_stats and return it accordingly. > > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > --- > drivers/net/wireless/ath/ath11k/mac.c | 91 +++++++++++++++++++++++++++ > 1 file changed, 91 insertions(+) > > diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c > index f11956163822..f2502ce7deac 100644 > --- a/drivers/net/wireless/ath/ath11k/mac.c > +++ b/drivers/net/wireless/ath/ath11k/mac.c > @@ -8471,6 +8471,94 @@ static int ath11k_mac_op_remain_on_channel(struct ieee80211_hw *hw, > return ret; > } > > +static int ath11k_fw_stats_request(struct ath11k *ar, > + struct stats_request_params *req_param) > +{ > + struct ath11k_base *ab = ar->ab; > + unsigned long time_left; > + int ret; > + > + lockdep_assert_held(&ar->conf_mutex); > + > + spin_lock_bh(&ar->data_lock); > + ar->fw_stats_done = false; > + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); > + spin_unlock_bh(&ar->data_lock); > + > + reinit_completion(&ar->fw_stats_complete); > + > + ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); > + if (ret) { > + ath11k_warn(ab, "could not request fw stats (%d)\n", > + ret); > + return ret; > + } > + > + time_left = wait_for_completion_timeout(&ar->fw_stats_complete, > + 1 * HZ); > + > + if (!time_left) > + return -ETIMEDOUT; > + > + return 0; > +} > + > +static int ath11k_mac_op_get_txpower(struct ieee80211_hw *hw, > + struct ieee80211_vif *vif, > + int *dbm) > +{ > + struct ath11k *ar = hw->priv; > + struct ath11k_base *ab = ar->ab; > + struct stats_request_params req_param; suggest you use an = {} initializer here. > + struct ath11k_fw_stats_pdev *pdev; > + int ret; > + > + /* Final Tx power is minimum of Target Power, CTL power, Regulatory > + * Power, PSD EIRP Power. We just know the Regulatory power from the > + * regulatory rules obtained. FW knows all these power and sets the min > + * of these. Hence, we request the FW pdev stats in which FW reports > + * the minimum of all vdev's channel Tx power. > + */ > + mutex_lock(&ar->conf_mutex); > + > + if (ar->state != ATH11K_STATE_ON) > + goto err_fallback; > + > + req_param.pdev_id = ar->pdev->pdev_id; > + req_param.vdev_id = 0; and remove this explicit setting of an unused param to 0 since it will not be needed if the entire struct is zeroed. the reason for this approach is that if, in the future, any additional fields are added to the struct, you don't want to have a situation where you forget to add code to clear the new fields, and as a result you potentially leak stack memory contents to firmware, which is a security hole. > + req_param.stats_id = WMI_REQUEST_PDEV_STAT; > + > + ret = ath11k_fw_stats_request(ar, &req_param); > + if (ret) { > + ath11k_warn(ab, "failed to request fw pdev stats: %d\n", ret); > + goto err_fallback; > + } > + > + spin_lock_bh(&ar->data_lock); > + pdev = list_first_entry_or_null(&ar->fw_stats.pdevs, > + struct ath11k_fw_stats_pdev, list); > + if (!pdev) { > + spin_unlock_bh(&ar->data_lock); > + goto err_fallback; > + } > + > + /* tx power is set as 2 units per dBm in FW. */ > + *dbm = pdev->chan_tx_power / 2; > + > + spin_unlock_bh(&ar->data_lock); > + mutex_unlock(&ar->conf_mutex); > + > + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower: %d from fw\n", __func__, *dbm); IMO this is misleading. technically pdev->chan_tx_power is the txpower from firmware, *dbm is the derived power after converting units. maybe that is splitting hairs, but when debugging issues you usually want to be very clear about what is the raw data and what is the calculated data Also follow ath11k coding style for debug messages (which follows ath10k) which does not allow colons so I'd suggest "txpower from firmware %d reported %d" > + return 0; > + > +err_fallback: > + mutex_unlock(&ar->conf_mutex); > + /* We didn't get txpower from FW. Hence, relying on vif->bss_conf.txpower */ > + *dbm = vif->bss_conf.txpower; > + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower from bss_conf\n", __func__); I'd log *dbm here as well > + return 0; > +} > + > static const struct ieee80211_ops ath11k_ops = { > .tx = ath11k_mac_op_tx, > .start = ath11k_mac_op_start, > @@ -8521,6 +8609,7 @@ static const struct ieee80211_ops ath11k_ops = { > #if IS_ENABLED(CONFIG_IPV6) > .ipv6_addr_change = ath11k_mac_op_ipv6_changed, > #endif > + .get_txpower = ath11k_mac_op_get_txpower, > > .set_sar_specs = ath11k_mac_op_set_bios_sar_specs, > .remain_on_channel = ath11k_mac_op_remain_on_channel, > @@ -9129,6 +9218,8 @@ int ath11k_mac_allocate(struct ath11k_base *ab) > clear_bit(ATH11K_FLAG_MONITOR_VDEV_CREATED, &ar->monitor_flags); > ar->vdev_id_11d_scan = ATH11K_11D_INVALID_VDEV_ID; > init_completion(&ar->completed_11d_scan); > + > + ath11k_fw_stats_init(ar); > } > > return 0; ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 2/2] ath11k: add get_txpower mac ops 2022-06-02 14:51 ` Jeff Johnson @ 2022-06-03 4:41 ` Aditya Kumar Singh (QUIC) -1 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh (QUIC) @ 2022-06-03 4:41 UTC (permalink / raw) To: Jeff Johnson (QUIC); +Cc: linux-wireless, Aditya Kumar Singh (QUIC), ath11k > -----Original Message----- > From: Jeff Johnson (QUIC) <quic_jjohnson@quicinc.com> > Sent: Thursday, June 2, 2022 20:21 > To: Aditya Kumar Singh (QUIC) <quic_adisi@quicinc.com>; > ath11k@lists.infradead.org > Cc: linux-wireless@vger.kernel.org > Subject: Re: [PATCH 2/2] ath11k: add get_txpower mac ops > > On 6/1/2022 10:14 PM, Aditya Kumar Singh wrote: > > Driver does not support get_txpower mac ops because of which > > cfg80211 returns vif->bss_conf.txpower to user space. bss_conf.txpower > > gets its value from ieee80211_channel->max_reg_power. However, the > > final txpower is dependent on few other parameters apart from max > > regulatory supported power. It is the firmware which knows about all > > these parameters and considers the minimum for each packet > transmission. > > > > All ath11k firmware reports the final tx power in firmware pdev stats > > which falls under fw_stats. > > > > Add get_txpower mac ops to get the tx power from firmware leveraging > > fw_stats and return it accordingly. > > > > Tested-on: IPQ8074 hw2.0 AHB > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: QCN9074 hw1.0 PCI > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: WCN6855 hw2.0 PCI > > WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > > --- > > drivers/net/wireless/ath/ath11k/mac.c | 91 > +++++++++++++++++++++++++++ > > 1 file changed, 91 insertions(+) > > > > diff --git a/drivers/net/wireless/ath/ath11k/mac.c > > b/drivers/net/wireless/ath/ath11k/mac.c > > index f11956163822..f2502ce7deac 100644 > > --- a/drivers/net/wireless/ath/ath11k/mac.c > > +++ b/drivers/net/wireless/ath/ath11k/mac.c > > @@ -8471,6 +8471,94 @@ static int > ath11k_mac_op_remain_on_channel(struct ieee80211_hw *hw, > > return ret; > > } > > > > +static int ath11k_fw_stats_request(struct ath11k *ar, > > + struct stats_request_params *req_param) { > > + struct ath11k_base *ab = ar->ab; > > + unsigned long time_left; > > + int ret; > > + > > + lockdep_assert_held(&ar->conf_mutex); > > + > > + spin_lock_bh(&ar->data_lock); > > + ar->fw_stats_done = false; > > + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); > > + spin_unlock_bh(&ar->data_lock); > > + > > + reinit_completion(&ar->fw_stats_complete); > > + > > + ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); > > + if (ret) { > > + ath11k_warn(ab, "could not request fw stats (%d)\n", > > + ret); > > + return ret; > > + } > > + > > + time_left = wait_for_completion_timeout(&ar->fw_stats_complete, > > + 1 * HZ); > > + > > + if (!time_left) > > + return -ETIMEDOUT; > > + > > + return 0; > > +} > > + > > +static int ath11k_mac_op_get_txpower(struct ieee80211_hw *hw, > > + struct ieee80211_vif *vif, > > + int *dbm) > > +{ > > + struct ath11k *ar = hw->priv; > > + struct ath11k_base *ab = ar->ab; > > + struct stats_request_params req_param; > > suggest you use an = {} initializer here. Okay. > > > + struct ath11k_fw_stats_pdev *pdev; > > + int ret; > > + > > + /* Final Tx power is minimum of Target Power, CTL power, > Regulatory > > + * Power, PSD EIRP Power. We just know the Regulatory power from > the > > + * regulatory rules obtained. FW knows all these power and sets the > min > > + * of these. Hence, we request the FW pdev stats in which FW > reports > > + * the minimum of all vdev's channel Tx power. > > + */ > > + mutex_lock(&ar->conf_mutex); > > + > > + if (ar->state != ATH11K_STATE_ON) > > + goto err_fallback; > > + > > + req_param.pdev_id = ar->pdev->pdev_id; > > + req_param.vdev_id = 0; > > and remove this explicit setting of an unused param to 0 since it will not be > needed if the entire struct is zeroed. the reason for this approach is that if, in > the future, any additional fields are added to the struct, you don't want to > have a situation where you forget to add code to clear the new fields, and as > a result you potentially leak stack memory contents to firmware, which is a > security hole. > Sure, will address in v2. Thanks for pointing out. > > + req_param.stats_id = WMI_REQUEST_PDEV_STAT; > > + > > + ret = ath11k_fw_stats_request(ar, &req_param); > > + if (ret) { > > + ath11k_warn(ab, "failed to request fw pdev stats: %d\n", > ret); > > + goto err_fallback; > > + } > > + > > + spin_lock_bh(&ar->data_lock); > > + pdev = list_first_entry_or_null(&ar->fw_stats.pdevs, > > + struct ath11k_fw_stats_pdev, list); > > + if (!pdev) { > > + spin_unlock_bh(&ar->data_lock); > > + goto err_fallback; > > + } > > + > > + /* tx power is set as 2 units per dBm in FW. */ > > + *dbm = pdev->chan_tx_power / 2; > > + > > + spin_unlock_bh(&ar->data_lock); > > + mutex_unlock(&ar->conf_mutex); > > + > > + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower: %d from > fw\n", > > +__func__, *dbm); > > IMO this is misleading. technically pdev->chan_tx_power is the txpower > from firmware, *dbm is the derived power after converting units. maybe > that is splitting hairs, but when debugging issues you usually want to be very > clear about what is the raw data and what is the calculated data > Yes, I see your point. Will rectify this and be clear in debug prints as you have suggested below. > Also follow ath11k coding style for debug messages (which follows > ath10k) which does not allow colons > > so I'd suggest "txpower from firmware %d reported %d" > Sure, will address. > > + return 0; > > + > > +err_fallback: > > + mutex_unlock(&ar->conf_mutex); > > + /* We didn't get txpower from FW. Hence, relying on vif- > >bss_conf.txpower */ > > + *dbm = vif->bss_conf.txpower; > > + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower from > bss_conf\n", > > +__func__); > > I'd log *dbm here as well > Much better. Will rectify in v2. > > + return 0; > > +} > > + > > static const struct ieee80211_ops ath11k_ops = { > > .tx = ath11k_mac_op_tx, > > .start = ath11k_mac_op_start, > > @@ -8521,6 +8609,7 @@ static const struct ieee80211_ops ath11k_ops = { > > #if IS_ENABLED(CONFIG_IPV6) > > .ipv6_addr_change = ath11k_mac_op_ipv6_changed, > > #endif > > + .get_txpower = ath11k_mac_op_get_txpower, > > > > .set_sar_specs = > ath11k_mac_op_set_bios_sar_specs, > > .remain_on_channel = > ath11k_mac_op_remain_on_channel, > > @@ -9129,6 +9218,8 @@ int ath11k_mac_allocate(struct ath11k_base *ab) > > clear_bit(ATH11K_FLAG_MONITOR_VDEV_CREATED, &ar- > >monitor_flags); > > ar->vdev_id_11d_scan = ATH11K_11D_INVALID_VDEV_ID; > > init_completion(&ar->completed_11d_scan); > > + > > + ath11k_fw_stats_init(ar); > > } > > > > return 0; ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 2/2] ath11k: add get_txpower mac ops @ 2022-06-03 4:41 ` Aditya Kumar Singh (QUIC) 0 siblings, 0 replies; 20+ messages in thread From: Aditya Kumar Singh (QUIC) @ 2022-06-03 4:41 UTC (permalink / raw) To: Jeff Johnson (QUIC); +Cc: linux-wireless, Aditya Kumar Singh (QUIC), ath11k > -----Original Message----- > From: Jeff Johnson (QUIC) <quic_jjohnson@quicinc.com> > Sent: Thursday, June 2, 2022 20:21 > To: Aditya Kumar Singh (QUIC) <quic_adisi@quicinc.com>; > ath11k@lists.infradead.org > Cc: linux-wireless@vger.kernel.org > Subject: Re: [PATCH 2/2] ath11k: add get_txpower mac ops > > On 6/1/2022 10:14 PM, Aditya Kumar Singh wrote: > > Driver does not support get_txpower mac ops because of which > > cfg80211 returns vif->bss_conf.txpower to user space. bss_conf.txpower > > gets its value from ieee80211_channel->max_reg_power. However, the > > final txpower is dependent on few other parameters apart from max > > regulatory supported power. It is the firmware which knows about all > > these parameters and considers the minimum for each packet > transmission. > > > > All ath11k firmware reports the final tx power in firmware pdev stats > > which falls under fw_stats. > > > > Add get_txpower mac ops to get the tx power from firmware leveraging > > fw_stats and return it accordingly. > > > > Tested-on: IPQ8074 hw2.0 AHB > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: QCN9074 hw1.0 PCI > > WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > > Tested-on: WCN6855 hw2.0 PCI > > WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3 > > > > Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> > > --- > > drivers/net/wireless/ath/ath11k/mac.c | 91 > +++++++++++++++++++++++++++ > > 1 file changed, 91 insertions(+) > > > > diff --git a/drivers/net/wireless/ath/ath11k/mac.c > > b/drivers/net/wireless/ath/ath11k/mac.c > > index f11956163822..f2502ce7deac 100644 > > --- a/drivers/net/wireless/ath/ath11k/mac.c > > +++ b/drivers/net/wireless/ath/ath11k/mac.c > > @@ -8471,6 +8471,94 @@ static int > ath11k_mac_op_remain_on_channel(struct ieee80211_hw *hw, > > return ret; > > } > > > > +static int ath11k_fw_stats_request(struct ath11k *ar, > > + struct stats_request_params *req_param) { > > + struct ath11k_base *ab = ar->ab; > > + unsigned long time_left; > > + int ret; > > + > > + lockdep_assert_held(&ar->conf_mutex); > > + > > + spin_lock_bh(&ar->data_lock); > > + ar->fw_stats_done = false; > > + ath11k_fw_stats_pdevs_free(&ar->fw_stats.pdevs); > > + spin_unlock_bh(&ar->data_lock); > > + > > + reinit_completion(&ar->fw_stats_complete); > > + > > + ret = ath11k_wmi_send_stats_request_cmd(ar, req_param); > > + if (ret) { > > + ath11k_warn(ab, "could not request fw stats (%d)\n", > > + ret); > > + return ret; > > + } > > + > > + time_left = wait_for_completion_timeout(&ar->fw_stats_complete, > > + 1 * HZ); > > + > > + if (!time_left) > > + return -ETIMEDOUT; > > + > > + return 0; > > +} > > + > > +static int ath11k_mac_op_get_txpower(struct ieee80211_hw *hw, > > + struct ieee80211_vif *vif, > > + int *dbm) > > +{ > > + struct ath11k *ar = hw->priv; > > + struct ath11k_base *ab = ar->ab; > > + struct stats_request_params req_param; > > suggest you use an = {} initializer here. Okay. > > > + struct ath11k_fw_stats_pdev *pdev; > > + int ret; > > + > > + /* Final Tx power is minimum of Target Power, CTL power, > Regulatory > > + * Power, PSD EIRP Power. We just know the Regulatory power from > the > > + * regulatory rules obtained. FW knows all these power and sets the > min > > + * of these. Hence, we request the FW pdev stats in which FW > reports > > + * the minimum of all vdev's channel Tx power. > > + */ > > + mutex_lock(&ar->conf_mutex); > > + > > + if (ar->state != ATH11K_STATE_ON) > > + goto err_fallback; > > + > > + req_param.pdev_id = ar->pdev->pdev_id; > > + req_param.vdev_id = 0; > > and remove this explicit setting of an unused param to 0 since it will not be > needed if the entire struct is zeroed. the reason for this approach is that if, in > the future, any additional fields are added to the struct, you don't want to > have a situation where you forget to add code to clear the new fields, and as > a result you potentially leak stack memory contents to firmware, which is a > security hole. > Sure, will address in v2. Thanks for pointing out. > > + req_param.stats_id = WMI_REQUEST_PDEV_STAT; > > + > > + ret = ath11k_fw_stats_request(ar, &req_param); > > + if (ret) { > > + ath11k_warn(ab, "failed to request fw pdev stats: %d\n", > ret); > > + goto err_fallback; > > + } > > + > > + spin_lock_bh(&ar->data_lock); > > + pdev = list_first_entry_or_null(&ar->fw_stats.pdevs, > > + struct ath11k_fw_stats_pdev, list); > > + if (!pdev) { > > + spin_unlock_bh(&ar->data_lock); > > + goto err_fallback; > > + } > > + > > + /* tx power is set as 2 units per dBm in FW. */ > > + *dbm = pdev->chan_tx_power / 2; > > + > > + spin_unlock_bh(&ar->data_lock); > > + mutex_unlock(&ar->conf_mutex); > > + > > + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower: %d from > fw\n", > > +__func__, *dbm); > > IMO this is misleading. technically pdev->chan_tx_power is the txpower > from firmware, *dbm is the derived power after converting units. maybe > that is splitting hairs, but when debugging issues you usually want to be very > clear about what is the raw data and what is the calculated data > Yes, I see your point. Will rectify this and be clear in debug prints as you have suggested below. > Also follow ath11k coding style for debug messages (which follows > ath10k) which does not allow colons > > so I'd suggest "txpower from firmware %d reported %d" > Sure, will address. > > + return 0; > > + > > +err_fallback: > > + mutex_unlock(&ar->conf_mutex); > > + /* We didn't get txpower from FW. Hence, relying on vif- > >bss_conf.txpower */ > > + *dbm = vif->bss_conf.txpower; > > + ath11k_dbg(ar->ab, ATH11K_DBG_MAC, "%s: txpower from > bss_conf\n", > > +__func__); > > I'd log *dbm here as well > Much better. Will rectify in v2. > > + return 0; > > +} > > + > > static const struct ieee80211_ops ath11k_ops = { > > .tx = ath11k_mac_op_tx, > > .start = ath11k_mac_op_start, > > @@ -8521,6 +8609,7 @@ static const struct ieee80211_ops ath11k_ops = { > > #if IS_ENABLED(CONFIG_IPV6) > > .ipv6_addr_change = ath11k_mac_op_ipv6_changed, > > #endif > > + .get_txpower = ath11k_mac_op_get_txpower, > > > > .set_sar_specs = > ath11k_mac_op_set_bios_sar_specs, > > .remain_on_channel = > ath11k_mac_op_remain_on_channel, > > @@ -9129,6 +9218,8 @@ int ath11k_mac_allocate(struct ath11k_base *ab) > > clear_bit(ATH11K_FLAG_MONITOR_VDEV_CREATED, &ar- > >monitor_flags); > > ar->vdev_id_11d_scan = ATH11K_11D_INVALID_VDEV_ID; > > init_completion(&ar->completed_11d_scan); > > + > > + ath11k_fw_stats_init(ar); > > } > > > > return 0; -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-06-03 10:38 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-02 5:14 [PATCH 0/2] ath11k: add support for get_txpower mac ops Aditya Kumar Singh 2022-06-02 5:14 ` Aditya Kumar Singh 2022-06-02 5:14 ` [PATCH 1/2] ath11k: move firmware stats out of debugfs Aditya Kumar Singh 2022-06-02 5:14 ` Aditya Kumar Singh 2022-06-02 14:33 ` Jeff Johnson 2022-06-02 14:33 ` Jeff Johnson 2022-06-03 4:23 ` Aditya Kumar Singh (QUIC) 2022-06-03 4:23 ` Aditya Kumar Singh (QUIC) 2022-06-03 10:37 ` Kalle Valo 2022-06-03 10:37 ` Kalle Valo 2022-06-03 6:50 ` Kalle Valo 2022-06-03 6:50 ` Kalle Valo 2022-06-03 8:30 ` Aditya Kumar Singh (QUIC) 2022-06-03 8:30 ` Aditya Kumar Singh (QUIC) 2022-06-02 5:14 ` [PATCH 2/2] ath11k: add get_txpower mac ops Aditya Kumar Singh 2022-06-02 5:14 ` Aditya Kumar Singh 2022-06-02 14:51 ` Jeff Johnson 2022-06-02 14:51 ` Jeff Johnson 2022-06-03 4:41 ` Aditya Kumar Singh (QUIC) 2022-06-03 4:41 ` Aditya Kumar Singh (QUIC)
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.