| Message ID | 20130415100909.GA29924@magnum.frso.rivierawaves.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, 2013-04-15 at 12:09 +0200, Karl Beldan wrote: > It seems to me we have to perform one of the following, otherwise one > driver may set negative rate indexes and iw et.al will report VHT NSSes > starting at 0. Hmm, yeah, this does seem inconsistent. > { > diff --git a/include/net/mac80211.h b/include/net/mac80211.h > index 0dde213..2317ca9 100644 > --- a/include/net/mac80211.h > +++ b/include/net/mac80211.h > @@ -601,8 +601,8 @@ static inline void ieee80211_rate_set_vht(struct ieee80211_tx_rate *rate, > u8 mcs, u8 nss) > { > WARN_ON(mcs & ~0xF); > - WARN_ON(nss & ~0x7); > - rate->idx = (nss << 4) | mcs; > + WARN_ON((nss - 1) & ~0x7); > + rate->idx = ((nss - 1) << 4) | mcs; > } > > static inline u8 > @@ -614,7 +614,7 @@ ieee80211_rate_get_vht_mcs(const struct ieee80211_tx_rate *rate) > static inline u8 > ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate) > { > - return rate->idx >> 4; > + return (rate->idx >> 4) + 1; > } > > /** > } I think this is nicer? But it should probably have some comments. > diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c > index 56df249..9631391 100644 > --- a/drivers/net/wireless/iwlwifi/mvm/tx.c > +++ b/drivers/net/wireless/iwlwifi/mvm/tx.c > @@ -545,7 +545,7 @@ static void iwl_mvm_hwrate_to_tx_control(u32 rate_n_flags, > ieee80211_rate_set_vht( > r, rate_n_flags & RATE_VHT_MCS_RATE_CODE_MSK, > ((rate_n_flags & RATE_VHT_MCS_NSS_MSK) >> > - RATE_VHT_MCS_NSS_POS) + 1); > + RATE_VHT_MCS_NSS_POS)); > r->flags |= IEEE80211_TX_RC_VHT_MCS; > } else { > r->idx = iwl_mvm_legacy_rate_to_mac80211_idx(rate_n_flags, > diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c > index fdd95bd..358d93c 100644 > --- a/net/mac80211/cfg.c > +++ b/net/mac80211/cfg.c > @@ -389,7 +389,7 @@ void sta_set_rate_info_tx(struct sta_info *sta, > } else if (rate->flags & IEEE80211_TX_RC_VHT_MCS) { > rinfo->flags |= RATE_INFO_FLAGS_VHT_MCS; > rinfo->mcs = ieee80211_rate_get_vht_mcs(rate); > - rinfo->nss = ieee80211_rate_get_vht_nss(rate); > + rinfo->nss = ieee80211_rate_get_vht_nss(rate) + 1; > } else { > struct ieee80211_supported_band *sband; > sband = sta->local->hw.wiphy->bands[ > } Wouldn't this one also require an update for VHT radiotap in net/mac80211/rx.c around line 320 (RX_FLAG_VHT)? 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
On Mon, Apr 15, 2013 at 12:32:55PM +0200, Johannes Berg wrote: > On Mon, 2013-04-15 at 12:09 +0200, Karl Beldan wrote: > > It seems to me we have to perform one of the following, otherwise one > > driver may set negative rate indexes and iw et.al will report VHT NSSes > > starting at 0. > > Hmm, yeah, this does seem inconsistent. > > > { > > diff --git a/include/net/mac80211.h b/include/net/mac80211.h > > index 0dde213..2317ca9 100644 > > --- a/include/net/mac80211.h > > +++ b/include/net/mac80211.h > > @@ -601,8 +601,8 @@ static inline void ieee80211_rate_set_vht(struct ieee80211_tx_rate *rate, > > u8 mcs, u8 nss) > > { > > WARN_ON(mcs & ~0xF); > > - WARN_ON(nss & ~0x7); > > - rate->idx = (nss << 4) | mcs; > > + WARN_ON((nss - 1) & ~0x7); > > + rate->idx = ((nss - 1) << 4) | mcs; > > } > > > > static inline u8 > > @@ -614,7 +614,7 @@ ieee80211_rate_get_vht_mcs(const struct ieee80211_tx_rate *rate) > > static inline u8 > > ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate) > > { > > - return rate->idx >> 4; > > + return (rate->idx >> 4) + 1; > > } > > > > /** > > } > > I think this is nicer? But it should probably have some comments. > This is what I prefer too. Ok, I'll send a patch then. > > diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c > > index 56df249..9631391 100644 > > --- a/drivers/net/wireless/iwlwifi/mvm/tx.c > > +++ b/drivers/net/wireless/iwlwifi/mvm/tx.c > > @@ -545,7 +545,7 @@ static void iwl_mvm_hwrate_to_tx_control(u32 rate_n_flags, > > ieee80211_rate_set_vht( > > r, rate_n_flags & RATE_VHT_MCS_RATE_CODE_MSK, > > ((rate_n_flags & RATE_VHT_MCS_NSS_MSK) >> > > - RATE_VHT_MCS_NSS_POS) + 1); > > + RATE_VHT_MCS_NSS_POS)); > > r->flags |= IEEE80211_TX_RC_VHT_MCS; > > } else { > > r->idx = iwl_mvm_legacy_rate_to_mac80211_idx(rate_n_flags, > > diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c > > index fdd95bd..358d93c 100644 > > --- a/net/mac80211/cfg.c > > +++ b/net/mac80211/cfg.c > > @@ -389,7 +389,7 @@ void sta_set_rate_info_tx(struct sta_info *sta, > > } else if (rate->flags & IEEE80211_TX_RC_VHT_MCS) { > > rinfo->flags |= RATE_INFO_FLAGS_VHT_MCS; > > rinfo->mcs = ieee80211_rate_get_vht_mcs(rate); > > - rinfo->nss = ieee80211_rate_get_vht_nss(rate); > > + rinfo->nss = ieee80211_rate_get_vht_nss(rate) + 1; > > } else { > > struct ieee80211_supported_band *sband; > > sband = sta->local->hw.wiphy->bands[ > > } > > > Wouldn't this one also require an update for VHT radiotap in > net/mac80211/rx.c around line 320 (RX_FLAG_VHT)? > The radiotap field is set with ieee80211_rx_status.vht_nss, so no need. Karl -- 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 Mon, 2013-04-15 at 14:33 +0200, Karl Beldan wrote: > > > +++ b/net/mac80211/cfg.c > > > @@ -389,7 +389,7 @@ void sta_set_rate_info_tx(struct sta_info *sta, > > > } else if (rate->flags & IEEE80211_TX_RC_VHT_MCS) { > > > rinfo->flags |= RATE_INFO_FLAGS_VHT_MCS; > > > rinfo->mcs = ieee80211_rate_get_vht_mcs(rate); > > > - rinfo->nss = ieee80211_rate_get_vht_nss(rate); > > > + rinfo->nss = ieee80211_rate_get_vht_nss(rate) + 1; > > > } else { > > > struct ieee80211_supported_band *sband; > > > sband = sta->local->hw.wiphy->bands[ > > > } > > > > > > Wouldn't this one also require an update for VHT radiotap in > > net/mac80211/rx.c around line 320 (RX_FLAG_VHT)? > > > The radiotap field is set with ieee80211_rx_status.vht_nss, so no need. And that's properly 1-based, rather than 0-based like in TX info? I guess I forgot all of this already, heh. 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
On Mon, Apr 15, 2013 at 02:47:55PM +0200, Johannes Berg wrote: > On Mon, 2013-04-15 at 14:33 +0200, Karl Beldan wrote: > > > > > +++ b/net/mac80211/cfg.c > > > > @@ -389,7 +389,7 @@ void sta_set_rate_info_tx(struct sta_info *sta, > > > > } else if (rate->flags & IEEE80211_TX_RC_VHT_MCS) { > > > > rinfo->flags |= RATE_INFO_FLAGS_VHT_MCS; > > > > rinfo->mcs = ieee80211_rate_get_vht_mcs(rate); > > > > - rinfo->nss = ieee80211_rate_get_vht_nss(rate); > > > > + rinfo->nss = ieee80211_rate_get_vht_nss(rate) + 1; > > > > } else { > > > > struct ieee80211_supported_band *sband; > > > > sband = sta->local->hw.wiphy->bands[ > > > > } > > > > > > > > > Wouldn't this one also require an update for VHT radiotap in > > > net/mac80211/rx.c around line 320 (RX_FLAG_VHT)? > > > > > The radiotap field is set with ieee80211_rx_status.vht_nss, so no need. > > And that's properly 1-based, rather than 0-based like in TX info? I > guess I forgot all of this already, heh. > ieee80211_rx_status.vht_nss asks for a 1-based field but it is set by drivers .. and since no driver report any (yet) .. anyways, I'll send a patch along with it for mac80211_hwsim .. that can serve as a reminder ;). Karl -- 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 Mon, 2013-04-15 at 14:53 +0200, Karl Beldan wrote: > > > The radiotap field is set with ieee80211_rx_status.vht_nss, so no need. > > > > And that's properly 1-based, rather than 0-based like in TX info? I > > guess I forgot all of this already, heh. > > > ieee80211_rx_status.vht_nss asks for a 1-based field but it is set by > drivers .. and since no driver report any (yet) .. anyways, I'll send a > patch along with it for mac80211_hwsim .. that can serve as a reminder ;). I think our driver reports it already. Anyway, looking forward to your patch :) 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
On Mon, Apr 15, 2013 at 03:06:27PM +0200, Johannes Berg wrote: > On Mon, 2013-04-15 at 14:53 +0200, Karl Beldan wrote: > > > > > The radiotap field is set with ieee80211_rx_status.vht_nss, so no need. > > > > > > And that's properly 1-based, rather than 0-based like in TX info? I > > > guess I forgot all of this already, heh. > > > > > ieee80211_rx_status.vht_nss asks for a 1-based field but it is set by > > drivers .. and since no driver report any (yet) .. anyways, I'll send a > > patch along with it for mac80211_hwsim .. that can serve as a reminder ;). > > I think our driver reports it already. > > Anyway, looking forward to your patch :) > Sent .. the description is a bit verbose for such a trivial change, adjust as you please. Karl -- 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/mac80211.h b/include/net/mac80211.h index 0dde213..2317ca9 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -601,8 +601,8 @@ static inline void ieee80211_rate_set_vht(struct ieee80211_tx_rate *rate, u8 mcs, u8 nss) { WARN_ON(mcs & ~0xF); - WARN_ON(nss & ~0x7); - rate->idx = (nss << 4) | mcs; + WARN_ON((nss - 1) & ~0x7); + rate->idx = ((nss - 1) << 4) | mcs; } static inline u8 @@ -614,7 +614,7 @@ ieee80211_rate_get_vht_mcs(const struct ieee80211_tx_rate *rate) static inline u8 ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate *rate) { - return rate->idx >> 4; + return (rate->idx >> 4) + 1; } /** } or: { diff --git a/drivers/net/wireless/iwlwifi/mvm/tx.c b/drivers/net/wireless/iwlwifi/mvm/tx.c index 56df249..9631391 100644 --- a/drivers/net/wireless/iwlwifi/mvm/tx.c +++ b/drivers/net/wireless/iwlwifi/mvm/tx.c @@ -545,7 +545,7 @@ static void iwl_mvm_hwrate_to_tx_control(u32 rate_n_flags, ieee80211_rate_set_vht( r, rate_n_flags & RATE_VHT_MCS_RATE_CODE_MSK, ((rate_n_flags & RATE_VHT_MCS_NSS_MSK) >> - RATE_VHT_MCS_NSS_POS) + 1); + RATE_VHT_MCS_NSS_POS)); r->flags |= IEEE80211_TX_RC_VHT_MCS; } else { r->idx = iwl_mvm_legacy_rate_to_mac80211_idx(rate_n_flags, diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index fdd95bd..358d93c 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -389,7 +389,7 @@ void sta_set_rate_info_tx(struct sta_info *sta, } else if (rate->flags & IEEE80211_TX_RC_VHT_MCS) { rinfo->flags |= RATE_INFO_FLAGS_VHT_MCS; rinfo->mcs = ieee80211_rate_get_vht_mcs(rate); - rinfo->nss = ieee80211_rate_get_vht_nss(rate); + rinfo->nss = ieee80211_rate_get_vht_nss(rate) + 1; } else { struct ieee80211_supported_band *sband; sband = sta->local->hw.wiphy->bands[
It seems to me we have to perform one of the following, otherwise one driver may set negative rate indexes and iw et.al will report VHT NSSes starting at 0. { } Karl -- 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