[PATCH] Refactor TX path
Olof Johansson
dev at skyshaper.net
Sat Jul 6 03:25:00 EDT 2013
It was a big patch, I've made some initial comments on the pull
request on github.
In general, I think it looke better and less messy without doubt though.
Will continue review later.
--
Olof
On Fri, Jul 5, 2013 at 9:24 PM, Eugene Krasnikov <k.eugene.e at gmail.com> wrote:
> Hi All,
>
> This patch is a refactoring of TX path. There are a lot of changes in
> this patch so i would really appreciate your review and feedback. What
> else can be improved?
> This is pull request to make review process easier
> https://github.com/KrasnikovEugene/wcn36xx/pull/68
>
> Mean while i will test the patch as much as possible. Will go through
> all possible usecases in both STA and AP mode.
>
>
> 2013/7/5 Eugene Krasnikov <k.eugene.e at gmail.com>:
>> With this patch all parameters will be first passed to txrx
>> module and only after that to dxe. This will allow passing
>> less parameters to the functions. Also make sure that data
>> frames will get as fast as possible to the HW.
>>
>> Short function description:
>>
>> wcn36xx_start_tx – fills in BD and trigger dxe to start
>> transmitting.
>>
>> wcn36xx_fill_tx_pdu – fills is PDU part of BD.
>>
>> wcn36xx_dxe_tx_frame – transmits one frame from the head
>> of the descriptors.
>>
>> Signed-off-by: Eugene Krasnikov <k.eugene.e at gmail.com>
>> ---
>> dxe.c | 59 +++++---------------------
>> dxe.h | 13 ++----
>> main.c | 35 +---------------
>> txrx.c | 139 +++++++++++++++++++++++++++++++++++++++++++++-----------------
>> txrx.h | 16 ++++++--
>> wcn36xx.h | 1 +
>> 6 files changed, 130 insertions(+), 133 deletions(-)
>>
>> diff --git a/dxe.c b/dxe.c
>> index 7237f4f..5e0dd89 100644
>> --- a/dxe.c
>> +++ b/dxe.c
>> @@ -24,6 +24,13 @@
>> #include "wcn36xx.h"
>> #include "txrx.h"
>>
>> +void *wcn36xx_dxe_get_next_bd(struct wcn36xx *wcn, bool is_low)
>> +{
>> + struct wcn36xx_dxe_ch *ch = is_low ?
>> + &wcn->dxe_tx_l_ch :
>> + &wcn->dxe_tx_h_ch;
>> + return ch->head_blk_ctl->bd_cpu_addr;
>> +}
>> static void wcn36xx_dxe_write_register(struct wcn36xx *wcn, int addr, int data)
>> {
>> wcn36xx_dbg(WCN36XX_DBG_DXE,
>> @@ -525,63 +532,19 @@ void wcn36xx_dxe_free_mem_pools(struct wcn36xx *wcn)
>> }
>> }
>>
>> -int wcn36xx_dxe_tx(struct wcn36xx *wcn,
>> - struct sk_buff *skb,
>> - u8 broadcast,
>> - bool is_high,
>> - u32 header_len,
>> - bool tx_ack,
>> - struct wcn_sta *sta_priv)
>> +int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
>> + struct sk_buff *skb,
>> + bool is_low)
>> {
>> struct wcn36xx_dxe_ctl *ctl = NULL;
>> struct wcn36xx_dxe_desc *desc = NULL;
>> struct wcn36xx_dxe_ch *ch = NULL;
>> - struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>> - unsigned long flags;
>> -
>> - ch = is_high ? &wcn->dxe_tx_h_ch : &wcn->dxe_tx_l_ch;
>> -
>> - if (tx_ack) {
>> - wcn36xx_dbg(WCN36XX_DBG_DXE, "TX_ACK status requested");
>> - spin_lock_irqsave(&wcn->dxe_lock, flags);
>> - if (wcn->tx_ack_skb) {
>> - spin_unlock_irqrestore(&wcn->dxe_lock, flags);
>> - wcn36xx_warn("tx_ack_skb already set");
>> - ieee80211_free_txskb(wcn->hw, skb);
>> - return -EINVAL;
>> - }
>> -
>> - wcn->tx_ack_skb = skb;
>> - spin_unlock_irqrestore(&wcn->dxe_lock, flags);
>>
>> - /* Only one at a time is supported by fw. Stop the TX queues
>> - * until the ack status gets back.
>> - *
>> - * TODO: Add watchdog in case FW does not answer
>> - */
>> - ieee80211_stop_queues(wcn->hw);
>> - }
>> + ch = is_low ? &wcn->dxe_tx_l_ch : &wcn->dxe_tx_h_ch;
>>
>> ctl = ch->head_blk_ctl;
>> ctl->skb = NULL;
>> desc = ctl->desc;
>> - if (!ctl->bd_cpu_addr) {
>> - /* TX DXE are used in pairs. One for the BD and one for the
>> - actual frame. The BD DXE's has a preallocated buffer while
>> - the skb ones does not. If this isn't true something is really
>> - wierd. TODO: Recover from this situation
>> - */
>> -
>> - wcn36xx_error("bd_cpu_addr may not be NULL for BD DXE");
>> - return -EINVAL;
>> - }
>> -
>> - wcn36xx_prepare_tx_bd(ctl->bd_cpu_addr, skb->len, header_len);
>> - wcn36xx_fill_tx_bd(wcn, ctl->bd_cpu_addr, broadcast,
>> - hdr, tx_ack, sta_priv);
>> -
>> - ctl = ch->head_blk_ctl;
>> - desc = ctl->desc;
>>
>> /* Set source address of the BD we send */
>> desc->src_addr_l = ctl->bd_phy_addr;
>> diff --git a/dxe.h b/dxe.h
>> index 9274d51..0b154bb 100644
>> --- a/dxe.h
>> +++ b/dxe.h
>> @@ -222,8 +222,6 @@ struct wcn36xx_dxe_mem_pool {
>> void *virt_addr;
>> dma_addr_t phy_addr;
>> };
>> -struct wcn36xx;
>> -struct wcn_sta;
>> int wcn36xx_dxe_allocate_mem_pools(struct wcn36xx *wcn);
>> void wcn36xx_dxe_free_mem_pools(struct wcn36xx *wcn);
>> void wcn36xx_rx_ready_work(struct work_struct *work);
>> @@ -232,12 +230,9 @@ void wcn36xx_dxe_free_ctl_blks(struct wcn36xx *wcn);
>> int wcn36xx_dxe_init(struct wcn36xx *wcn);
>> void wcn36xx_dxe_deinit(struct wcn36xx *wcn);
>> int wcn36xx_dxe_init_channels(struct wcn36xx *wcn);
>> -int wcn36xx_dxe_tx(struct wcn36xx *wcn,
>> - struct sk_buff *skb,
>> - u8 broadcast,
>> - bool is_high,
>> - u32 header_len,
>> - bool tx_compl,
>> - struct wcn_sta *sta_priv);
>> +int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
>> + struct sk_buff *skb,
>> + bool is_low);
>> void wcn36xx_dxe_tx_ack_ind(struct wcn36xx *wcn, u32 status);
>> +void *wcn36xx_dxe_get_next_bd(struct wcn36xx *wcn, bool is_low);
>> #endif /* _DXE_H_ */
>> diff --git a/main.c b/main.c
>> index 74646b0..cbfe403 100644
>> --- a/main.c
>> +++ b/main.c
>> @@ -14,7 +14,6 @@
>> * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>> */
>>
>> -#include <linux/etherdevice.h>
>> #include <linux/module.h>
>> #include <linux/wcnss_wlan.h>
>> #include "wcn36xx.h"
>> @@ -309,46 +308,14 @@ static void wcn36xx_tx(struct ieee80211_hw *hw,
>> struct ieee80211_tx_control *control,
>> struct sk_buff *skb)
>> {
>> - struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
>> - struct ieee80211_mgmt *mgmt;
>> - bool high, bcast, tx_compl;
>> - u32 header_len = 0;
>> struct wcn36xx *wcn = hw->priv;
>> struct wcn_sta *sta_priv = NULL;
>>
>> if (control->sta)
>> sta_priv = (struct wcn_sta *)control->sta->drv_priv;
>>
>> - tx_compl = info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS;
>> - mgmt = (struct ieee80211_mgmt *)skb->data;
>> -
>> - high = !(ieee80211_is_data(mgmt->frame_control) ||
>> - ieee80211_is_data_qos(mgmt->frame_control));
>> -
>> - bcast = is_broadcast_ether_addr(mgmt->da) ||
>> - is_multicast_ether_addr(mgmt->da);
>> -
>> - /*
>> - * In joining state trick hardware that probe is sent as unicast even
>> - * if address is broadcast.
>> - */
>> - if (wcn->is_joining && ieee80211_is_probe_req(mgmt->frame_control))
>> - bcast = false;
>> - wcn36xx_dbg(WCN36XX_DBG_TX,
>> - "tx skb %p len %d fc %04x sn %d %s %s",
>> - skb, skb->len, __le16_to_cpu(mgmt->frame_control),
>> - IEEE80211_SEQ_TO_SN(__le16_to_cpu(mgmt->seq_ctrl)),
>> - high ? "high" : "low", bcast ? "bcast" : "ucast");
>> -
>> - wcn36xx_dbg_dump(WCN36XX_DBG_TX_DUMP, "", skb->data, skb->len);
>> -
>> - header_len = ieee80211_is_data_qos(mgmt->frame_control) ?
>> - sizeof(struct ieee80211_qos_hdr) :
>> - sizeof(struct ieee80211_hdr_3addr);
>> - wcn36xx_dxe_tx(hw->priv, skb, bcast, high,
>> - header_len, tx_compl, sta_priv);
>> + wcn36xx_start_tx(wcn, sta_priv, skb);
>> }
>> -
>> static int wcn36xx_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>> struct ieee80211_vif *vif,
>> struct ieee80211_sta *sta,
>> diff --git a/txrx.c b/txrx.c
>> index 3b85727..5c35ec0 100644
>> --- a/txrx.c
>> +++ b/txrx.c
>> @@ -73,52 +73,85 @@ int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb)
>>
>> return 0;
>> }
>> -void wcn36xx_prepare_tx_bd(struct wcn36xx_tx_bd *bd, u32 len, u32 header_len)
>> +
>> +void wcn36xx_fill_tx_pdu(struct wcn36xx_tx_bd *bd,
>> + u32 mpdu_header_len,
>> + u32 len)
>> {
>> - memset(bd, 0, sizeof(*bd));
>> - bd->pdu.mpdu_header_len = header_len;
>> + bd->pdu.mpdu_header_len = mpdu_header_len;
>> bd->pdu.mpdu_header_off = sizeof(*bd);
>> bd->pdu.mpdu_data_off = bd->pdu.mpdu_header_len +
>> bd->pdu.mpdu_header_off;
>> bd->pdu.mpdu_len = len;
>> + bd->pdu.tid = WCN36XX_TID;
>> }
>> -void wcn36xx_fill_tx_bd(struct wcn36xx *wcn, struct wcn36xx_tx_bd *bd,
>> - u8 broadcast, struct ieee80211_hdr *hdr,
>> - bool tx_compl, struct wcn_sta *sta_priv)
>> +
>> +int wcn36xx_start_tx(struct wcn36xx *wcn,
>> + struct wcn_sta *sta_priv,
>> + struct sk_buff *skb)
>> {
>> - bd->dpu_rf = WCN36XX_BMU_WQ_TX;
>> - bd->pdu.tid = WCN36XX_TID;
>> - bd->pdu.reserved3 = 0xd;
>> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
>> + unsigned long flags;
>> + bool is_low = ieee80211_is_data(hdr->frame_control);
>> + bool bcast = is_broadcast_ether_addr(hdr->addr1) ||
>> + is_multicast_ether_addr(hdr->addr1);
>> + struct wcn36xx_tx_bd *bd = wcn36xx_dxe_get_next_bd(wcn, is_low);
>> +
>> + if (!bd) {
>> + /*
>> + * TX DXE are used in pairs. One for the BD and one for the
>> + * actual frame. The BD DXE's has a preallocated buffer while
>> + * the skb ones does not. If this isn't true something is really
>> + * wierd. TODO: Recover from this situation
>> + */
>> +
>> + wcn36xx_error("bd address may not be NULL for BD DXE");
>> + return -EINVAL;
>> + }
>>
>> - if (broadcast) {
>> - /* broadcast */
>> - bd->ub = 1;
>> - bd->queue_id = WCN36XX_TX_B_WQ_ID;
>> + wcn36xx_dbg(WCN36XX_DBG_TX,
>> + "tx skb %p len %d fc %04x sn %d %s %s",
>> + skb, skb->len, __le16_to_cpu(hdr->frame_control),
>> + IEEE80211_SEQ_TO_SN(__le16_to_cpu(hdr->seq_ctrl)),
>> + is_low ? "low" : "high", bcast ? "bcast" : "ucast");
>>
>> - /* default rate for broadcast */
>> - if (ieee80211_is_mgmt(hdr->frame_control))
>> - bd->bd_rate = (wcn->band == IEEE80211_BAND_5GHZ) ?
>> - WCN36XX_BD_RATE_CTRL :
>> - WCN36XX_BD_RATE_MGMT;
>> - /* No ack needed not unicast */
>> - bd->ack_policy = 1;
>> - } else {
>> - bd->queue_id = WCN36XX_TX_U_WQ_ID;
>> - /* default rate for unicast */
>> - bd->ack_policy = 0;
>> - if (ieee80211_is_data(hdr->frame_control))
>> - bd->bd_rate = WCN36XX_BD_RATE_DATA;
>> - else if (ieee80211_is_mgmt(hdr->frame_control))
>> - bd->bd_rate = (wcn->band == IEEE80211_BAND_5GHZ) ?
>> - WCN36XX_BD_RATE_CTRL :
>> - WCN36XX_BD_RATE_MGMT;
>> - else if (ieee80211_is_ctl(hdr->frame_control))
>> - bd->bd_rate = WCN36XX_BD_RATE_CTRL;
>> - else
>> - wcn36xx_warn("frame control type unknown");
>> + wcn36xx_dbg_dump(WCN36XX_DBG_TX_DUMP, "", skb->data, skb->len);
>> +
>> + memset(bd, 0, sizeof(*bd));
>> +
>> + bd->dpu_rf = WCN36XX_BMU_WQ_TX;
>> +
>> + bd->tx_comp = info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS;
>> + if (bd->tx_comp) {
>> + wcn36xx_dbg(WCN36XX_DBG_DXE, "TX_ACK status requested");
>> + spin_lock_irqsave(&wcn->dxe_lock, flags);
>> + if (wcn->tx_ack_skb) {
>> + spin_unlock_irqrestore(&wcn->dxe_lock, flags);
>> + wcn36xx_warn("tx_ack_skb already set");
>> + ieee80211_free_txskb(wcn->hw, skb);
>> + return -EINVAL;
>> + }
>> +
>> + wcn->tx_ack_skb = skb;
>> + spin_unlock_irqrestore(&wcn->dxe_lock, flags);
>> +
>> + /* Only one at a time is supported by fw. Stop the TX queues
>> + * until the ack status gets back.
>> + *
>> + * TODO: Add watchdog in case FW does not answer
>> + */
>> + ieee80211_stop_queues(wcn->hw);
>> }
>>
>> - if (ieee80211_is_data(hdr->frame_control)) {
>> + wcn36xx_fill_tx_pdu(bd, ieee80211_is_data_qos(hdr->frame_control) ?
>> + sizeof(struct ieee80211_qos_hdr) :
>> + sizeof(struct ieee80211_hdr_3addr),
>> + skb->len);
>> +
>> + /* Data frames served first*/
>> + if (is_low) {
>> + bd->bd_rate = WCN36XX_BD_RATE_DATA;
>> bd->dpu_sign = wcn->current_vif->ucast_dpu_signature;
>> bd->queue_id = 0;
>> bd->sta_index = wcn->current_vif->sta_index;
>> @@ -126,15 +159,45 @@ void wcn36xx_fill_tx_bd(struct wcn36xx *wcn, struct wcn36xx_tx_bd *bd,
>> if (ieee80211_is_nullfunc(hdr->frame_control) ||
>> (sta_priv && !sta_priv->is_data_encrypted))
>> bd->dpu_ne = 1;
>> -
>> + if (bcast) {
>> + bd->ub = 1;
>> + bd->ack_policy = 1;
>> + }
>> } else {
>> bd->sta_index = wcn->current_vif->self_sta_index;
>> bd->dpu_desc_idx = wcn->current_vif->self_dpu_desc_index;
>> bd->dpu_ne = 1;
>> - }
>>
>> - bd->tx_comp = tx_compl;
>> + /* default rate for unicast */
>> + if (ieee80211_is_mgmt(hdr->frame_control))
>> + bd->bd_rate = (wcn->band == IEEE80211_BAND_5GHZ) ?
>> + WCN36XX_BD_RATE_CTRL :
>> + WCN36XX_BD_RATE_MGMT;
>> + else if (ieee80211_is_ctl(hdr->frame_control))
>> + bd->bd_rate = WCN36XX_BD_RATE_CTRL;
>> + else
>> + wcn36xx_warn("frame control type unknown");
>> +
>> + /*
>> + * In joining state trick hardware that probe is sent as
>> + * unicast even if address is broadcast.
>> + */
>> + if (wcn->is_joining &&
>> + ieee80211_is_probe_req(hdr->frame_control))
>> + bcast = false;
>> +
>> + if (bcast) {
>> + /* broadcast */
>> + bd->ub = 1;
>> + /* No ack needed not unicast */
>> + bd->ack_policy = 1;
>> + bd->queue_id = WCN36XX_TX_B_WQ_ID;
>> + } else
>> + bd->queue_id = WCN36XX_TX_U_WQ_ID;
>> + }
>>
>> buff_to_be((u32 *)bd, sizeof(*bd)/sizeof(u32));
>> bd->tx_bd_sign = 0xbdbdbdbd;
>> +
>> + return wcn36xx_dxe_tx_frame(wcn, skb, is_low);
>> }
>> diff --git a/txrx.h b/txrx.h
>> index ffcbd17..bf2994f 100644
>> --- a/txrx.h
>> +++ b/txrx.h
>> @@ -17,6 +17,7 @@
>> #ifndef _TXRX_H_
>> #define _TXRX_H_
>>
>> +#include <linux/etherdevice.h>
>> #include "wcn36xx.h"
>>
>> /* TODO describe all properties */
>> @@ -147,9 +148,16 @@ struct wcn36xx_tx_bd {
>> u32 header_cks:16;
>> u32 reserved7:6;*/
>> };
>> +
>> +struct wcn_sta;
>> +struct wcn36xx;
>> +
>> int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb);
>> -void wcn36xx_prepare_tx_bd(struct wcn36xx_tx_bd *bd, u32 len, u32 header_len);
>> -void wcn36xx_fill_tx_bd(struct wcn36xx *wcn, struct wcn36xx_tx_bd *bd,
>> - u8 broadcast, struct ieee80211_hdr *hdr,
>> - bool tx_compl, struct wcn_sta *sta_priv);
>> +void wcn36xx_fill_tx_pdu(struct wcn36xx_tx_bd *bd,
>> + u32 mpdu_header_len,
>> + u32 len);
>> +int wcn36xx_start_tx(struct wcn36xx *wcn,
>> + struct wcn_sta *sta_priv,
>> + struct sk_buff *skb);
>> +
>> #endif /* _TXRX_H_ */
>> diff --git a/wcn36xx.h b/wcn36xx.h
>> index 78a0f7b..89ffc2f 100644
>> --- a/wcn36xx.h
>> +++ b/wcn36xx.h
>> @@ -27,6 +27,7 @@
>>
>> #include "hal.h"
>> #include "smd.h"
>> +#include "txrx.h"
>> #include "dxe.h"
>>
>> #define DRIVER_PREFIX "wcn36xx: "
>> --
>> 1.7.11.3
>>
>
>
>
> --
> Best regards,
> Eugene
>
> _______________________________________________
> wcn36xx mailing list
> wcn36xx at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/wcn36xx
More information about the wcn36xx
mailing list