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 41DC6C433ED
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Venkateswara Naralasetty <vnaralas@codeaurora.org>
Cc:     ath11k@lists.infradead.org, linux-wireless@vger.kernel.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>
Date:   Wed, 27 Jan 2021 20:02:08 +0200
In-Reply-To: <1598257589-19091-2-git-send-email-vnaralas@codeaurora.org>
        (Venkateswara Naralasetty's message of "Mon, 24 Aug 2020 13:56:29
        +0530")
Message-ID: <87h7n28di7.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

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.

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

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