| Message ID | 1456927710-32073-1-git-send-email-mohammed@qca.qualcomm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
On Wed, 2016-03-02 at 19:38 +0530, Mohammed Shafi Shajakhan wrote: > From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com> > > Add support for new netlink attribute 'NL80211_STA_INFO_RX_DURATION' > This flag will be set when drivers can fill rx_duration ( approximate > total air time(usecs) for unicast data/management frames from the > connected client) via 'drv_sta_statistics' callback So, what's that useful for? Are you really sure you mean "unicast"? What if the station is the AP? Why wouldn't multicast frames it transmitted be accounted for? > Also make sta_info flags 'filled' as 64 bit to accomodate for new typo: accommodate johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Johannes, thanks for taking time to review this. On Thu, Mar 03, 2016 at 04:46:23PM +0100, Johannes Berg wrote: > On Wed, 2016-03-02 at 19:38 +0530, Mohammed Shafi Shajakhan wrote: > > From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com> > > > > Add support for new netlink attribute 'NL80211_STA_INFO_RX_DURATION' > > This flag will be set when drivers can fill rx_duration ( approximate > > total air time(usecs) for unicast data/management frames from the > > connected client) via 'drv_sta_statistics' callback > > So, what's that useful for? [shafi] This is part of the Per Station statistics requirement, the information from 'iw dev wlan#N station dump' which has rx_duration field will be used by application assesing the statistics /performance of various clients connected to our AP. We will plot a graph based on this information (assesing the rx time spent by AP for various clients). > > Are you really sure you mean "unicast"? What if the station is the AP? > Why wouldn't multicast frames it transmitted be accounted for? [shafi] This is based on the implementation by the driver/firmware. We are focused on the AP side so unicast frames, if you are ok with this change, i can change the documentation to be specific (generic for all the drivers) something like implementation specific (please advise) > > > Also make sta_info flags 'filled' as 64 bit to accomodate for new > > typo: accommodate [shafi] will fix this regards, shafi -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2016-03-03 at 21:30 +0530, Mohammed Shafi Shajakhan wrote: > [shafi] This is part of the Per Station statistics requirement, Heh. Whose requirements? :) > the information from 'iw dev wlan#N station dump' which has > rx_duration > field will be used by application assesing the statistics > /performance of various clients connected to our AP. We will plot > a graph based on this information (assesing the rx time spent by > AP for various clients). > > > > > Are you really sure you mean "unicast"? What if the station is the > > AP? Why wouldn't multicast frames it transmitted be accounted for? > > [shafi] This is based on the implementation by the driver/firmware. > We are focused on the AP side so unicast frames, if you are ok with > this change, i can change the documentation to be specific (generic > for all the drivers) something like implementation specific (please > advise) Ok, but for AP side I don't see the difference between "unicast" and "all frames", since all frames from that station should be unicast? Apart, perhaps, from some management frames the station might send outside the BSS, but that's not likely to matter much? IOW, why not specify all frames, and let this make some sense for other interface modes? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi johannes, On Thu, Mar 03, 2016 at 05:07:51PM +0100, Johannes Berg wrote: > On Thu, 2016-03-03 at 21:30 +0530, Mohammed Shafi Shajakhan wrote: > > > [shafi] This is part of the Per Station statistics requirement, > > Heh. Whose requirements? :) [shafi] We have this as part of ath10k debugfs based on the end user (or) a customer requirement and i think its make more sense to have this common across all wifi drivers rather thank simply dumping it in ath10k debugfs. I think its similar to 'ieee80211_frame_duration'(?) in mac80211 but for drivers like ath10k where this is calculated in firmware for rx and implementation specific. > > > the information from 'iw dev wlan#N station dump' which has > > rx_duration > > field will be used by application assesing the statistics > > /performance of various clients connected to our AP. We will plot > > a graph based on this information (assesing the rx time spent by > > AP for various clients). > > > > > > > > Are you really sure you mean "unicast"? What if the station is the > > > AP? Why wouldn't multicast frames it transmitted be accounted for? > > > > [shafi] This is based on the implementation by the driver/firmware. > > We are focused on the AP side so unicast frames, if you are ok with > > this change, i can change the documentation to be specific (generic > > for all the drivers) something like implementation specific (please > > advise) > > Ok, but for AP side I don't see the difference between "unicast" and > "all frames", since all frames from that station should be unicast? > Apart, perhaps, from some management frames the station might send > outside the BSS, but that's not likely to matter much? [shafi] exactly, I can change the documentation as you wish and this is implementation specific, and we have implemented like this in ath10k targeting AP mode. > > IOW, why not specify all frames, and let this make some sense for other > interface modes? > [shafi] Agreed. regards, shafi -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Johannes, https://groups.google.com/a/chromium.org/forum/#!msg/chromium-os-reviews/lDzKxgl6fIE/H5GQfMcuBwAJ for more information regarding the request for implementation. I will send a v2 re-phrasing that it is implementation specific. regards, shafi On Thu, Mar 03, 2016 at 09:50:36PM +0530, Mohammed Shafi Shajakhan wrote: > Hi johannes, > > On Thu, Mar 03, 2016 at 05:07:51PM +0100, Johannes Berg wrote: > > On Thu, 2016-03-03 at 21:30 +0530, Mohammed Shafi Shajakhan wrote: > > > > > [shafi] This is part of the Per Station statistics requirement, > > > > Heh. Whose requirements? :) > > [shafi] We have this as part of ath10k debugfs based on the end user (or) a > customer requirement and i think its make more sense to have this common across all > wifi drivers rather thank simply dumping it in ath10k debugfs. I think its > similar to 'ieee80211_frame_duration'(?) in mac80211 but for drivers like ath10k > where this is calculated in firmware for rx and implementation specific. > > > > > > the information from 'iw dev wlan#N station dump' which has > > > rx_duration > > > field will be used by application assesing the statistics > > > /performance of various clients connected to our AP. We will plot > > > a graph based on this information (assesing the rx time spent by > > > AP for various clients). > > > > > > > > > > > Are you really sure you mean "unicast"? What if the station is the > > > > AP? Why wouldn't multicast frames it transmitted be accounted for? > > > > > > [shafi] This is based on the implementation by the driver/firmware. > > > We are focused on the AP side so unicast frames, if you are ok with > > > this change, i can change the documentation to be specific (generic > > > for all the drivers) something like implementation specific (please > > > advise) > > > > Ok, but for AP side I don't see the difference between "unicast" and > > "all frames", since all frames from that station should be unicast? > > Apart, perhaps, from some management frames the station might send > > outside the BSS, but that's not likely to matter much? > > [shafi] exactly, I can change the documentation as you wish and > this is implementation specific, and we have implemented like this in ath10k > targeting AP mode. > > > > > IOW, why not specify all frames, and let this make some sense for other > > interface modes? > > > [shafi] Agreed. > > regards, > shafi > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 9bcaaf7..b8bf024 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -1060,11 +1060,13 @@ struct cfg80211_tid_stats { * @rx_beacon: number of beacons received from this peer * @rx_beacon_signal_avg: signal strength average (in dBm) for beacons received * from this peer + * @rx_duration: approximate total air time(usecs) for unicast data/management + * frames from the connected client * @pertid: per-TID statistics, see &struct cfg80211_tid_stats, using the last * (IEEE80211_NUM_TIDS) index for MSDUs not encapsulated in QoS-MPDUs. */ struct station_info { - u32 filled; + u64 filled; u32 connected_time; u32 inactive_time; u64 rx_bytes; @@ -1103,6 +1105,7 @@ struct station_info { u32 expected_throughput; u64 rx_beacon; + u64 rx_duration; u8 rx_beacon_signal_avg; struct cfg80211_tid_stats pertid[IEEE80211_NUM_TIDS + 1]; }; diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index 5b7b5eb..1220a7b 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -2464,6 +2464,8 @@ enum nl80211_sta_bss_param { * TID+1 and the special TID 16 (i.e. value 17) is used for non-QoS frames; * each one of those is again nested with &enum nl80211_tid_stats * attributes carrying the actual values. + * @NL80211_STA_INFO_RX_DURATION: approximate total air time(usecs) for unicast + * data/management frames from the connected client * @__NL80211_STA_INFO_AFTER_LAST: internal * @NL80211_STA_INFO_MAX: highest possible station info attribute */ @@ -2500,6 +2502,7 @@ enum nl80211_sta_info { NL80211_STA_INFO_BEACON_RX, NL80211_STA_INFO_BEACON_SIGNAL_AVG, NL80211_STA_INFO_TID_STATS, + NL80211_STA_INFO_RX_DURATION, /* keep last */ __NL80211_STA_INFO_AFTER_LAST, diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index d4786f2..a81c016 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -3719,7 +3719,7 @@ static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid, goto nla_put_failure; #define PUT_SINFO(attr, memb, type) do { \ - if (sinfo->filled & BIT(NL80211_STA_INFO_ ## attr) && \ + if (sinfo->filled & (1ULL << NL80211_STA_INFO_ ## attr) && \ nla_put_ ## type(msg, NL80211_STA_INFO_ ## attr, \ sinfo->memb)) \ goto nla_put_failure; \ @@ -3745,6 +3745,7 @@ static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid, PUT_SINFO(LLID, llid, u16); PUT_SINFO(PLID, plid, u16); PUT_SINFO(PLINK_STATE, plink_state, u8); + PUT_SINFO(RX_DURATION, rx_duration, u64); switch (rdev->wiphy.signal_type) { case CFG80211_SIGNAL_TYPE_MBM: