| Message ID | 5AF431F4.9060408@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
Arend van Spriel <arend.vanspriel@broadcom.com> writes: > From: Arend van Spriel <aspriel@gmail.com> > > 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. > > Cc: Toke Høiland-Jørgensen <toke@toke.dk> > Fixes: 52539ca89f36 ("cfg80211: Expose TXQ stats and parameters to > userspace") > Signed-off-by: Arend van Spriel <aspriel@gmail.com> > --- > + 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. 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
On Thu, 2018-05-10 at 13:50 +0200, Arend van Spriel wrote: > > 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 decided to take this and remove the BIT() thing entirely (you had a bug there anyway) johannes
On 5/18/2018 11:25 AM, Johannes Berg wrote: > On Thu, 2018-05-10 at 13:50 +0200, Arend van Spriel wrote: >> >> 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 decided to take this and remove the BIT() thing entirely (you had a > bug there anyway) 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
On Fri, 2018-05-18 at 11:37 +0200, Arend van Spriel wrote: > On 5/18/2018 11:25 AM, Johannes Berg wrote: > > On Thu, 2018-05-10 at 13:50 +0200, Arend van Spriel wrote: > > > > > > 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 decided to take this and remove the BIT() thing entirely (you had a > > bug there anyway) > > 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. 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
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 8db6071..784377a 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -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 diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 43f34aa..25e8b15 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -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)) { diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index f7715b8..e51cae7 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -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); diff --git a/net/wireless/util.c b/net/wireless/util.c index d112e9a..b956c23fe 100644 --- a/net/wireless/util.c +++ b/net/wireless/util.c @@ -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) {