Received-SPF: pass (google.com: domain of linux-wireless+bounces-3655-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <b9e13882-38b6-4f97-9831-26205fb2ddc9@quicinc.com>
Date: Fri, 16 Feb 2024 08:34:27 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] wifi: mac80211: check beacon countdown is complete on
 per link basis
To: Johannes Berg <johannes@sipsolutions.net>
CC: <linux-wireless@vger.kernel.org>
References: <20240215162811.506065-1-quic_adisi@quicinc.com>
 <20240215162811.506065-2-quic_adisi@quicinc.com>
 <189c5d80de6b640686030eef00a1c82f191e5cac.camel@sipsolutions.net>
Content-Language: en-US
From: Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <189c5d80de6b640686030eef00a1c82f191e5cac.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 2/16/24 01:18, Johannes Berg wrote:
> 
>> -bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
>> +bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif,
>> +					 unsigned int link_id)
>>   {
>>   	struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
>> +	struct ieee80211_link_data *link;
>>   	struct beacon_data *beacon = NULL;
>>   	u8 *beacon_data;
>>   	size_t beacon_data_len;
>> @@ -5106,9 +5108,17 @@ bool ieee80211_beacon_cntdwn_is_complete(struct ieee80211_vif *vif)
>>   	if (!ieee80211_sdata_running(sdata))
>>   		return false;
>>   
>> +	if (WARN_ON(link_id >= IEEE80211_MLD_MAX_NUM_LINKS))
>> +		return 0;
>> +
>>   	rcu_read_lock();
>> +
>> +	link = rcu_dereference(sdata->link[link_id]);
>> +	if (!link)
>> +		goto out;
>>
> 
> Maybe that should be a warning too? Not sure I see any case where the
> driver can/should call it with a link that's not even there?
> 

Yes even I don't see any case like that.

> Oh ... and maybe it should check if the link is active? We had the
> sdata_running() check before, but that doesn't mean much for MLO?
> 

Correct. But at least in this function I don't see much use of checking 
if link is active.

> Though then again we have the check below anyway:
> 
>>   	if (vif->type == NL80211_IFTYPE_AP) {
>> -		beacon = rcu_dereference(sdata->deflink.u.ap.beacon);
>> +		beacon = rcu_dereference(link->u.ap.beacon);
>>   		if (WARN_ON(!beacon || !beacon->tail))
>>   			goto out;
>>
> 
> So that will just be NULL if it's not active... so I guess that's fine.
> 
Yep. That's the intention here. Ultimately beacon would be NULL if link 
is not active so eventually a WARN_ON() will trigger.