From: Johannes Berg <johannes@sipsolutions.net>
To: Vivek Natarajan <vnatarajan@atheros.com>
Cc: linux-wireless@vger.kernel.org
Subject: Re: [RFC 1/5] mac80211: Add support for transmit beam forming.
Date: Wed, 10 Nov 2010 08:22:53 -0800 [thread overview]
Message-ID: <1289406173.3748.20.camel@jlt3.sipsolutions.net> (raw)
In-Reply-To: <1289391829-8577-1-git-send-email-vnatarajan@atheros.com>
On Wed, 2010-11-10 at 17:53 +0530, Vivek Natarajan wrote:
> +struct ieee80211_txbf_caps {
> + u32 implicit_rx_capable:1,
> + rx_staggered_sounding:1,
> + tx_staggered_sounding:1,
> + rx_ndp_capable:1,
> + tx_ndp_capable:1,
> + implicit_txbf_capable:1,
> + calibration:2,
> + explicit_csi_txbf_capable:1,
> + explicit_noncomp_steering:1,
> + explicit_comp_steering:1,
> + explicit_csi_feedback:2,
> + explicit_noncomp_bf:2,
> + explicit_comp_bf:2,
> + minimal_grouping:2,
> + csi_bfer_antennas:2,
> + noncomp_bfer_antennas:2,
> + comp_bfer_antennas:2,
> + csi_max_rows_bfer:2,
> + channel_estimation_cap:2,
> + reserved:3;
> +};
No way, never use bitfields.
> @@ -862,7 +901,7 @@ struct ieee80211_ht_cap {
> struct ieee80211_mcs_info mcs;
>
> __le16 extended_ht_cap_info;
> - __le32 tx_BF_cap_info;
> + struct ieee80211_txbf_caps tx_BF_cap_info;
> u8 antenna_selection_info;
> } __attribute__ ((packed));
keep __le32 and add #defines for the bits.
> @@ -321,6 +321,8 @@ struct ieee80211_bss_conf {
> * @IEEE80211_TX_CTL_LDPC: tells the driver to use LDPC for this frame
> * @IEEE80211_TX_CTL_STBC: Enables Space-Time Block Coding (STBC) for this
> * frame and selects the maximum number of streams that it can use.
> + * @IEEE80211_TX_CTL_TXBF_UPDATE: Channel information needs to be updated
> + * for beamforming of Tx frames.
> *
> * Note: If you have to add new flags to the enumeration, then don't
> * forget to update %IEEE80211_TX_TEMPORARY_FLAGS when necessary.
> @@ -349,6 +351,8 @@ enum mac80211_tx_control_flags {
> IEEE80211_TX_INTFL_NL80211_FRAME_TX = BIT(21),
> IEEE80211_TX_CTL_LDPC = BIT(22),
> IEEE80211_TX_CTL_STBC = BIT(23) | BIT(24),
> + IEEE80211_TX_CTL_TXBF_UPDATE = BIT(25),
> + IEEE80211_TX_CTL_STAG_SOUND = BIT(26),
Missing docs for the second entry.
> #define IEEE80211_TX_CTL_STBC_SHIFT 23
> @@ -364,7 +368,7 @@ enum mac80211_tx_control_flags {
> IEEE80211_TX_STAT_AMPDU | IEEE80211_TX_STAT_AMPDU_NO_BACK | \
> IEEE80211_TX_CTL_RATE_CTRL_PROBE | IEEE80211_TX_CTL_PSPOLL_RESPONSE | \
> IEEE80211_TX_CTL_MORE_FRAMES | IEEE80211_TX_CTL_LDPC | \
> - IEEE80211_TX_CTL_STBC)
> + IEEE80211_TX_CTL_STBC | IEEE80211_TX_CTL_TXBF_UPDATE)
Therefore I cannot evaluate this change.
> @@ -900,7 +904,8 @@ struct ieee80211_sta {
> u8 addr[ETH_ALEN];
> u16 aid;
> struct ieee80211_sta_ht_cap ht_cap;
> -
> + bool txbf;
not sure I like names that short -- docs missing too
> @@ -698,6 +698,13 @@ static void sta_apply_parameters(struct ieee80211_local *local,
> params->ht_capa,
> &sta->sta.ht_cap);
>
> + if (sta->sta.ht_cap.explicit_compbf ||
> + sta->sta.ht_cap.explicit_noncompbf ||
> + sta->sta.ht_cap.implicit_bf) {
> + sta->sta.txbf = true;
> + sta->bf_update_cv = true;
> + }
I wonder at what point we should move the capability handling from
hostapd to the kernel ...
> @@ -99,6 +100,23 @@ void ieee80211_ht_cap_ie_to_sta_ht_cap(struct ieee80211_supported_band *sband,
> /* handle MCS rate 32 too */
> if (sband->ht_cap.mcs.rx_mask[32/8] & ht_cap_ie->mcs.rx_mask[32/8] & 1)
> ht_cap->mcs.rx_mask[32/8] |= 1;
> +
> + bfee = ht_cap_ie->tx_BF_cap_info;
> + bfmr = sband->ht_cap.txbf;
Nice variable names... what?
> + if (bfmr.explicit_comp_steering && (bfee.explicit_comp_bf != 0))
> + ht_cap->explicit_compbf = true;
> +
> + if (bfmr.explicit_noncomp_steering && (bfee.explicit_noncomp_bf != 0))
> + ht_cap->explicit_noncompbf = true;
> +
> + if (bfmr.implicit_txbf_capable && bfee.implicit_rx_capable)
> + ht_cap->implicit_bf = true;
xx = a && b;
instead of the if?
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -63,6 +63,8 @@
> #define TMR_RUNNING_TIMER 0
> #define TMR_RUNNING_CHANSW 1
>
> +#define TXBF_CV_TIMER 1000
Err ... missing units.
> +void ieee80211_txbf_cv_work(struct work_struct *work)
> +{
> + struct sta_info *sta =
> + container_of(work, struct sta_info, txbf_cv_work.work);
> + struct ieee80211_local *local = sta->local;
> +
> + sta->bf_update_cv = true;
> + ieee80211_queue_delayed_work(&local->hw,
> + &sta->txbf_cv_work, TXBF_CV_TIMER);
> +}
self arming -- how does it get cancelled properly?
also, this is part of a sta_info entry, so why is it in mlme.c??
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -1455,6 +1455,16 @@ ieee80211_rx_h_remove_qos_control(struct ieee80211_rx_data *rx)
> if (!ieee80211_is_data_qos(hdr->frame_control))
> return RX_CONTINUE;
>
> + /* Qos frame with Order bit set indicates an HTC frame */
> + if (ieee80211_has_order(hdr->frame_control)) {
> + memmove(data + IEEE80211_QOS_HTC_LEN, data,
> + ieee80211_hdrlen(hdr->frame_control) -
> + IEEE80211_QOS_HTC_LEN);
> + hdr = (struct ieee80211_hdr *)skb_pull(rx->skb,
> + IEEE80211_QOS_HTC_LEN);
> + hdr->frame_control &= ~cpu_to_le16(IEEE80211_FCTL_ORDER);
> + }
> +
> /* remove the qos control field, update frame type and meta-data */
> memmove(data + IEEE80211_QOS_CTL_LEN, data,
> ieee80211_hdrlen(hdr->frame_control) - IEEE80211_QOS_CTL_LEN);
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index 6d8f897..829398e 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -235,6 +235,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
> spin_lock_init(&sta->flaglock);
> INIT_WORK(&sta->drv_unblock_wk, sta_unblock);
> INIT_WORK(&sta->ampdu_mlme.work, ieee80211_ba_session_work);
> + INIT_DELAYED_WORK(&sta->txbf_cv_work, ieee80211_txbf_cv_work);
> mutex_init(&sta->ampdu_mlme.mtx);
>
> memcpy(sta->sta.addr, addr, ETH_ALEN);
> @@ -691,6 +692,7 @@ static int __must_check __sta_info_destroy(struct sta_info *sta)
> wiphy_debug(local->hw.wiphy, "Removed STA %pM\n", sta->sta.addr);
> #endif /* CONFIG_MAC80211_VERBOSE_DEBUG */
> cancel_work_sync(&sta->drv_unblock_wk);
> + cancel_delayed_work_sync(&sta->txbf_cv_work);
>
> rate_control_remove_sta_debugfs(sta);
> ieee80211_sta_debugfs_remove(sta);
> diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
> index 9265aca..61631e3 100644
> --- a/net/mac80211/sta_info.h
> +++ b/net/mac80211/sta_info.h
> @@ -312,6 +312,12 @@ struct sta_info {
> struct sta_ampdu_mlme ampdu_mlme;
> u8 timer_to_tid[STA_TID_NUM];
>
> + bool txbf;
> + bool bf_update_cv;
> + bool bf_sound_pending;
> + bool allow_cv_update;
> + struct delayed_work txbf_cv_work;
> +
> #ifdef CONFIG_MAC80211_MESH
> /*
> * Mesh peer link attributes
> diff --git a/net/mac80211/status.c b/net/mac80211/status.c
> index 3153c19..b0447ca 100644
> --- a/net/mac80211/status.c
> +++ b/net/mac80211/status.c
> @@ -209,6 +209,25 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb)
> return;
> }
>
> + if (ieee80211_has_order(fc)) {
> + if ((info->flags & IEEE80211_TX_STAT_ACK) &&
> + (sta->bf_sound_pending)) {
> + sta->bf_sound_pending = false;
> + ieee80211_queue_delayed_work(&local->hw,
> + &sta->txbf_cv_work, 1000);
1000 what?
> + } else
> + sta->bf_update_cv = true;
> + }
> +
> +
> + if ((info->flags & IEEE80211_TX_CTL_TXBF_UPDATE) &&
> + !(sta->bf_sound_pending)) {
> + if (sta->sta.ht_cap.explicit_compbf ||
> + sta->sta.ht_cap.explicit_noncompbf ||
> + sta->sta.ht_cap.implicit_bf)
> + sta->bf_update_cv = true;
> + }
> +
> if ((local->hw.flags & IEEE80211_HW_HAS_RATE_CONTROL) &&
> (rates_idx != -1))
> sta->last_tx_rate = info->status.rates[rates_idx];
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 96c5943..5900cf2 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -1888,6 +1888,8 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
> if ((sta_flags & WLAN_STA_WME) && local->hw.queues >= 4) {
> fc |= cpu_to_le16(IEEE80211_STYPE_QOS_DATA);
> hdrlen += 2;
> + if (sta->bf_update_cv)
> + hdrlen += 4;
4?
> @@ -1973,9 +1975,28 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
>
> if (ieee80211_is_data_qos(fc)) {
> __le16 *qos_control;
> -
> - qos_control = (__le16*) skb_push(skb, 2);
> - memcpy(skb_push(skb, hdrlen - 2), &hdr, hdrlen - 2);
> + __le32 *htc;
> +
> + if (sta->bf_update_cv) {
It seems to me that this variable access is racy between the hdrlen += 4
and checking it again here since there's no locking on it.
> + hdr.frame_control |= cpu_to_le16(IEEE80211_FCTL_ORDER);
> + htc = (__le32 *) skb_push(skb, 4);
> + sta->bf_sound_pending = true;
> + *htc = 0;
> + sta->bf_update_cv = false;
> +
> + if (sta->sta.ht_cap.explicit_compbf)
> + *htc |= IEEE80211_HTC2_CSI_COMP_BF;
no cpu_to_le32? have you run sparse on this?
> + qos_control = (__le16 *) skb_push(skb, 2);
> + memcpy(skb_push(skb, hdrlen - 6), &hdr, hdrlen - 6);
> + } else {
> + qos_control = (__le16 *) skb_push(skb, 2);
> + memcpy(skb_push(skb, hdrlen - 2), &hdr, hdrlen - 2);
> + };
}; ???
also why not move this out of the if()?
johannes
prev parent reply other threads:[~2010-11-10 16:21 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-10 12:23 [RFC 1/5] mac80211: Add support for transmit beam forming Vivek Natarajan
2010-11-10 12:23 ` [RFC 2/5] ath: Add a keycache entry if beamforming is enabled Vivek Natarajan
2010-11-10 12:23 ` [RFC 3/5] ath9k_common: Add HTC field length if order bit is set Vivek Natarajan
2010-11-10 12:23 ` [RFC 4/5] ath9k: Add support for Tx beamforming feature Vivek Natarajan
2010-11-10 12:23 ` [RFC 5/5] ath9k_hw: Add support for Tx beamforming Vivek Natarajan
2010-11-10 13:26 ` Felix Fietkau
2010-11-10 17:25 ` Luis R. Rodriguez
2010-11-10 13:06 ` [RFC 4/5] ath9k: Add support for Tx beamforming feature Felix Fietkau
2010-11-10 12:47 ` [RFC 2/5] ath: Add a keycache entry if beamforming is enabled Felix Fietkau
2010-11-10 12:44 ` [RFC 1/5] mac80211: Add support for transmit beam forming Felix Fietkau
2010-11-10 16:22 ` Johannes Berg [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1289406173.3748.20.camel@jlt3.sipsolutions.net \
--to=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
--cc=vnatarajan@atheros.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).