On Fri, 2022-03-25 at 13:18 +0530, Sriram R wrote:
> [snip]
I think this looks good so far!
> RFC Note:
> This patch breaks build since it requires changes in all
> mac80211 based drivers. Only ath11k driver changes are added
> here for reference. All driver changes will be covered once
> we arrive at final design.
> Basically for existing drivers, the link params that are moved
> to a new struct within ieee80211_sta will be accessed via first link.
> Ex. 'sta->rx_nss' will change to 'sta->link[0]->rx_nss'
Right. These are large but fairly mechanical changes as long as there's
no MLO support, I guess most could be done with a carefully crafted
spatch.
>
> +#define MAX_STA_LINKS 16
Isn't that 15? 15 is reserved, I believe?
> +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?
> + 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?
> + 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.
> +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
> +{
> + struct ieee80211_local *local = sdata->local;
> + struct ieee80211_hw *hw = &local->hw;
> + struct link_sta_info *lsinfo = NULL;
> + struct ieee80211_link_sta *lsta = NULL;
> + u8 link_id, i;
> +
> + spin_lock_bh(&sta->lock);
you spin_lock here so you need GFP_ATOMIC always
> + /* TODO This is temporary, the link_id will be assigned based on sta vif */
> + link_id = sta->num_sta_links;
> +
> + if (update) {
> + if (!sta->link[link_id] || !sta->sta.link[link_id])
> + return -EINVAL;
> + ether_addr_copy(sta->link[link_id]->addr, addr);
> + ether_addr_copy(sta->sta.link[link_id]->addr, addr);
> + goto out;
> + }
> +
> + 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?
Though if we follow them through pointers, we can still allocate them in
the same memory chunk (just add the sizes).
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.
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?
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.
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.
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.
Pivoting a bit - I meant to work on the netdev/wdev stuff again today,
but got side-tracked into the locking issue... but nonetheless I've been
thinking about it, and looking at this, I'm starting to wonder if we
shouldn't do the same with interfaces. The changes are large, but
basically mechanical, and then we can incrementally build on them to
differentiate the link[] in different areas, which makes it nicer to
work on things in parallel?
