The "Expected throughput" towards a generic wireless peer is
a value that can be used by other kernel components to
achieve different goals.
In particular we have 802.11s and batman-adv which will
iuse this estimation to compute their throughput based
metric. Therefore with this patchset I wanted to export
this result and make it available for future use.
Changes included in this patchset:
- add the new get_expected_throughput() RC API
- export the result of such API via cfg80211::get_station()
- implement the get_expected_throughput() RC API for minstrel(_ht)
For what concerns Minstrel_HT I decided to directly re-use the
throughput value computed by the algorithm itself with
minstrel_ht_calc_tp() for max_tp_rate.
For the legacy Minstrel instead I computed the expected
throughput as the product of the max_tp bitrate multplied
by its probability of success.
I am not entirely sure about passing sband as third parameter
to get_expected_throughput()..maybe somebody can suggest a
better solution.
Cheers,
Antonio Quartulli (6):
cfg80211: export expected throughput through get_station()
mac80211: add new RC API to retrieve expected throughput
mac80211: export expected throughput in set_sta_info()
mac80211: minstrel - implement get_expected_throughput() API
mac80211: minstrel_ht - implement get_expected_throughput() API
cfg80211: implement cfg80211_get_station cfg80211 API
include/net/cfg80211.h | 74 +++++++++++++++++++++++---------------
include/net/mac80211.h | 3 ++
net/mac80211/cfg.c | 17 +++++++++
net/mac80211/rc80211_minstrel.c | 15 ++++++++
net/mac80211/rc80211_minstrel_ht.c | 19 ++++++++++
net/wireless/util.c | 18 ++++++++++
6 files changed, 118 insertions(+), 28 deletions(-)
--
1.8.3.2
From: Antonio Quartulli <[email protected]>
Implement and export the new cfg80211_get_station() API.
This utility can be used by other kernel modules to obtain
detailed information about a given wireless station.
It will be in particular useful to batman-adv which will
implement a wireless rate based metric.
Signed-off-by: Antonio Quartulli <[email protected]>
---
include/net/cfg80211.h | 11 +++++++++++
net/wireless/util.c | 18 ++++++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 4c8ebe9..45062aa 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1065,6 +1065,17 @@ struct station_info {
};
/**
+ * cfg80211_get_station - retrieve information about a given station
+ * @dev: the device where the station is supposed to be connected to
+ * @mac_addr: the mac address of the station of interest
+ * @sinfo: pointer to the structure to fill with the information
+ *
+ * Returns 0 on success or a negative error code otherwise.
+ */
+int cfg80211_get_station(struct net_device *dev, u8 *mac_addr,
+ struct station_info *sinfo);
+
+/**
* enum monitor_flags - monitor flags
*
* Monitor interface configuration flags. Note that these must be the bits
diff --git a/net/wireless/util.c b/net/wireless/util.c
index c5d0208..c691cf8 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1482,6 +1482,24 @@ unsigned int ieee80211_get_num_supported_channels(struct wiphy *wiphy)
}
EXPORT_SYMBOL(ieee80211_get_num_supported_channels);
+int cfg80211_get_station(struct net_device *dev, u8 *mac_addr,
+ struct station_info *sinfo)
+{
+ struct cfg80211_registered_device *rdev;
+ struct wireless_dev *wdev;
+
+ wdev = dev->ieee80211_ptr;
+ if (!wdev)
+ return -EOPNOTSUPP;
+
+ rdev = wiphy_to_dev(wdev->wiphy);
+ if (!rdev->ops->get_station)
+ return -EOPNOTSUPP;
+
+ return rdev_get_station(rdev, dev, mac_addr, sinfo);
+}
+EXPORT_SYMBOL(cfg80211_get_station);
+
/* See IEEE 802.1H for LLC/SNAP encapsulation/decapsulation */
/* Ethernet-II snap header (RFC1042 for most EtherTypes) */
const unsigned char rfc1042_header[] __aligned(2) =
--
1.8.3.2
On 30/03/14 22:35, Antonio Quartulli wrote:
> +static u32
> +minstrel_get_expected_throughput(void *priv, void *priv_sta,
> + struct ieee80211_supported_band *sband)
> +{
> + struct minstrel_sta_info *mi = priv_sta;
> + int idx = mi->max_tp_rate[0];
> + u32 bitrate, ret;
> +
> + bitrate = sband->bitrates[mi->r[idx].rix].bitrate;
> + ret = bitrate * MINSTREL_TRUNC(mi->r[idx].probability * 1000) / 1000;
Thanks to the point raised by Andrew Lunn I just realised that this
value is expressed in Mbps/10 and so should be converted to Mbps/100
before being returned (assuming we agree on using this unit).
Cheers,
--
Antonio Quartulli
From: Antonio Quartulli <[email protected]>
Users may need information about the expected throughput
towards a given peer computed by the RC algorithm.
Export such value through the get_station() API.
This information is useful to the batman-adv module which
will use it for its new metric computation.
Signed-off-by: Antonio Quartulli <[email protected]>
---
include/net/cfg80211.h | 63 ++++++++++++++++++++++++++++----------------------
1 file changed, 35 insertions(+), 28 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index fb8afce..4c8ebe9 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -862,36 +862,38 @@ int cfg80211_check_station_change(struct wiphy *wiphy,
* @STATION_INFO_NONPEER_PM: @nonpeer_pm filled
* @STATION_INFO_CHAIN_SIGNAL: @chain_signal filled
* @STATION_INFO_CHAIN_SIGNAL_AVG: @chain_signal_avg filled
+ * @STATION_INFO_EXPECTED_THROUGHPUT: @expected_throughput filled
*/
enum station_info_flags {
- STATION_INFO_INACTIVE_TIME = 1<<0,
- STATION_INFO_RX_BYTES = 1<<1,
- STATION_INFO_TX_BYTES = 1<<2,
- STATION_INFO_LLID = 1<<3,
- STATION_INFO_PLID = 1<<4,
- STATION_INFO_PLINK_STATE = 1<<5,
- STATION_INFO_SIGNAL = 1<<6,
- STATION_INFO_TX_BITRATE = 1<<7,
- STATION_INFO_RX_PACKETS = 1<<8,
- STATION_INFO_TX_PACKETS = 1<<9,
- STATION_INFO_TX_RETRIES = 1<<10,
- STATION_INFO_TX_FAILED = 1<<11,
- STATION_INFO_RX_DROP_MISC = 1<<12,
- STATION_INFO_SIGNAL_AVG = 1<<13,
- STATION_INFO_RX_BITRATE = 1<<14,
- STATION_INFO_BSS_PARAM = 1<<15,
- STATION_INFO_CONNECTED_TIME = 1<<16,
- STATION_INFO_ASSOC_REQ_IES = 1<<17,
- STATION_INFO_STA_FLAGS = 1<<18,
- STATION_INFO_BEACON_LOSS_COUNT = 1<<19,
- STATION_INFO_T_OFFSET = 1<<20,
- STATION_INFO_LOCAL_PM = 1<<21,
- STATION_INFO_PEER_PM = 1<<22,
- STATION_INFO_NONPEER_PM = 1<<23,
- STATION_INFO_RX_BYTES64 = 1<<24,
- STATION_INFO_TX_BYTES64 = 1<<25,
- STATION_INFO_CHAIN_SIGNAL = 1<<26,
- STATION_INFO_CHAIN_SIGNAL_AVG = 1<<27,
+ STATION_INFO_INACTIVE_TIME = 1<<0,
+ STATION_INFO_RX_BYTES = 1<<1,
+ STATION_INFO_TX_BYTES = 1<<2,
+ STATION_INFO_LLID = 1<<3,
+ STATION_INFO_PLID = 1<<4,
+ STATION_INFO_PLINK_STATE = 1<<5,
+ STATION_INFO_SIGNAL = 1<<6,
+ STATION_INFO_TX_BITRATE = 1<<7,
+ STATION_INFO_RX_PACKETS = 1<<8,
+ STATION_INFO_TX_PACKETS = 1<<9,
+ STATION_INFO_TX_RETRIES = 1<<10,
+ STATION_INFO_TX_FAILED = 1<<11,
+ STATION_INFO_RX_DROP_MISC = 1<<12,
+ STATION_INFO_SIGNAL_AVG = 1<<13,
+ STATION_INFO_RX_BITRATE = 1<<14,
+ STATION_INFO_BSS_PARAM = 1<<15,
+ STATION_INFO_CONNECTED_TIME = 1<<16,
+ STATION_INFO_ASSOC_REQ_IES = 1<<17,
+ STATION_INFO_STA_FLAGS = 1<<18,
+ STATION_INFO_BEACON_LOSS_COUNT = 1<<19,
+ STATION_INFO_T_OFFSET = 1<<20,
+ STATION_INFO_LOCAL_PM = 1<<21,
+ STATION_INFO_PEER_PM = 1<<22,
+ STATION_INFO_NONPEER_PM = 1<<23,
+ STATION_INFO_RX_BYTES64 = 1<<24,
+ STATION_INFO_TX_BYTES64 = 1<<25,
+ STATION_INFO_CHAIN_SIGNAL = 1<<26,
+ STATION_INFO_CHAIN_SIGNAL_AVG = 1<<27,
+ STATION_INFO_EXPECTED_THROUGHPUT = 1<<28,
};
/**
@@ -1013,6 +1015,9 @@ struct sta_bss_parameters {
* @local_pm: local mesh STA power save mode
* @peer_pm: peer mesh STA power save mode
* @nonpeer_pm: non-peer mesh STA power save mode
+ * @expected_throughput: expected throughput as reported by the RC algorithm
+ * about the bitrate having the maximum throughput. This field can be filled
+ * only by drivers using Minstrel
*/
struct station_info {
u32 filled;
@@ -1051,6 +1056,8 @@ struct station_info {
enum nl80211_mesh_power_mode peer_pm;
enum nl80211_mesh_power_mode nonpeer_pm;
+ u32 expected_throughput;
+
/*
* Note: Add a new enum station_info_flags value for each new field and
* use it to check which fields are initialized.
--
1.8.3.2
Hi Andrew,
On 31/03/14 10:22, Andrew Lunn wrote:
>> /**
>> @@ -1013,6 +1015,9 @@ struct sta_bss_parameters {
>> * @local_pm: local mesh STA power save mode
>> * @peer_pm: peer mesh STA power save mode
>> * @nonpeer_pm: non-peer mesh STA power save mode
>> + * @expected_throughput: expected throughput as reported by the RC algorithm
>> + * about the bitrate having the maximum throughput. This field can be filled
>> + * only by drivers using Minstrel
>> */
>> struct station_info {
>> u32 filled;
>> @@ -1051,6 +1056,8 @@ struct station_info {
>> enum nl80211_mesh_power_mode peer_pm;
>> enum nl80211_mesh_power_mode nonpeer_pm;
>>
>> + u32 expected_throughput;
>> +
>
> It would be nice to comment on what the units are. I known from our
> BATMAN V discussions, it is something odd.
>
Thanks for raising the point.
Actually this is something we could discuss with Felix: at the moment
this value uses the same unit used by the cur_tp member of the
minstrel_ht_sta structure.
It should be Mbps/100 --> expected_throughput = 1 ==> 0.01Mbps
When exposing this value to the user (i.e. iw output) I'd recommend to
convert it to Mbps, but for internal purposes I don't see a clear
problem in exporting a value in this form.
Thoughts? Suggestions?
Cheers,
--
Antonio Quartulli
On Sun, Mar 30, 2014 at 10:34:59PM +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <[email protected]>
>
> Users may need information about the expected throughput
> towards a given peer computed by the RC algorithm.
> Export such value through the get_station() API.
>
> This information is useful to the batman-adv module which
> will use it for its new metric computation.
>
> Signed-off-by: Antonio Quartulli <[email protected]>
> ---
> include/net/cfg80211.h | 63 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 35 insertions(+), 28 deletions(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index fb8afce..4c8ebe9 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -862,36 +862,38 @@ int cfg80211_check_station_change(struct wiphy *wiphy,
> * @STATION_INFO_NONPEER_PM: @nonpeer_pm filled
> * @STATION_INFO_CHAIN_SIGNAL: @chain_signal filled
> * @STATION_INFO_CHAIN_SIGNAL_AVG: @chain_signal_avg filled
> + * @STATION_INFO_EXPECTED_THROUGHPUT: @expected_throughput filled
> */
> enum station_info_flags {
> - STATION_INFO_INACTIVE_TIME = 1<<0,
> - STATION_INFO_RX_BYTES = 1<<1,
> - STATION_INFO_TX_BYTES = 1<<2,
> - STATION_INFO_LLID = 1<<3,
> - STATION_INFO_PLID = 1<<4,
> - STATION_INFO_PLINK_STATE = 1<<5,
> - STATION_INFO_SIGNAL = 1<<6,
> - STATION_INFO_TX_BITRATE = 1<<7,
> - STATION_INFO_RX_PACKETS = 1<<8,
> - STATION_INFO_TX_PACKETS = 1<<9,
> - STATION_INFO_TX_RETRIES = 1<<10,
> - STATION_INFO_TX_FAILED = 1<<11,
> - STATION_INFO_RX_DROP_MISC = 1<<12,
> - STATION_INFO_SIGNAL_AVG = 1<<13,
> - STATION_INFO_RX_BITRATE = 1<<14,
> - STATION_INFO_BSS_PARAM = 1<<15,
> - STATION_INFO_CONNECTED_TIME = 1<<16,
> - STATION_INFO_ASSOC_REQ_IES = 1<<17,
> - STATION_INFO_STA_FLAGS = 1<<18,
> - STATION_INFO_BEACON_LOSS_COUNT = 1<<19,
> - STATION_INFO_T_OFFSET = 1<<20,
> - STATION_INFO_LOCAL_PM = 1<<21,
> - STATION_INFO_PEER_PM = 1<<22,
> - STATION_INFO_NONPEER_PM = 1<<23,
> - STATION_INFO_RX_BYTES64 = 1<<24,
> - STATION_INFO_TX_BYTES64 = 1<<25,
> - STATION_INFO_CHAIN_SIGNAL = 1<<26,
> - STATION_INFO_CHAIN_SIGNAL_AVG = 1<<27,
> + STATION_INFO_INACTIVE_TIME = 1<<0,
> + STATION_INFO_RX_BYTES = 1<<1,
> + STATION_INFO_TX_BYTES = 1<<2,
> + STATION_INFO_LLID = 1<<3,
> + STATION_INFO_PLID = 1<<4,
> + STATION_INFO_PLINK_STATE = 1<<5,
> + STATION_INFO_SIGNAL = 1<<6,
> + STATION_INFO_TX_BITRATE = 1<<7,
> + STATION_INFO_RX_PACKETS = 1<<8,
> + STATION_INFO_TX_PACKETS = 1<<9,
> + STATION_INFO_TX_RETRIES = 1<<10,
> + STATION_INFO_TX_FAILED = 1<<11,
> + STATION_INFO_RX_DROP_MISC = 1<<12,
> + STATION_INFO_SIGNAL_AVG = 1<<13,
> + STATION_INFO_RX_BITRATE = 1<<14,
> + STATION_INFO_BSS_PARAM = 1<<15,
> + STATION_INFO_CONNECTED_TIME = 1<<16,
> + STATION_INFO_ASSOC_REQ_IES = 1<<17,
> + STATION_INFO_STA_FLAGS = 1<<18,
> + STATION_INFO_BEACON_LOSS_COUNT = 1<<19,
> + STATION_INFO_T_OFFSET = 1<<20,
> + STATION_INFO_LOCAL_PM = 1<<21,
> + STATION_INFO_PEER_PM = 1<<22,
> + STATION_INFO_NONPEER_PM = 1<<23,
> + STATION_INFO_RX_BYTES64 = 1<<24,
> + STATION_INFO_TX_BYTES64 = 1<<25,
> + STATION_INFO_CHAIN_SIGNAL = 1<<26,
> + STATION_INFO_CHAIN_SIGNAL_AVG = 1<<27,
> + STATION_INFO_EXPECTED_THROUGHPUT = 1<<28,
> };
Hi Antonio
> /**
> @@ -1013,6 +1015,9 @@ struct sta_bss_parameters {
> * @local_pm: local mesh STA power save mode
> * @peer_pm: peer mesh STA power save mode
> * @nonpeer_pm: non-peer mesh STA power save mode
> + * @expected_throughput: expected throughput as reported by the RC algorithm
> + * about the bitrate having the maximum throughput. This field can be filled
> + * only by drivers using Minstrel
> */
> struct station_info {
> u32 filled;
> @@ -1051,6 +1056,8 @@ struct station_info {
> enum nl80211_mesh_power_mode peer_pm;
> enum nl80211_mesh_power_mode nonpeer_pm;
>
> + u32 expected_throughput;
> +
It would be nice to comment on what the units are. I known from our
BATMAN V discussions, it is something odd.
Andrew
From: Antonio Quartulli <[email protected]>
Cc: Felix Fietkau <[email protected]>
Signed-off-by: Antonio Quartulli <[email protected]>
---
net/mac80211/rc80211_minstrel_ht.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/net/mac80211/rc80211_minstrel_ht.c b/net/mac80211/rc80211_minstrel_ht.c
index bccaf85..480f657 100644
--- a/net/mac80211/rc80211_minstrel_ht.c
+++ b/net/mac80211/rc80211_minstrel_ht.c
@@ -1031,6 +1031,24 @@ minstrel_ht_free(void *priv)
mac80211_minstrel.free(priv);
}
+static u32
+minstrel_ht_get_expected_throughput(void *priv, void *priv_sta,
+ struct ieee80211_supported_band *sband)
+{
+ struct minstrel_ht_sta_priv *msp = priv_sta;
+ struct minstrel_ht_sta *mi = &msp->ht;
+ int i, j;
+
+ if (!msp->is_ht)
+ return mac80211_minstrel.get_expected_throughput(priv, priv_sta,
+ sband);
+
+ i = mi->max_tp_rate / MCS_GROUP_RATES;
+ j = mi->max_tp_rate % MCS_GROUP_RATES;
+
+ return mi->groups[i].rates[j].cur_tp;
+}
+
static const struct rate_control_ops mac80211_minstrel_ht = {
.name = "minstrel_ht",
.tx_status = minstrel_ht_tx_status,
@@ -1045,6 +1063,7 @@ static const struct rate_control_ops mac80211_minstrel_ht = {
.add_sta_debugfs = minstrel_ht_add_sta_debugfs,
.remove_sta_debugfs = minstrel_ht_remove_sta_debugfs,
#endif
+ .get_expected_throughput = minstrel_ht_get_expected_throughput,
};
--
1.8.3.2
From: Antonio Quartulli <[email protected]>
If the RC algorithm implements the get_expected_throughput()
API fill the related member in the station_info object when
dumping a station.
Signed-off-by: Antonio Quartulli <[email protected]>
---
net/mac80211/cfg.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index aa39381..0e80e2c 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -472,9 +472,13 @@ static void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo)
{
struct ieee80211_sub_if_data *sdata = sta->sdata;
struct ieee80211_local *local = sdata->local;
+ struct rate_control_ref *ref = local->rate_ctrl;
+ struct ieee80211_supported_band *sband;
+ enum ieee80211_band band;
struct timespec uptime;
u64 packets = 0;
int i, ac;
+ u32 thr;
sinfo->generation = sdata->local->sta_generation;
@@ -587,6 +591,19 @@ static void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo)
sinfo->sta_flags.set |= BIT(NL80211_STA_FLAG_ASSOCIATED);
if (test_sta_flag(sta, WLAN_STA_TDLS_PEER))
sinfo->sta_flags.set |= BIT(NL80211_STA_FLAG_TDLS_PEER);
+
+ if (ref->ops->get_expected_throughput) {
+ band = ieee80211_get_sdata_band(sta->sdata);
+ sband = sta->local->hw.wiphy->bands[band];
+
+ thr = ref->ops->get_expected_throughput(ref->priv,
+ sta->rate_ctrl_priv,
+ sband);
+ if (thr != 0) {
+ sinfo->filled |= STATION_INFO_EXPECTED_THROUGHPUT;
+ sinfo->expected_throughput = thr;
+ }
+ }
}
static const char ieee80211_gstrings_sta_stats[][ETH_GSTRING_LEN] = {
--
1.8.3.2
From: Antonio Quartulli <[email protected]>
Cc: Felix Fietkau <[email protected]>
Signed-off-by: Antonio Quartulli <[email protected]>
---
net/mac80211/rc80211_minstrel.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/net/mac80211/rc80211_minstrel.c b/net/mac80211/rc80211_minstrel.c
index 26fd94f..9434435d 100644
--- a/net/mac80211/rc80211_minstrel.c
+++ b/net/mac80211/rc80211_minstrel.c
@@ -657,6 +657,20 @@ minstrel_free(void *priv)
kfree(priv);
}
+static u32
+minstrel_get_expected_throughput(void *priv, void *priv_sta,
+ struct ieee80211_supported_band *sband)
+{
+ struct minstrel_sta_info *mi = priv_sta;
+ int idx = mi->max_tp_rate[0];
+ u32 bitrate, ret;
+
+ bitrate = sband->bitrates[mi->r[idx].rix].bitrate;
+ ret = bitrate * MINSTREL_TRUNC(mi->r[idx].probability * 1000) / 1000;
+
+ return ret;
+}
+
const struct rate_control_ops mac80211_minstrel = {
.name = "minstrel",
.tx_status = minstrel_tx_status,
@@ -670,6 +684,7 @@ const struct rate_control_ops mac80211_minstrel = {
.add_sta_debugfs = minstrel_add_sta_debugfs,
.remove_sta_debugfs = minstrel_remove_sta_debugfs,
#endif
+ .get_expected_throughput = minstrel_get_expected_throughput,
};
int __init
--
1.8.3.2
From: Antonio Quartulli <[email protected]>
In order to make get_station() export the expected
throughput, we need a new API which extracts such
information from the Rate Control algorithm.
Therefore add the get_expected_throughput() member
to the rate_control_ops structure.
Signed-off-by: Antonio Quartulli <[email protected]>
---
include/net/mac80211.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 604e279..209e004 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -4476,6 +4476,9 @@ struct rate_control_ops {
void (*add_sta_debugfs)(void *priv, void *priv_sta,
struct dentry *dir);
void (*remove_sta_debugfs)(void *priv, void *priv_sta);
+
+ u32 (*get_expected_throughput)(void *priv, void *priv_sta,
+ struct ieee80211_supported_band *sband);
};
static inline int rate_supported(struct ieee80211_sta *sta,
--
1.8.3.2
On Sun, 2014-03-30 at 22:35 +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <[email protected]>
>
> In order to make get_station() export the expected
> throughput, we need a new API which extracts such
> information from the Rate Control algorithm.
> Therefore add the get_expected_throughput() member
> to the rate_control_ops structure.
What about drivers with HW rate control?
johannes
On Thu, 2014-04-10 at 17:53 +0200, Antonio Quartulli wrote:
>
> On 08/04/14 12:04, Johannes Berg wrote:
> > On Sun, 2014-03-30 at 22:35 +0200, Antonio Quartulli wrote:
> >
> >> + u32 (*get_expected_throughput)(void *priv, void *priv_sta,
> >> + struct ieee80211_supported_band *sband);
> >
> > why would that need an sband argument?
> >
>
> I needed this because I saw it is required by some RC algo (only
> minstrel) to extract the bitrate starting from the index.
>
> You can check patch 4/6 for this:
>
> + bitrate = sband->bitrates[mi->r[idx].rix].bitrate;
>
> maybe I can extract it from another structure? I couldn't find any way..
Bit strange that the algorithm doesn't have the band, if it has an
index? but I guess I didn't check all the patches...
johannes
On Thu, 2014-04-10 at 17:46 +0200, Antonio Quartulli wrote:
> > What about drivers with HW rate control?
> >
>
> True, they do not implement the rate_control_ops API.
>
> Therefore, as discussed on IRC, I need to move this new API at the
> mac80211 level, so that each driver can implement it the way it prefer.
Well, not *move*, but rather have both. Or you can just say that the
drivers that may need this can worry about it ...
johannes
On 10/04/14 19:14, Johannes Berg wrote:
> On Thu, 2014-04-10 at 17:53 +0200, Antonio Quartulli wrote:
>>
>> On 08/04/14 12:04, Johannes Berg wrote:
>>> On Sun, 2014-03-30 at 22:35 +0200, Antonio Quartulli wrote:
>>>
>>>> + u32 (*get_expected_throughput)(void *priv, void *priv_sta,
>>>> + struct ieee80211_supported_band *sband);
>>>
>>> why would that need an sband argument?
>>>
>>
>> I needed this because I saw it is required by some RC algo (only
>> minstrel) to extract the bitrate starting from the index.
>>
>> You can check patch 4/6 for this:
>>
>> + bitrate = sband->bitrates[mi->r[idx].rix].bitrate;
>>
>> maybe I can extract it from another structure? I couldn't find any way..
>
> Bit strange that the algorithm doesn't have the band, if it has an
> index? but I guess I didn't check all the patches...
I just rechecked, and it looked to me that all the other functions in
Minstreal (non-HT) take the sband as argument.
It is not stored anywhere and therefore it has to be passed as argument
to this new API as well..
A general beautification might be applied to rate_control_ops (and not
only)...but it is out of the scope of this patchset :)
Cheers,
--
Antonio Quartulli
On 10/04/14 19:13, Johannes Berg wrote:
> On Thu, 2014-04-10 at 17:46 +0200, Antonio Quartulli wrote:
>
>>> What about drivers with HW rate control?
>>>
>>
>> True, they do not implement the rate_control_ops API.
>>
>> Therefore, as discussed on IRC, I need to move this new API at the
>> mac80211 level, so that each driver can implement it the way it prefer.
>
> Well, not *move*, but rather have both. Or you can just say that the
> drivers that may need this can worry about it ...
>
You are right, I meant to *add* another API at the mac80211 level and
have both. I think this is the cleanest solution.
Cheers,
--
Antonio Quartulli
On 08/04/14 12:03, Johannes Berg wrote:
> On Sun, 2014-03-30 at 22:35 +0200, Antonio Quartulli wrote:
>> From: Antonio Quartulli <[email protected]>
>>
>> In order to make get_station() export the expected
>> throughput, we need a new API which extracts such
>> information from the Rate Control algorithm.
>> Therefore add the get_expected_throughput() member
>> to the rate_control_ops structure.
>
> What about drivers with HW rate control?
>
True, they do not implement the rate_control_ops API.
Therefore, as discussed on IRC, I need to move this new API at the
mac80211 level, so that each driver can implement it the way it prefer.
Cheers,
--
Antonio Quartulli
On 08/04/14 12:05, Johannes Berg wrote:
> On Sun, 2014-03-30 at 22:35 +0200, Antonio Quartulli wrote:
>
>> +static u32
>> +minstrel_get_expected_throughput(void *priv, void *priv_sta,
>> + struct ieee80211_supported_band *sband)
>> +{
>> + struct minstrel_sta_info *mi = priv_sta;
>> + int idx = mi->max_tp_rate[0];
>> + u32 bitrate, ret;
>> +
>> + bitrate = sband->bitrates[mi->r[idx].rix].bitrate;
>> + ret = bitrate * MINSTREL_TRUNC(mi->r[idx].probability * 1000) / 1000;
>> +
>> + return ret;
>
> You don't need a ret variable.
True.
thanks!
--
Antonio Quartulli
On Sun, 2014-03-30 at 22:34 +0200, Antonio Quartulli wrote:
> - STATION_INFO_INACTIVE_TIME = 1<<0,
Since you're touching these all anyway, maybe convert to use BIT().
> @@ -1013,6 +1015,9 @@ struct sta_bss_parameters {
> * @local_pm: local mesh STA power save mode
> * @peer_pm: peer mesh STA power save mode
> * @nonpeer_pm: non-peer mesh STA power save mode
> + * @expected_throughput: expected throughput as reported by the RC algorithm
> + * about the bitrate having the maximum throughput. This field can be filled
> + * only by drivers using Minstrel
This value needs a documented unit ... :)
johannes
On Sun, 2014-03-30 at 22:35 +0200, Antonio Quartulli wrote:
> /**
> + * cfg80211_get_station - retrieve information about a given station
> + * @dev: the device where the station is supposed to be connected to
> + * @mac_addr: the mac address of the station of interest
> + * @sinfo: pointer to the structure to fill with the information
> + *
> + * Returns 0 on success or a negative error code otherwise.
> + */
> +int cfg80211_get_station(struct net_device *dev, u8 *mac_addr,
> + struct station_info *sinfo);
mac_addr should be const
Any thoughts about clearing/filling/partially filling *sinfo when
returning an error?
johannes
On Thu, 2014-04-10 at 18:02 +0200, Antonio Quartulli wrote:
> > Any thoughts about clearing/filling/partially filling *sinfo when
> > returning an error?
>
> At the moment this function relies on what rdev_get_station() does and I
> always assumed that in case of error the content of *sinfo should be
> considered "undefined".
That's fine, maybe add a line of documentation?
johannes
On Sun, 2014-03-30 at 22:35 +0200, Antonio Quartulli wrote:
> + u32 (*get_expected_throughput)(void *priv, void *priv_sta,
> + struct ieee80211_supported_band *sband);
why would that need an sband argument?
johannes
On 08/04/14 12:06, Johannes Berg wrote:
> On Sun, 2014-03-30 at 22:35 +0200, Antonio Quartulli wrote:
>
>> /**
>> + * cfg80211_get_station - retrieve information about a given station
>> + * @dev: the device where the station is supposed to be connected to
>> + * @mac_addr: the mac address of the station of interest
>> + * @sinfo: pointer to the structure to fill with the information
>> + *
>> + * Returns 0 on success or a negative error code otherwise.
>> + */
>> +int cfg80211_get_station(struct net_device *dev, u8 *mac_addr,
>> + struct station_info *sinfo);
>
> mac_addr should be const
True
>
> Any thoughts about clearing/filling/partially filling *sinfo when
> returning an error?
At the moment this function relies on what rdev_get_station() does and I
always assumed that in case of error the content of *sinfo should be
considered "undefined".
An option can be to set the object to 0 in case of error, but is it
really needed?
Any other change should be applied to rdev->ops->get_station(), not
here. I.e. fill the object as much as possible and never return an error
- just flag what was filled.
Cheers,
--
Antonio Quartulli
On 08/04/14 12:04, Johannes Berg wrote:
> On Sun, 2014-03-30 at 22:35 +0200, Antonio Quartulli wrote:
>
>> + u32 (*get_expected_throughput)(void *priv, void *priv_sta,
>> + struct ieee80211_supported_band *sband);
>
> why would that need an sband argument?
>
I needed this because I saw it is required by some RC algo (only
minstrel) to extract the bitrate starting from the index.
You can check patch 4/6 for this:
+ bitrate = sband->bitrates[mi->r[idx].rix].bitrate;
maybe I can extract it from another structure? I couldn't find any way..
Cheers,
--
Antonio Quartulli
On 08/04/14 12:03, Johannes Berg wrote:
> On Sun, 2014-03-30 at 22:34 +0200, Antonio Quartulli wrote:
>
>> - STATION_INFO_INACTIVE_TIME = 1<<0,
>
> Since you're touching these all anyway, maybe convert to use BIT().
Will do!
>
>
>> @@ -1013,6 +1015,9 @@ struct sta_bss_parameters {
>> * @local_pm: local mesh STA power save mode
>> * @peer_pm: peer mesh STA power save mode
>> * @nonpeer_pm: non-peer mesh STA power save mode
>> + * @expected_throughput: expected throughput as reported by the RC algorithm
>> + * about the bitrate having the maximum throughput. This field can be filled
>> + * only by drivers using Minstrel
>
> This value needs a documented unit ... :)
Yep. As suggested by Andrew, I will try to be clear about what unit is
returned.
Thanks a lot for your review!!!!
Cheers,
--
Antonio Quartulli
On 10/04/14 19:12, Johannes Berg wrote:
> On Thu, 2014-04-10 at 18:02 +0200, Antonio Quartulli wrote:
>
>>> Any thoughts about clearing/filling/partially filling *sinfo when
>>> returning an error?
>>
>> At the moment this function relies on what rdev_get_station() does and I
>> always assumed that in case of error the content of *sinfo should be
>> considered "undefined".
>
> That's fine, maybe add a line of documentation?
Oky! will do.
Cheers,
--
Antonio Quartulli
On Sun, 2014-03-30 at 22:35 +0200, Antonio Quartulli wrote:
> +static u32
> +minstrel_get_expected_throughput(void *priv, void *priv_sta,
> + struct ieee80211_supported_band *sband)
> +{
> + struct minstrel_sta_info *mi = priv_sta;
> + int idx = mi->max_tp_rate[0];
> + u32 bitrate, ret;
> +
> + bitrate = sband->bitrates[mi->r[idx].rix].bitrate;
> + ret = bitrate * MINSTREL_TRUNC(mi->r[idx].probability * 1000) / 1000;
> +
> + return ret;
You don't need a ret variable.
johannes