Received-SPF: pass (google.com: domain of linux-wireless+bounces-5989-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: <15bc7705-5da9-4c56-aa45-81972bad5e01@quicinc.com>
Date: Tue, 9 Apr 2024 09:27:16 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: mac80211: handle sdata->u.ap.active flag with MLO
Content-Language: en-US
To: Johannes Berg <johannes@sipsolutions.net>
CC: <linux-wireless@vger.kernel.org>
References: <20240326151141.3824454-1-quic_adisi@quicinc.com>
 <bae1e71edcc13e73e8ef7f24d30140a4a6aefb98.camel@sipsolutions.net>
From: Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <bae1e71edcc13e73e8ef7f24d30140a4a6aefb98.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 4/8/24 23:55, Johannes Berg wrote:
> On Tue, 2024-03-26 at 20:41 +0530, Aditya Kumar Singh wrote:
> 
>> @@ -1232,7 +1256,9 @@ ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
>>   	}
>>   
>>   	rcu_assign_pointer(link->u.ap.beacon, new);
>> -	sdata->u.ap.active = true;
>> +
>> +	if (ieee80211_num_beaconing_links(sdata) <= 1)
>> +		sdata->u.ap.active = true;
> 
> I don't understand this change. Neither the <= 1 really, nor the fact
> that you actually _make_ this change.
> 

The place above where we are checking number of beaconing links, at that 
point at least 1 should be active. Since before checking, we have done 
rcu_assign_pointer() so at least 1 should be there. That is why that 
condition.

If it is more than 1, then this is not the first link which is going to 
come up and hence there is no need to set the flag again.

>> @@ -1486,7 +1488,10 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
>>   		if (old)
>>   			kfree_rcu(old, rcu_head);
>>   		RCU_INIT_POINTER(link->u.ap.beacon, NULL);
>> -		sdata->u.ap.active = false;
>> +
>> +		if (!ieee80211_num_beaconing_links(sdata))
>> +			sdata->u.ap.active = false;
> 
> == 0 maybe?
> 

Yeah can do. I prefer "!expr" over "expr == 0". Do you have any preference?

> Or maybe we should just save/restore the value instead?
> 
>>   	list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list)
>>   		netif_carrier_off(vlan->dev);
>>   
>> -	if (ieee80211_num_beaconing_links(sdata) <= 1)
> 
> Unrelated, but it looks like the VLAN netif_carrier_off() handling above
> is also wrong and should really go into this if block as well.
> 

Yeah MLO VLAN changes would do that? The previous change was focusing on 
the AP mode alone and I did not want to break anything in VLAN so did 
not touch it there.