RE: [PATCH 2/2] ath11k: add get_txpower mac ops

["Aditya Kumar Singh (QUIC)" <quic_adisi@quicinc.com>]

> -----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;