[PATCH 01/16] hostapd: MLO: avoid usage of mld_id as user configuration

[Aditya Kumar Singh]

On 3/18/24 16:23, Benjamin Berg wrote:
...
>> diff --git a/src/ap/beacon.c b/src/ap/beacon.c
>> index e50f0a0c976e..9c028b5b72d6 100644
>> --- a/src/ap/beacon.c
>> +++ b/src/ap/beacon.c
>> @@ -960,7 +960,7 @@ static void
>> hostapd_fill_probe_resp_ml_params(struct hostapd_data *hapd,
>>   	 * We want to include the AP MLD ID in the response if it
>> was
>>   	 * included in the request.
>>   	 */
>> -	probed_mld_id = mld_id != -1 ? mld_id : hapd->conf->mld_id;
>> +	probed_mld_id = mld_id != -1 ? mld_id :
>> hostapd_get_mld_id(hapd);
> 
> We are not (yet) hitting these code paths. But, I think conceptually it
> would make more sense to get the probed hapd from the MLD ID. i.e.
> something like:
>       probed_hapd = mld_id == 0 ? hapd : hostapd_get_mbssid_hapd(hapd, mld_id);
> 

Yes correct. But fixes regarding MLO+MBSSID is in later part of our 
work. As you rightly said, this we are not hitting as of today. Also, 
this patch series tries to just support 1 MLD interface (as it was doing 
already before this) and at the same time just scale up certain 
structures/handling so that it can be leveraged when support for 
multiple MLD interfaces (co-hosted) is added later (this I'm working on 
right now and will probably send out series soon :)). So at that time 
this would be fixed properly.

>>   	for_each_mld_link(link, i, j, hapd->iface->interfaces,
>>   			  probed_mld_id) {
> 
> Then we need to change the iteration, but I think that makes sense
> anyway, see below.
> 

...			\
>> @@ -794,7 +798,7 @@ int hostapd_link_remove(struct hostapd_data
>> *hapd, u32 count);
>>   			for (_link
>> =					\
>>   			     (_ifaces)->iface[_iface_idx]-
>>> bss[_bss_idx]; \
>>   			    _link && _link->conf->mld_ap
>> &&		\
>> -				_link->conf->mld_id ==
>> _mld_id;		\
>> +				hostapd_get_mld_id(_link) ==
>> _mld_id;		\
>>   			    _link = NULL)
> 
> I think this part is slightly wrong, as the mld_id here would be local
> to the link, but it should to be local to the hapd from which we
> started the iteration.
> 
> Maybe we can update the macro and simplify it a bit at the same time.
> We always have an hapd instance anyway, so passing the interfaces
> explicitly does not really make sense. So, we can just start with an
> hapd instance, and iterate all links (including itself). Something
> like:
> 
> #define for_each_mld_link(_hapd, _link, _bss_idx, _iface_idx)		\
> 	for (_iface_idx = 0;						\
> 	     _iface_idx < (_hapd)->iface->interfaces)->count;		\
> 	     _iface_idx++)						\
> 		for (_bss_idx = 0;					\
> 		     _bss_idx <						\
> 			(_ifaces)->iface[_iface_idx]->num_bss;		\
> 		     _bss_idx++)					\
> 			for (_link =					\
> 			     (_hapd)->iface->interfaces->		\
> 				iface[_iface_idx]->bss[_bss_idx]; 	\
> 			    _link &&					\
> 				hostapd_is_ml_partner(_link, _hapd);	\
> 			    _link = NULL)
> 
> Benjamin

Yeah that makes sense. Thanks for the input! But as I said above, in the 
next series, actually this logic will be replaced with another one. 
Hence did not change much here.

Now (after this series) since we have all affiliated links tied up to a 
common MLD structure via a list member, there is no need of these many 
arguments for this macro. We can just use dl_list_for_each() and iterate 
over all affiliated links.