Sender: wgong=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date:   Fri, 21 May 2021 10:21:33 +0800
From:   Wen Gong <wgong@codeaurora.org>
To:     Arend van Spriel <arend.vanspriel@broadcom.com>
Cc:     johannes@sipsolutions.net, ath11k@lists.infradead.org,
        linux-wireless@vger.kernel.org
Subject: Re: [PATCH] wireless: add check of field VHT Extended NSS BW Capable
 for 160/80+80 MHz setting
In-Reply-To: <fe1b7675-23ba-a86e-90db-de5803231e21@broadcom.com>
References: <20210520034021.14690-1-wgong@codeaurora.org>
 <fe1b7675-23ba-a86e-90db-de5803231e21@broadcom.com>
Message-ID: <2f580f80af0ec9b17068d80d42e666b9@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

On 2021-05-21 03:21, Arend van Spriel wrote:
> On 5/20/2021 5:40 AM, Wen Gong wrote:
>> Table 9-251—Supported VHT-MCS and NSS Set subfields, it has subfield 
>> VHT Extended
>> NSS BW Capable, its definition is:
>> Indicates whether the STA is capable of interpreting the Extended NSS 
>> BW
>> Support subfield of the VHT Capabilities Information field.
>> 
>> This patch is to add check for the subfield.
> 
> Almost looks good to me, but...
> 
>> Signed-off-by: Wen Gong <wgong@codeaurora.org>
>> ---
>>   net/wireless/chan.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
>> index 285b8076054b..f7f86bd75cc0 100644
>> --- a/net/wireless/chan.c
>> +++ b/net/wireless/chan.c
> 
> [...]
> 
>> @@ -1037,7 +1039,7 @@ bool cfg80211_chandef_usable(struct wiphy 
>> *wiphy,
>>   		cap = vht_cap->cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK;
>>   		if (cap != IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160MHZ &&
>>   		    cap != IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ &&
>> -		    !(vht_cap->cap & IEEE80211_VHT_CAP_EXT_NSS_BW_MASK))
>> +		    !(ext_nss_cap && vht_cap->cap & 
>> IEEE80211_VHT_CAP_EXT_NSS_BW_MASK))
> 
> you should not mix logical and binary operators so add braces around
> the binary operation here.
priority of & is higher than &&, so it not need to add braces here.
however, i have sent patch v2 which added the braces.
https://lore.kernel.org/linux-wireless/20210521021809.23986-1-wgong@codeaurora.org/T/#u
> 
> Regards,
> Arend
> 
>>   			return false;
>>   		break;
>>   	default:
>>