> -----Original Message-----
> From: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> Sent: Friday, March 25, 2022 6:27 PM
> To: Sriram R (QUIC) <quic_srirrama@xxxxxxxxxxx>; linux-
> wireless@xxxxxxxxxxxxxxx
> Cc: vkthiagu@xxxxxxxxx; Aaron Komisar <akomisar@xxxxxxxxxxxxx>; Jeff
> Johnson (QUIC) <quic_jjohnson@xxxxxxxxxxx>; Jouni Malinen <j@xxxxx>
> Subject: Re: [RFC] mac80211: prepare sta handling for MLO support
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
>
Hi Johannes,
<snip>
>
> >
> > +#define MAX_STA_LINKS 16
>
> Isn't that 15? 15 is reserved, I believe?
Yeah right, I'll correct in next revision.
>
> > +struct ieee80211_link_sta {
> > + u8 addr[ETH_ALEN];
> > +
> > + struct ieee80211_sta_ht_cap ht_cap;
> > + struct ieee80211_sta_vht_cap vht_cap;
> > + struct ieee80211_sta_he_cap he_cap;
> > + struct ieee80211_he_6ghz_capa he_6ghz_capa;
> > +
> > + u8 rx_nss;
> > + enum ieee80211_sta_rx_bandwidth bandwidth;
> > + struct ieee80211_sta_txpwr txpwr;
>
> That looks mostly fine.
>
> I think you missed at least
> - supp_rates (clearly, the list of legacy rates)
> - rates (rate control must be done separately per band)
>
> and I'm not sure about the A-MSDU related ones?
>
Sure, the contents of the link sta (link_sta_info or ieee80211_link_sta) needs to be definitely revisited.
This RFC was to mainly conclude on the approach of splitting the sta_info, ieee80211_sta structs
and how we access the link sta related info within them. I'll relook into the contents and add any
link related info to link structs in next version and we can review that part again if any more info
requires to be moved.
> > + bool multi_link_sta;
> > + u8 num_sta_links;
>
> Why do we need the num_sta_links? We can always count which link[] entries
> are non-NULL?
Correct I don’t see any use having this counter, I'll remove num_sta_links.
>
> > + struct ieee80211_link_sta *link[MAX_STA_LINKS];
>
> I guess we index this by link-ID, which may be assigned by the AP and we might
> end up using e.g. 1 and 14, and then things are simpler but we might have a lot
> of NULL pointers? I'm fine with that, just asking if I'm interpreting it correctly.
Yes that’s correct.
>
> > +static int sta_link_alloc(struct ieee80211_sub_if_data *sdata,
> > + struct sta_info *sta, const u8 *addr,
> > + bool update, gfp_t gfp)
>
> no point passing gfp
Sure, I'll remove it.
<snip>
> > + lsinfo = kzalloc(sizeof(*lsinfo), gfp);
> > + if (!lsinfo)
> > + goto free;
> > +
> > + lsta = kzalloc(sizeof(*lsta), gfp);
> > + if (!lsta)
> > + goto free;
>
> I did start wondering if we wouldn't want to move struct link_sta_info to
> mac80211.h as well, though I'm not sure what else we'd have to move, and not
> have to allocate all of these separately?
I didn’t get this point correctly, Do you mean to merge link_sta_info and ieee80211_link_sta structs
so as to avoid these two allocs?
>
> Though if we follow them through pointers, we can still allocate them in the
> same memory chunk (just add the sizes).
Do you mean something like,
lsinfo = kzalloc(sizeof(*lsinfo) + sizeof(*lsta), gfp);
lsta = (u8 *)lsinfo + sizeof(*lsinfo);
This seems fine I guess and helps to do away with the second kzalloc(). Can we go with this?
>
> Not sure we need to optimise anything here though.
>
> Or maybe in addition or instead we should allocate an *array* of links?
> But of course only however many we actually need, regardless of which ones
> are actually active.
The array of link pointers are already allocated as part of struct ieee80211_sta and struct sta_info, right?
Did I misunderstood?
>
> OTOH, maybe we should keep it this way, and then we could embed the 0th link
> instead? Then we wouldn't have
>
> sta->link[0]->rx_nss
>
> (picking up your example), but
>
> sta->deflink.rx_nss
>
> which saves a pointer dereference. We'd also do
>
> sta->link[0] = &sta->deflink;
>
> of course. This removes the MLO overhead in the *drivers* at least, for those
> drivers not supporting MLO at all.
>
> We couldn't do this in mac80211 though, only in the mac80211.h level, but still,
> might be worthwhile?
This 'deflink' approach looks very neat, Thanks. I'll update in next version.
>
> You say in the commit message:
>
> > Stats are accumulated per link sta and finally aggregated if mld sta
> > stats is required.
>
> but the changes here
>
> > static struct ieee80211_sta_rx_stats * sta_get_last_rx_stats(struct
> > sta_info *sta) {
> > - struct ieee80211_sta_rx_stats *stats = &sta->rx_stats;
> > + struct ieee80211_sta_rx_stats *stats = &sta->link[0]->rx_stats;
>
> don't seem like you did that yet? Which again, is fine, just wondering
> if I'm missing something.
Yes, this change would be part of MLO support changes.
>
>
> Which, btw, makes this patch fairly much mechanical, so maybe put the
> spatch in the commit message? That way can re-apply it if there are
> conflicts, or such.
>
> > + * TODO Move other link params from sta_info as required for MLD
> operation
>
> :)
>
> So looks good I think! Let's get an spatch to do it in both mac80211 and
> the drivers, and start splitting things.
Sure, let me prepare it for the next version.
>
> Unless you see any major problem with it, I think I'd really prefer to
> have a "deflink" embedded and instead of changing
>
> sta->something
> to
> sta->links[0]->something
>
> make all the changes be like
>
> sta->something
> to
> sta->deflink.something
>
> which of course does nothing for MLO yet, but it's easier to grep for,
> and we'll surely always have a single link, so in the non-MLO cases we
> need not have all the extra allocations around etc., even if in mac80211
> we end up accessing through a pointer.
Sure, I'll update. Thanks for the quick review.
Regards,
Sriram.R
