[PATCH] Refactor TX path
Eugene Krasnikov
k.eugene.e at gmail.com
Mon Jul 8 04:23:28 EDT 2013
Updated pull request with fixed comments
https://github.com/KrasnikovEugene/wcn36xx/pull/71
2013/7/6 Olof Johansson <dev at skyshaper.net>:
> 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
--
Best regards,
Eugene
More information about the wcn36xx
mailing list