| Message ID | 20220602051425.9265-3-quic_adisi@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ath11k: add support for get_txpower mac ops | expand |
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;
> -----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;
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;
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(+)