> -----Original Message-----
> From: Jeff Johnson (QUIC) <quic_jjohnson@xxxxxxxxxxx>
> Sent: Thursday, June 2, 2022 20:21
> To: Aditya Kumar Singh (QUIC) <quic_adisi@xxxxxxxxxxx>;
> ath11k@xxxxxxxxxxxxxxxxxxx
> Cc: linux-wireless@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxx>
> > ---
> > 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;
