[PATCH wireless-next 4/5] wifi: cfg80211: Fragment per-link station stats in nl80211_dump_station()

[Johannes Berg]

On Tue, 2026-06-09 at 00:31 +0530, Praneesh P wrote:
> 
> > That seems really odd? why even bother going into the whole thing if
> > it's invalid? Also, doesn't that ENOENT get propagated all the way and
> > it aborts? I guess it does but it should never happen because of
> > valid_links? Still seems a bit odd.
> Both the link_sinfo null check and is_valid_ether_addr() check
> will be moved to the top of the function, before any header or
> nest construction. The null check for sinfo->links[link_idx] is
> already guarded upstream by the valid_links bitmask so it should
> never be NULL in practice, I'll add a WARN_ON_ONCE and skip rather
> than returning -ENOENT, which would otherwise abort the entire dump.
> Will it be okay ?

I guess? Anyway the structure will change based on our other discussion
below.

> ok, I will try to align with this suggested model. I'll refactor so the 
> outer loop
> produces exactly one netlink message per iteration, with header
> construction in one place:
> 
> while (true) {
>      if (ctx->phase == PHASE_AGGREGATED) {
>          /* fetch sinfo from driver once per station */
>          memset(&ctx->sinfo, ...);
>          /* allocate links[], call rdev_dump_station() */
>          /* apply cfg80211_sta_set_mld_sinfo() if needed */
>      }
> 
>      hdr = nl80211hdr_put(msg, ...);
>      /* common attrs: ifindex, wdev, mac, generation */
>      nla_put_u32(msg, NL80211_ATTR_IFINDEX, ...);
>      nla_put_u64_64bit(msg, NL80211_ATTR_WDEV, ...);
>      nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, ctx->mac_addr);
>      nla_put_u32(msg, NL80211_ATTR_GENERATION, ...);
> 
>      switch (ctx->phase) {
>      case PHASE_AGGREGATED:
>          nl80211_put_sta_info_common(msg, rdev, &ctx->sinfo);
>          ctx->phase = PHASE_PER_LINK;
>          ctx->link_idx = first_valid_link();
>          break;
>      case PHASE_PER_LINK:
>          nl80211_put_link_station(msg, rdev, &ctx->sinfo, ctx->link_idx);
>          advance ctx->link_idx to next valid link;
>          if (no more links) {
>              cfg80211_sinfo_release_content(&ctx->sinfo);
>              ctx->sta_idx++;
>              ctx->phase = PHASE_AGGREGATED;
>          }
>          break;
>      }
> 
>      genlmsg_end(msg, hdr);   /* or genlmsg_cancel() on EMSGSIZE 
> break-out */
> }
> 
> 
> This eliminates the duplicated nla_put_u32/u64/MAC header blocks
> that currently exist in both nl80211_send_accumulated_station()
> and nl80211_send_link_station(), and makes the EMSGSIZE bail-out
> uniform since we return before committing the message.

Sounds good!

> should the rdev_dump_station() / sinfo fill stay outside the
> per-message loop (as a separate if (phase == AGGREGATED) guard
> at the top), or is it preferable to fold it into the switch
> case? I've sketched the former above since fetching sinfo is
> logically per-station, not per-message. I'm also ok to adjust
> if you'd prefer it inside the switch ?

I think it has to stay above otherwise you can't unify pushing the MAC
address to the message? And then perhaps you could even unify the nest
push (i.e. NL80211_ATTR_STA_INFO nest start), but I'm not sure how that
interacts with other code and it's not really important. The header is
more important since it could change in the future.

johannes