cfg80211: dynamically allocate per-tid stats for station info

From: Arend van Spriel <[email protected]>

With the addition of TXQ stats in the per-tid statistics the struct station_info grew significantly. This resulted in stack limit warnings like below:

CC [M] drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.o
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:
In function ‘brcmf_notify_connect_status_ap’:
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:5530:1:
error: the frame size of 1592 bytes is larger than 1024 bytes

This patch adds an allocation function that those who want to provide per-tid stats should use to allocate the tid array, ie. struct station_info::pertid.

userspace")

---
+ linux-wireless list

Johannes, Toke,

Here an alternative approach. Currently the only cfg80211-based driver providing per-tid stats is mac80211. This patch only changes mac80211 and the other driver can keep using stack allocation. Even mac80211 could if wanted, but I left that part as is.

I considered making the new helper more generic and allocate 'pertid' based on filled flags, ie.:

int cfg80211_sinfo_prepare(struct station_info *sinfo, gfp_t gfp)

{
	:
	if (sinfo->filled & BIT(NL80211_STA_INFO_TID_STATS)) {
		sinfo->pertid = kzalloc(..., gfp);
		if (!pertid)
			return -ENOMEM;
	}
	:
	return 0;
}

but decided to stick close to the issue.

Regards,
Arend

---
 include/net/cfg80211.h  |  9 ++++++++-
 net/mac80211/sta_info.c | 11 ++++++-----
 net/wireless/nl80211.c  |  4 +++-
 net/wireless/util.c     | 12 ++++++++++++
 4 files changed, 29 insertions(+), 7 deletions(-)

index 8db6071..784377a 100644
@@ -1230,7 +1230,7 @@ struct station_info {
 	u64 rx_beacon;
 	u64 rx_duration;
 	u8 rx_beacon_signal_avg;
-	struct cfg80211_tid_stats pertid[IEEE80211_NUM_TIDS + 1];
+	struct cfg80211_tid_stats *pertid;
 	s8 ack_signal;
 	s8 avg_ack_signal;
 };

@@ -5701,6 +5701,13 @@ void cfg80211_remain_on_channel_expired(struct

wireless_dev *wdev, u64 cookie,
struct ieee80211_channel *chan, gfp_t gfp);

+/**
+ * cfg80211_sinfo_alloc_tid_stats - allocate per-tid statistics.
+ *
+ * @sinfo: the station information
+ * @gfp: allocation flags
+ */
+int cfg80211_sinfo_alloc_tid_stats(struct station_info *sinfo, gfp_t gfp);

/**
* cfg80211_new_sta - notify userspace about station

index 43f34aa..25e8b15 100644
@@ -2233,11 +2233,12 @@ void sta_set_sinfo(struct sta_info *sta, struct

station_info *sinfo)
sinfo->filled |= BIT(NL80211_STA_INFO_RX_BITRATE);
}

- sinfo->filled |= BIT(NL80211_STA_INFO_TID_STATS);
- for (i = 0; i < IEEE80211_NUM_TIDS + 1; i++) {
- struct cfg80211_tid_stats *tidstats = &sinfo->pertid[i];
-
- sta_set_tidstats(sta, tidstats, i);
+ if (!cfg80211_sinfo_alloc_tid_stats(sinfo, GFP_KERNEL)) {
+ for (i = 0; i < IEEE80211_NUM_TIDS + 1; i++) {
+ struct cfg80211_tid_stats *tidstats = &sinfo->pertid[i];
+
+ sta_set_tidstats(sta, tidstats, i);
+ }
}

if (ieee80211_vif_is_mesh(&sdata->vif)) {

index f7715b8..e51cae7 100644
@@ -4663,7 +4663,7 @@ static int nl80211_send_station(struct sk_buff

*msg, u32 cmd, u32 portid,
int tid;

tidsattr = nla_nest_start(msg, NL80211_STA_INFO_TID_STATS);
- if (!tidsattr)
+ if (!tidsattr || !sinfo->pertid)
goto nla_put_failure;

for (tid = 0; tid < IEEE80211_NUM_TIDS + 1; tid++) {
@@ -4702,6 +4702,8 @@ static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid,
}

nla_nest_end(msg, tidsattr);
+
+ kfree(sinfo->pertid);
}

nla_nest_end(msg, sinfoattr);

index d112e9a..b956c23fe 100644
@@ -1750,6 +1750,18 @@ int cfg80211_get_station(struct net_device *dev,

const u8 *mac_addr,
}
EXPORT_SYMBOL(cfg80211_get_station);

+int cfg80211_sinfo_alloc_tid_stats(struct station_info *sinfo, gfp_t gfp) +{
+ sinfo->pertid = kcalloc(sizeof(*(sinfo->pertid)),
+ IEEE80211_NUM_TIDS + 1, gfp);
+ if (!sinfo->pertid)
+ return -ENOMEM;
+
+ sinfo->filled |= NL80211_STA_INFO_TID_STATS;
+ return 0;
+}
+EXPORT_SYMBOL(cfg80211_sinfo_alloc_tid_stats);
+
void cfg80211_free_nan_func(struct cfg80211_nan_func *f)
{
int i;

2.7.4

Hmm, yeah, that would work too. Though I worry that mixing dynamically and statically allocated fields is going to result in errors further down the road? I guess it also depends on whether struct station_info is likely to grow more for other reasons, in which case on-stack allocation needs to be changed anyway.

I'm fine with either approach; I'm happy to let it be up to our friendly maintainers to decide which approach they prefer :)

-Toke

I decided to take this and remove the BIT() thing entirely (you had a bug there anyway)

johannes

I figured you would drop this one. The error paths may need to do kfree here and there. Kalle took dynamic alloc fixes in driver already so my patch is not really needed right now.

Gr. AvS

Heh. I asked Kalle if he wanted to revert, but I decided that for the single user in mac80211 it wasn't worth the hassle of doing the dynamic allocations all over the place.

I'll look at the error paths I guess.

johannes