On Sat, 2008-12-06 at 15:10 +0100, Henning Rogge wrote:
> Okay, here is v6 of my patch... feel free to rip it apart. ;)
Heh. Please inline patches, makes it much easier.
> /**
> + * enum nl80211_sta_info_rate - station information about bitrate
> + *
> + * These attribute types are used with %NL80211_STA_INFO_TXRATE
> + * when getting information about the bitrate of a station.
> + *
> + * @__NL80211_STA_INFO_RATE_INVALID: attribute number 0 is reserved
> + * @NL80211_STA_INFO_RATE_TOTAL: total bitrate (u16, 100kbit/s)
> + * @NL80211_STA_INFO_RATE_MCS: mcs index for 802.11n (u8)
> + * @NL80211_STA_INFO_RATE_40_MHZ_WIDTH: 40 Mhz dualchannel bitrate
> + * @NL80211_STA_INFO_RATE_SHORT_GI: 400ns guard interval
> + */
> +enum nl80211_sta_info_rate {
> + __NL80211_STA_INFO_RATE_INVALID,
> + NL80211_STA_INFO_RATE_TOTAL,
> + NL80211_STA_INFO_RATE_MCS,
> + NL80211_STA_INFO_RATE_40_MHZ_WIDTH,
> + NL80211_STA_INFO_RATE_SHORT_GI,
> +
> + /* keep last */
> + __NL80211_STA_INFO_RATE_AFTER_LAST,
> + NL80211_STA_INFO_RATE_MAX = __NL80211_STA_INFO_RATE_AFTER_LAST - 1
> +};
I'd drop the _STA_ part from these and do RATE_INFO_... since we may end
up using these for other things potentially. Not sure we will, but who
knows. I could imagine using it to fix a transmit rate, for example, and
then it wouldn't necessarily be per station.
> + * @NL80211_STA_INFO_SIGNAL: signal strength of last received package (u8, dBm)
packet, or, more accurately, PPDU I guess
> + * @NL80211_STA_INFO_TX_BITRATE: current unicast tx rate, nested attribute
> + * containing info as possible, see &enum nl80211_sta_info_txrate.
Please indent with a tab here
> + * @STATION_INFO_RX_BITRATE: @rx_bitrate filled
> + * @STATION_INFO_TX_BITRATE: @tx_bitrate fields are filled
> + * (tx_bitrate, tx_bitrate_flags and tx_bitrate_mcs)
> */
> enum station_info_flags {
> STATION_INFO_INACTIVE_TIME = 1<<0,
> @@ -177,6 +181,25 @@ enum station_info_flags {
> STATION_INFO_LLID = 1<<3,
> STATION_INFO_PLID = 1<<4,
> STATION_INFO_PLINK_STATE = 1<<5,
> + STATION_INFO_SIGNAL = 1<<6,
> + STATION_INFO_RX_BITRATE = 1<<7,
RX bitrate?
> +/**
> + * enum station_info_rate_flags - station transmission rate flags
> + *
> + * Used by the driver to indicate the specific rate transmission
> + * type for 802.11n transmissions.
> + *
> + * @STATION_INFO_BITRATE_MCS: @tx_bitrate_mcs filled
> + * @STATION_INFO_BITRATE_40_MHZ_WIDTH: 40 Mhz width transmission
> + * @STATION_INFO_BITRATE_SHORT_GI: 400ns guard interval
> + */
> +enum station_info_rate_flags {
> + STATION_INFO_BITRATE_MCS = 1<<0,
> + STATION_INFO_BITRATE_40_MHZ_WIDTH = 1<<1,
> + STATION_INFO_BITRATE_SHORT_GI = 1<<2,
> };
Same here as the first comment, this is rate specific, so I'd call these
BITRATE_* or something like that.
> /**
> @@ -191,6 +214,11 @@ enum station_info_flags {
> * @llid: mesh local link id
> * @plid: mesh peer link id
> * @plink_state: mesh peer link state
> + * @signal: signal strength of last received package in dBm
> + * @txrate_total: current unicast bitrate to this station in 100 kbit/sec
> + * @txrate_flags: bitflag of flags from &enum station_info_bitrate_flags
> + * @txrate_mcs: MCS index of a 802.11n transmission, see
> + * &enum station_info_bitrate_flags
> */
> struct station_info {
> u32 filled;
> @@ -200,6 +228,10 @@ struct station_info {
> u16 llid;
> u16 plid;
> u8 plink_state;
> + u8 signal;
> + u16 txrate_total;
> + u8 txrate_flags;
> + u8 txrate_mcs;
can you please move the rate parts into a new struct, so we can use
tx.flags, tx.mcs etc. and later just need to add a second instance of
the struct for rx
> +static u16 sta_get_80211n_bitrate(struct ieee80211_tx_rate *rate)
> +{
> + int modulation = rate->idx & 7;
> + int streams = rate->idx >> 3;
> +
> + int bitrate = (rate->flags & IEEE80211_TX_RC_40_MHZ_WIDTH) ? 13500000 : 6500000;
> +
> + if (modulation < 4)
> + bitrate *= (modulation + 1);
> + else if (modulation == 4)
> + bitrate *= (modulation + 2);
> + else
> + bitrate *= (modulation + 3);
> +
> + bitrate *= streams;
> +
> + if (rate->flags & IEEE80211_TX_RC_SHORT_GI)
> + bitrate = (bitrate * 10) / 9;
> +
> + return (bitrate + 50000) / 100000; // do NOT just round down
> +}
Don't use // comments please, see Documentation/CodingStyle.
Also, if we really really need this information (I still don't see why
the kernel has to calculate it) then it should be in cfg80211 not
mac80211 so other drivers could potentially make use of it. Personally,
I'd just leave it out though.
> sinfo->filled = STATION_INFO_INACTIVE_TIME |
> STATION_INFO_RX_BYTES |
> - STATION_INFO_TX_BYTES;
> + STATION_INFO_TX_BYTES |
> + STATION_INFO_RX_BITRATE |
> + STATION_INFO_TX_BITRATE;
RX?
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -727,8 +727,9 @@ ieee80211_rx_h_sta_process(struct ieee80211_rx_data *rx)
> if (rx->sdata->vif.type == NL80211_IFTYPE_ADHOC) {
> u8 *bssid = ieee80211_get_bssid(hdr, rx->skb->len,
> NL80211_IFTYPE_ADHOC);
> - if (compare_ether_addr(bssid, rx->sdata->u.sta.bssid) == 0)
> + if (compare_ether_addr(bssid, rx->sdata->u.sta.bssid) == 0) {
> sta->last_rx = jiffies;
> + }
> } else
> if (!is_multicast_ether_addr(hdr->addr1) ||
> rx->sdata->vif.type == NL80211_IFTYPE_STATION) {
That looks unintended
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 9caee60..197dc4d 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -16,6 +16,7 @@
> #include <linux/netlink.h>
> #include <net/genetlink.h>
> #include <net/cfg80211.h>
> +#include <net/mac80211.h>
Ahem. Please observe the layering, cfg80211 and nl80211 are strictly
below mac80211.
I'd rename _TOTAL to _LEGACY and remove the calculation for MCS rates,
that way it's obviously clear what you're getting in userland.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
