2014-03-21 09:34:20

by Janusz Dziedzic

[permalink] [raw]
Subject: [PATCH 1/2] ath10k: introduce rx_status htt template

Introduce rx_status htt template instead of
stack version.

Signed-off-by: Janusz Dziedzic <[email protected]>
---
Base on:
ath10k: finally kill htt_rx_info

drivers/net/wireless/ath/ath10k/htt.h | 3 +++
drivers/net/wireless/ath/ath10k/htt_rx.c | 36 +++++++++++++++---------------
2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index 2d669c0..645a563 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -1273,6 +1273,9 @@ struct ath10k_htt {
struct tasklet_struct txrx_compl_task;
struct sk_buff_head tx_compl_q;
struct sk_buff_head rx_compl_q;
+
+ /* rx_status template */
+ struct ieee80211_rx_status rx_status;
};

#define RX_HTT_HDR_STATUS_LEN 64
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 1bb3c5e..4f1cc67 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1139,7 +1139,7 @@ static bool ath10k_htt_rx_amsdu_allowed(struct ath10k_htt *htt,
static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
struct htt_rx_indication *rx)
{
- struct ieee80211_rx_status rx_status;
+ struct ieee80211_rx_status *rx_status = &htt->rx_status;
struct htt_rx_indication_mpdu_range *mpdu_ranges;
struct htt_rx_desc *rxd;
enum htt_rx_mpdu_status status;
@@ -1154,7 +1154,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,

lockdep_assert_held(&htt->rx_ring.lock);

- memset(&rx_status, 0, sizeof(rx_status));
+ memset(rx_status, 0, sizeof(*rx_status));

fw_desc_len = __le16_to_cpu(rx->prefix.fw_rx_desc_bytes);
fw_desc = (u8 *)&rx->fw_desc;
@@ -1164,23 +1164,23 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
mpdu_ranges = htt_rx_ind_get_mpdu_ranges(rx);

/* Fill this once, while this is per-ppdu */
- rx_status.signal = ATH10K_DEFAULT_NOISE_FLOOR;
- rx_status.signal += rx->ppdu.combined_rssi;
+ rx_status->signal = ATH10K_DEFAULT_NOISE_FLOOR;
+ rx_status->signal += rx->ppdu.combined_rssi;

if (rx->ppdu.info0 & HTT_RX_INDICATION_INFO0_END_VALID) {
/* TSF available only in 32-bit */
- rx_status.mactime = __le32_to_cpu(rx->ppdu.tsf) & 0xffffffff;
- rx_status.flag |= RX_FLAG_MACTIME_END;
+ rx_status->mactime = __le32_to_cpu(rx->ppdu.tsf) & 0xffffffff;
+ rx_status->flag |= RX_FLAG_MACTIME_END;
}

- channel_set = ath10k_htt_rx_h_channel(htt->ar, &rx_status);
+ channel_set = ath10k_htt_rx_h_channel(htt->ar, rx_status);

if (channel_set) {
- ath10k_htt_rx_h_rates(htt->ar, rx_status.band,
+ ath10k_htt_rx_h_rates(htt->ar, rx_status->band,
rx->ppdu.info0,
__le32_to_cpu(rx->ppdu.info1),
__le32_to_cpu(rx->ppdu.info2),
- &rx_status);
+ rx_status);
}

ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "htt rx ind: ",
@@ -1229,21 +1229,21 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
}

if (attention & RX_ATTENTION_FLAGS_FCS_ERR)
- rx_status.flag |= RX_FLAG_FAILED_FCS_CRC;
+ rx_status->flag |= RX_FLAG_FAILED_FCS_CRC;
else
- rx_status.flag &= ~RX_FLAG_FAILED_FCS_CRC;
+ rx_status->flag &= ~RX_FLAG_FAILED_FCS_CRC;

if (attention & RX_ATTENTION_FLAGS_TKIP_MIC_ERR)
- rx_status.flag |= RX_FLAG_MMIC_ERROR;
+ rx_status->flag |= RX_FLAG_MMIC_ERROR;
else
- rx_status.flag &= ~RX_FLAG_MMIC_ERROR;
+ rx_status->flag &= ~RX_FLAG_MMIC_ERROR;

hdr = ath10k_htt_rx_skb_get_hdr(msdu_head);

if (ath10k_htt_rx_hdr_is_amsdu(hdr))
- ath10k_htt_rx_amsdu(htt, &rx_status, msdu_head);
+ ath10k_htt_rx_amsdu(htt, rx_status, msdu_head);
else
- ath10k_htt_rx_msdu(htt, &rx_status, msdu_head);
+ ath10k_htt_rx_msdu(htt, rx_status, msdu_head);
}
}

@@ -1257,7 +1257,7 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
enum htt_rx_mpdu_encrypt_type enctype;
struct htt_rx_desc *rxd;
enum rx_msdu_decap_format fmt;
- struct ieee80211_rx_status rx_status = {};
+ struct ieee80211_rx_status *rx_status = &htt->rx_status;
struct ieee80211_hdr *hdr;
int ret;
bool tkip_mic_err;
@@ -1305,7 +1305,7 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,

enctype = MS(__le32_to_cpu(rxd->mpdu_start.info0),
RX_MPDU_START_INFO0_ENCRYPT_TYPE);
- ath10k_htt_rx_h_protected(htt, &rx_status, msdu_head, enctype);
+ ath10k_htt_rx_h_protected(htt, rx_status, msdu_head, enctype);
msdu_head->ip_summed = ath10k_htt_rx_get_csum_state(msdu_head);

if (tkip_mic_err)
@@ -1350,7 +1350,7 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,

ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "htt rx frag mpdu: ",
msdu_head->data, msdu_head->len);
- ath10k_process_rx(htt->ar, &rx_status, msdu_head);
+ ath10k_process_rx(htt->ar, rx_status, msdu_head);

end:
if (fw_desc_len > 0) {
--
1.7.9.5



2014-03-24 10:16:56

by Janusz Dziedzic

[permalink] [raw]
Subject: Re: [PATCH 1/2] ath10k: introduce rx_status htt template

On 24 March 2014 11:06, Kalle Valo <[email protected]> wrote:
> Janusz Dziedzic <[email protected]> writes:
>
>> Introduce rx_status htt template instead of
>> stack version.
>>
>> Signed-off-by: Janusz Dziedzic <[email protected]>
>
> Why? What's the benefit from doing this?
>
This is the must in case we would like to report rssi + rates correctly.
I describe this in 2/2 - maybe squash them?
Currently during heavy traffic we could have:
> 10 packets with START_VALID - correct RSSI + rates
> 10 packets with END_VALID
> 10 packets with START_VALID - correct RSSI + rates
> 10 packets with END_VALID -
...

Because of that we have to store rssi + rates for case when END_VALID only.
In other case when using monitor we will see:
- 10 packets with correct rssi/rates
- 10 packets with rssi=-95/rate=6Mbps

BR
Janusz

2014-03-24 10:47:52

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/2] ath10k: introduce rx_status htt template

Janusz Dziedzic <[email protected]> writes:

> On 24 March 2014 11:06, Kalle Valo <[email protected]> wrote:
>> Janusz Dziedzic <[email protected]> writes:
>>
>>> Introduce rx_status htt template instead of
>>> stack version.
>>>
>>> Signed-off-by: Janusz Dziedzic <[email protected]>
>>
>> Why? What's the benefit from doing this?
>>
> This is the must in case we would like to report rssi + rates correctly.
> I describe this in 2/2 - maybe squash them?

Nah, better to keep them separated. But in patch 1 please document why
you are doing that, so that I (and others cherry picking ath10k patches)
don't need to guess. For example, you can say "in preparation for foo"
or something like that.

> Currently during heavy traffic we could have:
>> 10 packets with START_VALID - correct RSSI + rates
>> 10 packets with END_VALID
>> 10 packets with START_VALID - correct RSSI + rates
>> 10 packets with END_VALID -
> ...
>
> Because of that we have to store rssi + rates for case when END_VALID only.
> In other case when using monitor we will see:
> - 10 packets with correct rssi/rates
> - 10 packets with rssi=-95/rate=6Mbps

Ok, I understand now. Please document that in patch 2.

And in the future, please double check that your commit logs answer the
question "why?". That will save everyone's time :)

--
Kalle Valo

2014-03-21 09:34:21

by Janusz Dziedzic

[permalink] [raw]
Subject: [PATCH 2/2] ath10k: fix rssi and rate reporting

RSSI and RATE fields are valid only when START_VALID
bit is set. So, in current implementation we have to
remember/caclulate them when START_VALID and report the same
when only END_VALID is set. In other case during high
RX traffic we will report invalid RSSI (-95) and RATE (6Mbps)
when only END_VALID bit set.

Signed-off-by: Janusz Dziedzic <[email protected]>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 4f1cc67..5a4310f 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1154,8 +1154,6 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,

lockdep_assert_held(&htt->rx_ring.lock);

- memset(rx_status, 0, sizeof(*rx_status));
-
fw_desc_len = __le16_to_cpu(rx->prefix.fw_rx_desc_bytes);
fw_desc = (u8 *)&rx->fw_desc;

@@ -1164,8 +1162,11 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
mpdu_ranges = htt_rx_ind_get_mpdu_ranges(rx);

/* Fill this once, while this is per-ppdu */
- rx_status->signal = ATH10K_DEFAULT_NOISE_FLOOR;
- rx_status->signal += rx->ppdu.combined_rssi;
+ if (rx->ppdu.info0 & HTT_RX_INDICATION_INFO0_START_VALID) {
+ memset(rx_status, 0, sizeof(*rx_status));
+ rx_status->signal = ATH10K_DEFAULT_NOISE_FLOOR +
+ rx->ppdu.combined_rssi;
+ }

if (rx->ppdu.info0 & HTT_RX_INDICATION_INFO0_END_VALID) {
/* TSF available only in 32-bit */
--
1.7.9.5


2014-03-24 10:08:40

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 2/2] ath10k: fix rssi and rate reporting

Janusz Dziedzic <[email protected]> writes:

> RSSI and RATE fields are valid only when START_VALID
> bit is set. So, in current implementation we have to
> remember/caclulate them when START_VALID and report the same
> when only END_VALID is set. In other case during high
> RX traffic we will report invalid RSSI (-95) and RATE (6Mbps)
> when only END_VALID bit set.
>
> Signed-off-by: Janusz Dziedzic <[email protected]>

It's not clear for me what (user visible) bug you are fixing, please
describe that in the commit log.

--
Kalle Valo

2014-03-24 10:06:40

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH 1/2] ath10k: introduce rx_status htt template

Janusz Dziedzic <[email protected]> writes:

> Introduce rx_status htt template instead of
> stack version.
>
> Signed-off-by: Janusz Dziedzic <[email protected]>

Why? What's the benefit from doing this?

--
Kalle Valo