[PATCH wireless-next 3/5] wifi: cfg80211: Refactor nl80211_dump_station() to prepare for per-link stats

[Johannes Berg]

On Sun, 2026-06-07 at 23:29 +0530, P Praneesh wrote:
> 
> +++ b/net/wireless/nl80211.c
> @@ -39,6 +39,39 @@ static int nl80211_crypto_settings(struct cfg80211_registered_device *rdev,
>  
>  /* the netlink family */
>  static struct genl_family nl80211_fam;
> +/**
> + * enum nl80211_dump_station_phase - station dump fragmentation phases
> + * @NL80211_DUMP_STA_PHASE_AGGREGATED: send aggregated (MLO-combined) station
> + *	statistics for the station entry
> + * @NL80211_DUMP_STA_PHASE_PER_LINK: send per-link statistics for each active
> + *	MLO link of the station; only used when dump_link_stats is set
> + */
> +enum nl80211_dump_station_phase {
> +	NL80211_DUMP_STA_PHASE_AGGREGATED = 0,
> +	NL80211_DUMP_STA_PHASE_PER_LINK   = 1,
> +};

[snip]

probably nicer to move this stuff to closer to the dump code that needs
it; at the very least it's missing a blank line.

Also I think the whole 'phase' introduction etc. seems to more belong to
patch 4 instead of this one? We can still do the structure allocation
etc. here.

> +struct nl80211_dump_station_ctx {
> +	int sta_idx;
> +	enum nl80211_dump_station_phase phase;
> +	u8 mac_addr[ETH_ALEN];
> +	struct station_info sinfo;
> +};
> +
> +/**
> + * struct nl80211_dump_station_cb - state stored in netlink_callback::ctx
> + * @wiphy_idx: args[0] - wiphy index from nl80211_prepare_wdev_dump
> + * @wdev_id: args[1] - wdev identifier from nl80211_prepare_wdev_dump
> + * @ctx: args[2] - dump station context pointer
> + *
> + * args[0] and args[1] are reserved by nl80211_prepare_wdev_dump().
> + * The ctx pointer must live at args[2] to avoid corrupting those fields.
> + */
> +struct nl80211_dump_station_cb {
> +	long wiphy_idx;
> +	long wdev_id;
> +	struct nl80211_dump_station_ctx *ctx;
> +};

I'm not sure I'm happy with this - better to just use args[2] directly
for a pointer to struct nl80211_dump_station_ctx?

>  static int nl80211_dump_station(struct sk_buff *skb,
>  				struct netlink_callback *cb)
>  {
> -	struct station_info sinfo;
>  	struct cfg80211_registered_device *rdev;
>  	struct wireless_dev *wdev;
> -	u8 mac_addr[ETH_ALEN];
> -	int sta_idx = cb->args[2];
> -	bool sinfo_alloc = false;
> -	int err, i;
> +	struct nl80211_dump_station_cb *cb_data = (void *)cb->ctx;
> +	struct nl80211_dump_station_ctx *ctx = cb_data->ctx;

This doesn't really seem better than just doing

	struct nl80211_dump_station_ctx *ctx = cb->args[2];

given the overlay with generic code.

Alternatively we could change the _whole_ nl80211 code (as a separate
commit, perhaps even converting other things later) to have

struct nl80211_dump_cb {
	long wiphy_idx;
	long wdev_id;
	union {
		long wiphy_filter;
		int mpath_idx;
		int reg_idx;
		int bss_idx;
		int survey_idx;
		//...

		// and later add
		struct nl80211_dump_station_ctx *sta_ctx;
	};
};

or so, and then we don't have the args[0]/... problem at all.

But I'm not convinced mixing the structs with "must have this type at
this offset" etc. is a good idea if it's going to stay this way.

> +	/* Phase 0: dump aggregated station info */
> +	if (ctx->phase == NL80211_DUMP_STA_PHASE_AGGREGATED) {
> +		while (true) {
> +			memset(&ctx->sinfo, 0, sizeof(ctx->sinfo));
> 

if you have this memset, why do you need this one:

> +out_err_release:
> +	cfg80211_sinfo_release_content(&ctx->sinfo);
> +	memset(&ctx->sinfo, 0, sizeof(ctx->sinfo));

Seems a bit odd?

johannes