Received-SPF: pass (google.com: domain of linux-wireless+bounces-5994-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <f3032ce0-fe04-42e4-822b-a920ea6e0e3f@quicinc.com>
Date: Tue, 9 Apr 2024 14:36:31 +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>
 <15bc7705-5da9-4c56-aa45-81972bad5e01@quicinc.com>
 <98081f4e417cf20beea41288ca7b75235b8fe0e0.camel@sipsolutions.net>
From: Aditya Kumar Singh <quic_adisi@quicinc.com>
In-Reply-To: <98081f4e417cf20beea41288ca7b75235b8fe0e0.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

On 4/9/24 12:57, Johannes Berg wrote:
> On Tue, 2024-04-09 at 09:27 +0530, Aditya Kumar Singh wrote:
>> 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.
> 
> Hmm, OK, I guess that makes some sense. However, it's also completely
> pointless, since setting it =true when it's already =true doesn't really
> do anything. Adding the code seems to imply it should avoid setting it
> in some cases, which isn't actually the case.
> 
> Besides, doing the counting is almost certainly far more expensive than
> simply setting it to true when it's already true. Certainly the state
> should be =true after this function is called.
> 
> If you really think the extra write might be a problem (it isn't though)
> then you'd still want to write it as "if (!active) active=true" since
> that's actually checking the right thing. But ... that really wouldn't
> matter in all but the highest-performance code meant to deal with high
> (CPU/core) parallelism.
> 
> So this is just a long-winded way of saying: don't do that, just keep it
> unconditionally setting active.
> 

Well, that's true. Just to have symmetry in beaconing up and tear down I 
did like that but it seems costly as you said. I will instead go with 
!active thing as you suggested. Thanks for your inputs. Will send v2 
soon for review.

>>>> @@ -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?
> 
> I think for something that actually _counts_, like here, I'd (slightly)
> prefer ==0. It's obviously equivalent, but it seems more natural to
> write "is number of beaconing links equal to zero" rather than "is not
> number of beaconing links".
> 
> I may be influenced too much by Dan's thoughts on the matter ;-)
> 
> https://staticthinking.wordpress.com/2024/02/20/when-to-use-0/

Yeah in this case it seems reasonable to use "== 0" instead. Will do 
like this.

> 
>>> 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.
> 
> Sure sure, just drive-by observation.

:)