Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 08DB2C433CA
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Venkateswara Naralasetty <vnaralas@codeaurora.org>
Cc:     linux-wireless@vger.kernel.org, ath11k@lists.infradead.org
Subject: Re: [PATCHv2 2/2] ath11k: Add ap power save support
References: <1598257589-19091-1-git-send-email-vnaralas@codeaurora.org>
        <1598257589-19091-2-git-send-email-vnaralas@codeaurora.org>
        <87h7n28di7.fsf@codeaurora.org>
Date:   Wed, 27 Jan 2021 20:14:13 +0200
In-Reply-To: <87h7n28di7.fsf@codeaurora.org> (Kalle Valo's message of "Wed, 27
        Jan 2021 20:02:08 +0200")
Message-ID: <87czxq8cy2.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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