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
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
