[RESEND PATCH] cfg80211: dynamically allocate per-tid stats for station info

[RESEND PATCH] cfg80211: dynamically allocate per-tid stats for station info

[Arend van Spriel]

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.

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(-)

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)
  {
  	int i;
-- 
2.7.4

Re: [RESEND PATCH] cfg80211: dynamically allocate per-tid stats for station info

[Toke Høiland-Jørgensen]

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 =E2=80=98brcmf_notify_connect_status_ap=E2=80=99:
> 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=C3=B8iland-J=C3=B8rgensen <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

Re: [RESEND PATCH] cfg80211: dynamically allocate per-tid stats for station info

[Johannes Berg]

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

Re: [RESEND PATCH] cfg80211: dynamically allocate per-tid stats for station info

[Arend van Spriel]

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

Re: [RESEND PATCH] cfg80211: dynamically allocate per-tid stats for station info

[Johannes Berg]

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