From patchwork Fri Jan 13 08:54:35 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Felix Fietkau X-Patchwork-Id: 9514853 X-Patchwork-Delegate: johannes@sipsolutions.net Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E9E25607D4 for ; Fri, 13 Jan 2017 08:54:45 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E41CF285ED for ; Fri, 13 Jan 2017 08:54:45 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D785B286A4; Fri, 13 Jan 2017 08:54:45 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4140D28696 for ; Fri, 13 Jan 2017 08:54:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751328AbdAMIyk (ORCPT ); Fri, 13 Jan 2017 03:54:40 -0500 Received: from nbd.name ([46.4.11.11]:48262 "EHLO nbd.name" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751298AbdAMIyj (ORCPT ); Fri, 13 Jan 2017 03:54:39 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=nbd.name; s=20160729; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:MIME-Version:Date:Message-ID:From:Cc:References:To:Subject; bh=baxekeWoirg9tcocSpLAnmhQ5SwUDU8DhfNE5PXcKK4=; b=isCcy6VepJZD6LSuS89vxLxwu1QdTUYRRvR+tUXBUUPTG2ZjH2nn1Vz8s1ddUYNCnJPlaehr4kWririww7NiIObIEolc/zCNDHyM4AOufy4IzuFaFnys/Uxlbpl+S1E1uKuo/SF8bz79W4ygfL8VeZnW2B2MsDWyRi9A5fq0KTY=; Subject: Re: [PATCH] mac80211: initialize SMPS field in HT capabilities To: Johannes Berg , linux-wireless@vger.kernel.org References: <20170111223322.14698-1-nbd@nbd.name> <1484295655.19860.9.camel@sipsolutions.net> Cc: onelektra@gmx.net From: Felix Fietkau Message-ID: Date: Fri, 13 Jan 2017 09:54:35 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <1484295655.19860.9.camel@sipsolutions.net> Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 2017-01-13 09:20, Johannes Berg wrote: > On Wed, 2017-01-11 at 23:33 +0100, Felix Fietkau wrote: >> ibss and mesh modes copy the ht capabilites from the band without >> overriding the SMPS state. Unfortunately the default value 0 for the >> SMPS field means static SMPS instead of disabled. >> >> This results in HT ibss and mesh setups using only single-stream >> rates, >> even though SMPS is not supposed to be active. >> >> Initialize SMPS to disabled for all bands on ieee80211_hw_register to >> ensure that the value is sane where it is not overriden with the real >> SMPS state. > > Hmm. I guess the only other place affected by it will be scanning? Right. >> Reported-by: Elektra Wagenrad >> Signed-off-by: Felix Fietkau >> --- >> net/mac80211/main.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/net/mac80211/main.c b/net/mac80211/main.c >> index 1822c77f2b1c..c269046aa02b 100644 >> --- a/net/mac80211/main.c >> +++ b/net/mac80211/main.c >> @@ -913,10 +913,15 @@ int ieee80211_register_hw(struct ieee80211_hw >> *hw) >> supp_ht = supp_ht || sband->ht_cap.ht_supported; >> supp_vht = supp_vht || sband->vht_cap.vht_supported; >> >> - if (sband->ht_cap.ht_supported) >> - local->rx_chains = >> - max(ieee80211_mcs_to_chains(&sband- >> >ht_cap.mcs), >> - local->rx_chains); >> + if (!sband->ht_cap.ht_supported) >> + continue; >> + >> + local->rx_chains = >> + max(ieee80211_mcs_to_chains(&sband- >> >ht_cap.mcs), >> + local->rx_chains); >> + >> + sband->ht_cap.cap |= WLAN_HT_CAP_SM_PS_DISABLED << >> + IEEE80211_HT_CAP_SM_PS_SHIFT; > > This ... looks fishy. I know it's not, since it sets both bits, but > still. > > Additionally, ath10k appears to be setting this to > WLAN_HT_CAP_SM_PS_DYNAMIC already, so apparently it's expecting > something to happen with that value? Is it really correct then to be > overwriting it? Actually, that code seems to leave the value at WLAN_HT_CAP_SM_PS_DISABLED, because it sets that first and doesn't mask out the field before trying to set it to WLAN_HT_CAP_SM_PS_DYNAMIC. I don't think it even makes sense to set WLAN_HT_CAP_SM_PS_DYNAMIC at this point, since it's up to mac80211 to deal with the SMPS state. Either way, WLAN_HT_CAP_SM_PS_STATIC is a really bad default to have at init time. If you want, I can change the patch to check for that value before changing it, but I don't really see the point. Additionally, I found this ath10k commit: commit e33a99e227e430a788467e5a85dc29f6df16b983 Author: Peter Oh Date: Thu Dec 31 15:26:20 2015 +0200 ath10k: set SM power save disabled to default value Use SMPS disabled as default because FW does not indicate any support of SMPS. This change will help STAs out that don’t support SMPS from sticking on 1SS, since they don’t have method to change it back to multiple chains. This change also should not affect power consumption of STAs supporting SMPS, because they are capable to switch the mode to dynamic or static either at the end of frame sequence or by using SMPS action frame. Signed-off-by: Peter Oh Signed-off-by: Kalle Valo - Felix diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index b4bdeb07a012..6146a293601a 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -3860,7 +3860,8 @@ static struct ieee80211_sta_ht_cap ath10k_get_ht_cap(struct ath10k *ar) ht_cap.ampdu_density = IEEE80211_HT_MPDU_DENSITY_8; ht_cap.cap |= IEEE80211_HT_CAP_SUP_WIDTH_20_40; ht_cap.cap |= IEEE80211_HT_CAP_DSSSCCK40; - ht_cap.cap |= WLAN_HT_CAP_SM_PS_STATIC << IEEE80211_HT_CAP_SM_PS_SHIFT; + ht_cap.cap |= + WLAN_HT_CAP_SM_PS_DISABLED << IEEE80211_HT_CAP_SM_PS_SHIFT; if (ar->ht_cap_info & WMI_HT_CAP_HT20_SGI) ht_cap.cap |= IEEE80211_HT_CAP_SGI_20;