Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <76266a0b-d371-53c1-9ad0-fbff7a506d0c@quicinc.com>
Date:   Wed, 23 Nov 2022 10:18:59 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [RFC PATCH] mac80211: mlme: Handle Puncturing information
 received from the AP
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>,
        <linux-wireless@vger.kernel.org>
CC:     Aloka Dixit <quic_alokad@quicinc.com>,
        Miri Korenblit <miriam.rachel.korenblit@intel.com>,
        "Carl Huang (QUIC)" <quic_cjhuang@quicinc.com>
References: <20220325140859.e48bf244f157.I3547481d49f958389f59dfeba3fcc75e72b0aa6e@changeid>
 <e4db49e4-6363-0c8d-10dd-a1a564da2542@quicinc.com>
 <95ad4207e62b4990476d867bd240fef3ede31369.camel@sipsolutions.net>
From:   Kang Yang <quic_kangyang@quicinc.com>
In-Reply-To: <95ad4207e62b4990476d867bd240fef3ede31369.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi:

	Thanks for your reply!

On 11/23/2022 12:43 AM, Johannes Berg wrote:
> On Mon, 2022-11-21 at 15:29 +0800, Kang Yang wrote:
>> Hi:
>> 	1.Do you have any new version about this RFC patch?
> 
> Not really, no.
> 
> 
>> 	2.I have some questions about this patch:
> 
> (a couple of blank lines and some trimming of the context really would
> help ... please try next time)
> 
>>> +static u16
>>> +ieee80211_extract_dis_subch_bmap(const struct ieee80211_eht_operation *eht_oper,
>>> +				 struct cfg80211_chan_def *chandef, u16 bitmap)
>>> +{
>>> +	int sta_center_freq = ieee80211_channel_to_frequency(eht_oper->ccfs,
>>> +							     chandef->chan->band);
>>> +	u32 center_freq = chandef->chan->center_freq;
>> The shift is calculated by the difference of old and new channel center
>> frequency.The new channel center frequency should be
>> "chandef->center_freq1" after BW negotitaion.
>> "chandef->chan->center_freq" is the primary channel frequency.
> 
> 
> Yeah I think we did fix a couple of bugs in this area later.
> 
>>> +	u8 sta_bw = 20 * BIT(u8_get_bits(eht_oper->chan_width,
>>> +					 IEEE80211_EHT_OPER_CHAN_WIDTH));
>>> +	u8 bw = 20 * BIT(ieee80211_chan_width_to_rx_bw(chandef->width));
>>> +	int sta_start_freq = sta_center_freq - sta_bw / 2;
>>> +	int start_freq = center_freq - bw / 2;
>>> +	u16 shift = (start_freq - sta_start_freq) / 20;
>>> +	u16 mask = BIT(sta_bw / 20) - 1;
>> The mask is used to extra the valid bit according to the new BW,
>> but current algorithm is using the old bandwidth.
> 
> 
> :)
> 
>>> +	while (chandef->width > NL80211_CHAN_WIDTH_40) {
>>> +		extracted =
>>> +			ieee80211_extract_dis_subch_bmap(eht_oper, chandef,
>>> +							 bitmap);
>>> +
>>> +		if (ieee80211_valid_disable_subchannel_bitmap(&bitmap,
>>> +							      chandef->width))
>> Here extract the bitmap according new negotiated BW. After extracting,
>> check whether it is valid.
>> I think you should use "&extracted"  instead of "&bitmap"
> 
> Yeah I guess that makes sense.
> 
> I don't know what fixes we have and what version of this patch this is.
> 
>>> +static bool ieee80211_config_puncturing(struct ieee80211_sub_if_data *sdata,
>>> +					const struct ieee80211_eht_operation *eht_oper,
>>> +					u64 *changed)
>>> +{
>>> +	u16 bitmap, extracted;
>>> +	u8 bw;
>>> +
>>> +	if (!u8_get_bits(eht_oper->present_bm,
>>> +			 IEEE80211_EHT_OPER_DISABLED_SUBCHANNEL_BITMAP_PRESENT))
>>> +		bitmap = 0;
>>> +	else
>>> +		bitmap = get_unaligned_le16(eht_oper->disable_subchannel_bitmap);
>>> +
>> Should check initial bitmap here.
> 
> 
> What do you mean by "initial" here?


"Initial bitmap" is the original bitmap that AP send in beacon . STA 
will extract it according to the negotiated BW. So i call it "Initial 
bitmap".

This is my personal statement.


> 
>>> +	extracted = ieee80211_extract_dis_subch_bmap(eht_oper,
>>> +						     &sdata->vif.bss_conf.chandef,
>>> +						     bitmap);
>>> +
>>> +	/* accept if there are no changes */
>>> +	if (!(*changed & BSS_CHANGED_BANDWIDTH) &&
>>> +	    extracted == sdata->vif.bss_conf.eht_puncturing)
>>> +		return true;
>>> +
>>> +	bw = u8_get_bits(eht_oper->chan_width, IEEE80211_EHT_OPER_CHAN_WIDTH);
>>> +
>>> +	if (!ieee80211_valid_disable_subchannel_bitmap(&bitmap, bw)) {
>>> +		sdata_info(sdata,
>>> +			   "Got an invalid disable subchannel bitmap from AP %pM: bitmap = 0x%x, bw = 0x%x. disconnect\n",
>>> +			    sdata->u.mgd.associated->bssid, bitmap, bw);
>>> +		return false;
>>> +	}
>> The initial bitmap received from the AP is checked here.
>> I think it should be carried out before the extraction above.
> 
> Ah, yes I guess that makes sense.
> 
> 
> Anyway the more fundamental thing we have to figure out here (and thanks
> for bringing this back) is how we treat the puncturing - QCOM's AP-side
> puncturing patch treated it as part of the chandef, but that's not
> working well for client side ...
> 


Yes, to my understanding, I think it's more appropriate to define it 
like you in "ieee80211_bss_conf".

But this should be discussed by people like you. I'm just responsible 
for developing according to these definitions.


> johannes

My question is over.

Thanks again for your patient reply.


Best regard
Kang