Re: [PATCHv2 2/2] ath11k: Add ap power save support

[Kalle Valo <kvalo@codeaurora.org>]

Kalle Valo <kvalo@codeaurora.org> writes:

> Venkateswara Naralasetty <vnaralas@codeaurora.org> writes:
>
>> AP power save where AP goes to power save mode when no stations associate
>> to it and come out of power save when any station associate to AP.
>>
>> This AP power save capability can be used to save power with the drawback
>> of reduced range or delayed discovery of the AP
>>
>> This patch also porvides user configuration to enable/disable
>> this feature using vendor command. This feature is disabled by default.
>>
>> Tested-on: IPQ8074 WLAN.HK.2.1.0.1-01228-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Venkateswara Naralasetty <vnaralas@codeaurora.org>
>
> [...]
>
>> +static int ath11k_vendor_set_wifi_config(struct wiphy *wihpy,
>
> s/wihpy/wiphy/
>
>> +					 struct wireless_dev *wdev,
>> +					 const void *data,
>> +					 int data_len)
>> +{
>> +	struct ieee80211_vif *vif;
>> +	struct ath11k_vif *arvif;
>> +	struct ath11k *ar;
>> +	struct nlattr *tb[QCA_WLAN_VENDOR_ATTR_CONFIG_MAX + 1];
>> +	int ret = 0;
>> +
>> +	if (!wdev)
>> +		return -EINVAL;
>> +
>> +	vif = wdev_to_ieee80211_vif(wdev);
>> +	if (!vif)
>> +		return -EINVAL;
>> +
>> +	arvif = (struct ath11k_vif *)vif->drv_priv;
>> +	if (!arvif)
>> +		return -EINVAL;
>> +
>> +	ar = arvif->ar;
>> +
>> +	mutex_lock(&ar->conf_mutex);
>> +
>> +	ret = nla_parse(tb, QCA_WLAN_VENDOR_ATTR_CONFIG_MAX, data, data_len,
>> +			ath11k_vendor_set_wifi_config_policy, NULL);
>> +	if (ret) {
>> +		ath11k_warn(ar->ab, "invalid set wifi config policy attribute\n");
>> +		goto exit;
>> +	}
>> +
>> +	ar->ap_ps_enabled = nla_get_flag(tb[QCA_WLAN_VENDOR_ATTR_CONFIG_GTX]);
>> +	ret = ath11k_mac_ap_ps_recalc(ar);
>> +	if (ret) {
>> +		ath11k_warn(ar->ab, "failed to send ap ps ret %d\n", ret);
>> +		goto exit;
>> +	}
>> +
>> +exit:
>> +	mutex_unlock(&ar->conf_mutex);
>> +	return ret;
>> +}
>
> Something which I find awkward here is that this is per pdev (=all
> vdevs), even though the vendor command is per vif. So if you change the
> config on one vif, all other vifs will change as well. And there's no
> way to check if the state from user space as there's only a set command
> and no equivalent get command.

Actually the problem comes here:

+static struct wiphy_vendor_command ath11k_vendor_commands[] = {
+	{
+		.info.vendor_id = QCA_NL80211_VENDOR_ID,
+		.info.subcmd = QCA_NL80211_VENDOR_SUBCMD_SET_WIFI_CONFIGURATION,
+		.flags = WIPHY_VENDOR_CMD_NEED_WDEV |
+			WIPHY_VENDOR_CMD_NEED_RUNNING,
+		.doit = ath11k_vendor_set_wifi_config,
+		.policy = ath11k_vendor_set_wifi_config_policy,
+		.maxattr = QCA_WLAN_VENDOR_ATTR_CONFIG_MAX
+	}

If it's per pdev you shouldn't set WIPHY_VENDOR_CMD_NEED_WDEV.

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

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