On 5/8/2025 2:47 PM, Johannes Berg wrote:
On Thu, 2025-05-08 at 14:34 +0530, Sarika Sharma wrote:
But to be honest I completely forgot all the history here. Looks like
your initial patches couldn't even build, so I can't compare well with
what it would've looked like before.
Oops! if require can send new version V7, in building state(as already
prepared the patch as per last comments)?
No no, I meant *initial* patches, I was just trying to look at v1/v2/...
to compare how much driver changes they all needed, but in the earlier
versions you just skipped the driver changes entirely, so I couldn't
compare :)
Yes, that patches send as RFC and avoided driver changes, to get review
on design.
As far as I can tell this version builds fine, the robot seemed happy.
:-)
We can also prohibit filling entries that make no sense for multi-link
statistics when provided on a global level, though honestly I feel like
for most of them we should actually provide some value, even if tx/rx
rate is just the best rate of all the links, or something like that?
Yes, but then document for station_info structure needed to be updated here.
As for some of fields it will be similar for both non-Ml and MLO case.
But some of fields like signal,rates, etc, meaning will be different for
non-ML and MLO stats.
Agree, and maybe some of the fields should just be prohibited for MLO
(like signal/rate) until we figure out what semantics we want for them
for MLO. But we can document and implement that, I'd think?
Sure.
This would also not yet require any drivers or mac80211 to change at
all, since it's just adding a new capability.
Oh! Do you mean not add all the things now at link level? and add
afterwards as per requirement/ while adding new capabilities?
or add as of now and let driver/mac80211 filling those capabilities
handle it later part on.
I just mean wrt. the patch sequence.
As it is now you're making a lot of changes to drivers/mac80211 during
the cfg80211 patches (specifically patch 4), but then in some sense you
*undo* the need for those later (after patch 7 a lot of the driver
changes aren't _really_ needed, particularly for non-MLO drivers).
That seems harder than it should be. We later have a lot of the fields
back that were modified earlier, so I'm suggesting to just not do it
that way, but rather keep _all_ the fields, then you don't have any
driver changes.
Then add all the fields for each link, and the nl80211 code, aggregation
code, etc.
And _then_ you can start moving things over much more easily, like
mac80211 would start providing per-link stats for all the fields, and
accumulated stats about removed links in the original "non-MLO-
statistics" fields (regardless of whether they're reported that way
because the connection isn't MLO, or because the driver wasn't updated.)
Yes, sure. I will start this way.
Oh, in terms of allocation - the struct isn't huge now, do we even
care about simply adding 15 links? It still stays well below a
page as far as I can tell ... keep it simpler for now?
Okay! but although many fields are more applicable at link level. So
isn't keeping the pointer is better then keeping array[15](seeing error
with array [15], mentioned below)?
Right ... hm. I actually thought this struct was already not on the
stack, not sure why. That's why I was talking about the size of
allocations etc.
Alright, since we already have cfg80211_sinfo_release_content() it seems
easier now to actually allocate the link stats dynamically, so instead
of what I just said in my other email:
struct link_station_info links[15];
we'd have
struct link_station_info *links;
and then just make sure we allocate a 15 links array, and free it in
cfg80211_sinfo_release_content().
Yes, can have that as struct link_station_info *links[15];
For memory allocation here, we have 2 approaches.
1. allocate in cfg80211 during get_station/dump_station call (by adding
the wiphy flag introduced in patch-6 or allocate the memory everytime ?)
2. Allocate in mac80211, in sta_set_sinfo() during station_info
structure filling?
That actually it looks the same for the users, e.g. "sinfo.links[3]".
Yes.
johannes
