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

[Benjamin Berg]

Hi,

On Mon, 2024-03-18 at 17:49 +0530, Aditya Kumar Singh wrote:
> 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.

Yeah, it'll become much nicer. I had not realized you added struct
hostapd_mld when I wrote the mail.

So, with that, I am happy with it as-is :-)

Benjamin