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: <124e5f40-04b4-21be-9e37-bc01633e8e7a@quicinc.com>
Date:   Thu, 11 Aug 2022 15:58:27 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.1.2
Subject: Re: [RFC 2/2] wifi: mac80211: use link_id from ieee80211_rx_status to
 retrieve rx link
Content-Language: en-US
To:     Johannes Berg <johannes@sipsolutions.net>
CC:     <linux-wireless@vger.kernel.org>
References: <20220802065019.20791-1-quic_vthiagar@quicinc.com>
 <20220802065019.20791-3-quic_vthiagar@quicinc.com>
 <103f62efc89d86773fbd07729326f3ba08f4ea23.camel@sipsolutions.net>
From:   Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
In-Reply-To: <103f62efc89d86773fbd07729326f3ba08f4ea23.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 8/9/2022 11:45 PM, Johannes Berg wrote:
> On Tue, 2022-08-02 at 12:20 +0530, Vasanthakumar Thiagarajan wrote:
>> Fill rx.link with respective data_link from the reported link_id
>> in rx_status. Any link_id > 15 is invalid. Non-MLO connections
>> can use either 0 or 15 as the link_id. Please note that link_id
>> 0 is used with non-MLO connections to avoid changes in the
>> drivers not supporting MLO. For a 802.11 MLD address translated
>> frame, driver must report the right link_id for the
>> frame to get processed. When processing 802.3 frame format,
>> link_id is not that critical, used only with stats update.
>> In such case, all the stats will be updated for the deflink.
> 
> I think it might be worth splitting this patch a bit different - putting
> some parts into the first patch already (that fill rx.link_id), and
> keeping some others (statistics etc.) in this patch, which is then more
> related to statistics?

Sure.

> 
>>
>>   	if (rx->link_id >= 0) {
>> -		link = rcu_dereference(rx->sdata->link[rx->link_id]);
>> -
>> +		link =  rcu_dereference(sdata->link[rx->link_id]);
> 
> that has some spurious whitespace changes
> 
>> +	/*
>> +	 * TODO: In MLO, should the frame be dropped if the right link_id is not
>> +	 * available? Or may be it is fine in the current form to proceed with
>> +	 * the frame processing because with frame being in 802.3 format,
>> +	 * link_id is used only for stats purpose and updating the stats on
>> +	 * the deflink is fine?
>> +	 */
>> +	if (pubsta->mlo && status->link_id != IEEE80211_LINK_UNSPECIFIED)
>> +		rx.link_id = status->link_id;
> 
> If the driver *does* give a link ID, it better be valid?

Sure, we can remove IEEE80211_LINK_UNSPECIFIED. If now valid
link_id is set for an MLO link, we still try to retrieve it from
the addr2 in case the MLD address translation is not done in the
lower layers but when this happens with already translated packets
then we may need to drop the packet?

> 
> OTOH, could there be races, e.g. while disabling a link?

I assume this is for the packets where the received on a
link which is disabled? May be driver sets that link but
that link will not be valid in mac80211 while processing
this packet and the packet will be dropped? Or may be
a different race you are mentioning?

Vasanth