Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <96ae3c1d-274b-00b1-cb19-a9a301b6aa56@quicinc.com>
Date:   Fri, 15 Sep 2023 16:33:30 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.0
Subject: Re: [PATCH 2/3] wifi: mac80211: add support to allow driver to set
 active link while connection for station
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>,
        <ath12k@lists.infradead.org>
CC:     <linux-wireless@vger.kernel.org>,
        Jeff Johnson <quic_jjohnson@quicinc.com>
References: <20230906103458.24092-1-quic_wgong@quicinc.com>
 <20230906103458.24092-3-quic_wgong@quicinc.com>
 <4b597bba9dc13c3260f174826de38b4f1bc3bf6e.camel@sipsolutions.net>
From:   Wen Gong <quic_wgong@quicinc.com>
In-Reply-To: <4b597bba9dc13c3260f174826de38b4f1bc3bf6e.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 9/13/2023 4:58 PM, Johannes Berg wrote:
> On Wed, 2023-09-06 at 06:34 -0400, Wen Gong wrote:
>> There are 2 fields valid_links and active_links in struct ieee80211_vif
>> of mac80211. For station mode, valid_links is always include the bitmap
>> of active_links. valid_links stores the bitmap of links which is created
>> in mac80211, and mac80211 only indicate the info of links which is existed
>> in active_links to driver. Finally, the active_links is the bitmap of
>> links which station can interactive with MLO AP it is connected to.
>>
>> Currently the active links is always only contain the primary link,
>> primary link means the link used by the latest exchange of successful
>> (Re)Association Request/Response frames. Then it will always have only
>> one link in driver after connected.
>>
>> Hence add this ops in struct ieee80211_ops to allow driver to determine
>> the active links bit map dynamically while connecting to MLO AP.
> This commit log does nothing to address the question "why do we need
> this", particularly since we already have
> ieee80211_set_active_links(/_async), so the driver can pretty much
> immediately enable all the links it wants.
>
> I see no value in this patch whatsoever.
For ieee80211_set_active_links(), driver need save and install pairwise
keys for the other links as you said in link below. But driver do not
save the key data currently. So driver could not install the pairwise
keys by itself.
https://lore.kernel.org/linux-wireless/50719d34bc48d816d00b56d3d9efdb59e3e51a16.camel@sipsolutions.net/

And driver could ensure this rule in spec as I said before in link below:
"For MLO, if RSNA has not been established, each message of the 4-way
handshake shall be sent on the same link used by the latest exchange of
successful (Re)Association Request/Response frames."
https://lore.kernel.org/linux-wireless/b6314b61-0700-0e1c-7b39-21c305dc28b3@quicinc.com/

It will be complext for driver/ath12k to use 
ieee80211_set_active_links(/_async).
>
>> @@ -166,6 +167,13 @@ static void ieee80211_set_vif_links_bitmaps(struct ieee80211_sub_if_data *sdata,
>>   		WARN_ON(dormant_links);
>>   		break;
>>   	case NL80211_IFTYPE_STATION:
>> +		active_links = drv_calculate_active_links(sdata->local, sdata,
>> +							  valid_links & ~dormant_links);
>> +		if (active_links) {
>> +			sdata->vif.active_links = active_links;
>> +			break;
>> +		}
>>
> I also _really_ don't think this should operate at this low-level
> infrastructure.

I really want to know the reason why active *only* the assoc link for 
station, but

active *all* links for AP here?????

> johannes