| Message ID | 20250514164857.227540-6-quic_sarishar@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: cfg80211/mac80211: add support to handle per link statistics of multi-link station | expand |
On 5/15/2025 1:23 AM, Johannes Berg wrote: > On Wed, 2025-05-14 at 22:18 +0530, Sarika Sharma wrote: >> Currently, station_info structure is passed to fill station statistics >> from mac80211/drivers. After NL message send to user space for requested >> station statistics, memory for station statistics is freed in cfg80211. >> Therefore, memory allocation/free for link station statistics should >> also happen in cfg80211 only. >> >> Hence, allocate the memory for link_station structure for all >> possible links and free in cfg80211_sinfo_release_content(). > > I'll probably take a look myself tomorrow in the interest of getting all > things lined up for 6.16 quickly, but chances are you'll be at work a > couple of hours before me (and I'm not really all that awake now), so > pointer to this bug report from the bot: > > http://wifibot.sipsolutions.net/results/962902/14088291/build_allmodconfig_warn/summary > > Says some locking issue. > Oops! my bad, got it now. let me fix it and send new version. > > Also, is there any specific reason to want to allocate each link > individually? Why not allocate them all together in one bigger > allocation? Doesn't matter much though I guess. No, it doesn't matter much. Sure, will allocate them all together. > > johannes
On 5/15/2025 1:23 AM, Johannes Berg wrote: > On Wed, 2025-05-14 at 22:18 +0530, Sarika Sharma wrote: >> Currently, station_info structure is passed to fill station statistics >> from mac80211/drivers. After NL message send to user space for requested >> station statistics, memory for station statistics is freed in cfg80211. >> Therefore, memory allocation/free for link station statistics should >> also happen in cfg80211 only. >> >> Hence, allocate the memory for link_station structure for all >> possible links and free in cfg80211_sinfo_release_content(). > > I'll probably take a look myself tomorrow in the interest of getting all > things lined up for 6.16 quickly, but chances are you'll be at work a > couple of hours before me (and I'm not really all that awake now), so > pointer to this bug report from the bot: > > http://wifibot.sipsolutions.net/results/962902/14088291/build_allmodconfig_warn/summary > > Says some locking issue. > > > Also, is there any specific reason to want to allocate each link > individually? Why not allocate them all together in one bigger > allocation? Doesn't matter much though I guess. > Also, while implementing I think this way looks clean and easy to parse the links using link_id, also while allocating/de-allocating pertid for link level, at least to me. So I would keep this like this only? > johannes
On Thu, 2025-05-15 at 10:49 +0530, Sarika Sharma wrote: > > > > Also, is there any specific reason to want to allocate each link > > individually? Why not allocate them all together in one bigger > > allocation? Doesn't matter much though I guess. > > > > Also, while implementing I think this way looks clean and easy to parse > the links using link_id, also while allocating/de-allocating pertid for > link level, at least to me. > So I would keep this like this only? > OK sure, whatever? I like I said before, we can change these internal details later too. johannes
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 48096a23deb2..65a1a6511172 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -8579,7 +8579,16 @@ int cfg80211_sinfo_alloc_tid_stats(struct station_info *sinfo, gfp_t gfp); */ static inline void cfg80211_sinfo_release_content(struct station_info *sinfo) { + int link_id; + kfree(sinfo->pertid); + + for_each_valid_link(sinfo, link_id) { + if (sinfo->links[link_id]) { + kfree(sinfo->links[link_id]->pertid); + kfree(sinfo->links[link_id]); + } + } } /** diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index a5c760605ce9..392e7990f8f0 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -7360,7 +7360,7 @@ static int nl80211_dump_station(struct sk_buff *skb, struct wireless_dev *wdev; u8 mac_addr[ETH_ALEN]; int sta_idx = cb->args[2]; - int err; + int err, i; err = nl80211_prepare_wdev_dump(cb, &rdev, &wdev, NULL); if (err) @@ -7380,6 +7380,16 @@ static int nl80211_dump_station(struct sk_buff *skb, while (1) { memset(&sinfo, 0, sizeof(sinfo)); + + for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++) { + sinfo.links[i] = + kzalloc(sizeof(*sinfo.links[0]), GFP_KERNEL); + if (!sinfo.links[i]) { + cfg80211_sinfo_release_content(&sinfo); + return -ENOMEM; + } + } + err = rdev_dump_station(rdev, wdev->netdev, sta_idx, mac_addr, &sinfo); if (err == -ENOENT) @@ -7404,6 +7414,7 @@ static int nl80211_dump_station(struct sk_buff *skb, cb->args[2] = sta_idx; err = skb->len; out_err: + cfg80211_sinfo_release_content(&sinfo); wiphy_unlock(&rdev->wiphy); return err; @@ -7416,7 +7427,7 @@ static int nl80211_get_station(struct sk_buff *skb, struct genl_info *info) struct station_info sinfo; struct sk_buff *msg; u8 *mac_addr = NULL; - int err; + int err, i; memset(&sinfo, 0, sizeof(sinfo)); @@ -7428,9 +7439,19 @@ static int nl80211_get_station(struct sk_buff *skb, struct genl_info *info) if (!rdev->ops->get_station) return -EOPNOTSUPP; + for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++) { + sinfo.links[i] = kzalloc(sizeof(*sinfo.links[0]), GFP_KERNEL); + if (!sinfo.links[i]) { + cfg80211_sinfo_release_content(&sinfo); + return -ENOMEM; + } + } + err = rdev_get_station(rdev, dev, mac_addr, &sinfo); - if (err) + if (err) { + cfg80211_sinfo_release_content(&sinfo); return err; + } msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); if (!msg) {
Currently, station_info structure is passed to fill station statistics from mac80211/drivers. After NL message send to user space for requested station statistics, memory for station statistics is freed in cfg80211. Therefore, memory allocation/free for link station statistics should also happen in cfg80211 only. Hence, allocate the memory for link_station structure for all possible links and free in cfg80211_sinfo_release_content(). Signed-off-by: Sarika Sharma <quic_sarishar@quicinc.com> --- include/net/cfg80211.h | 9 +++++++++ net/wireless/nl80211.c | 27 ++++++++++++++++++++++++--- 2 files changed, 33 insertions(+), 3 deletions(-)