2014-07-28 06:26:33

by Janusz Dziedzic

[permalink] [raw]
Subject: [PATCH v2] ath10k: extend debug code for RX path

Print sequence number, AMSDU_MORE flag and AC when additional
debug enabled in RX path. This is usefull for debugging purpose.

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

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index d80fcda..f7c8021 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -818,19 +818,55 @@ static bool ath10k_htt_rx_h_channel(struct ath10k *ar,
return true;
}

+static const char * const tid_to_ac[] = {
+ "BE",
+ "BK",
+ "BK",
+ "BE",
+ "VI",
+ "VI",
+ "VO",
+ "VO",
+};
+
+static char *ath10k_get_tid(struct ieee80211_hdr *hdr, char *out, size_t size)
+{
+ u8 *qc;
+ int tid;
+
+ if (!ieee80211_is_data_qos(hdr->frame_control))
+ return "";
+
+ qc = ieee80211_get_qos_ctl(hdr);
+ tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
+ if (tid < 8)
+ snprintf(out, size, "tid %d (%s)", tid, tid_to_ac[tid]);
+ else
+ snprintf(out, size, "tid %d", tid);
+
+ return out;
+}
+
static void ath10k_process_rx(struct ath10k *ar,
struct ieee80211_rx_status *rx_status,
struct sk_buff *skb)
{
struct ieee80211_rx_status *status;
+ struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+ char tid[32];

status = IEEE80211_SKB_RXCB(skb);
*status = *rx_status;

ath10k_dbg(ATH10K_DBG_DATA,
- "rx skb %p len %u %s%s%s%s%s %srate_idx %u vht_nss %u freq %u band %u flag 0x%x fcs-err %imic-err %i\n",
+ "rx skb %p len %u peer %pM %s %s sn %u %s%s%s%s%s %srate_idx %u vht_nss %u freq %u band %u flag 0x%x fcs-err %i mic-err %i amsdu-more %i\n",
skb,
skb->len,
+ ieee80211_get_SA(hdr),
+ ath10k_get_tid(hdr, tid, sizeof(tid)),
+ is_multicast_ether_addr(ieee80211_get_DA(hdr)) ?
+ "mcast" : "ucast",
+ (__le16_to_cpu(hdr->seq_ctrl) & IEEE80211_SCTL_SEQ) >> 4,
status->flag == 0 ? "legacy" : "",
status->flag & RX_FLAG_HT ? "ht" : "",
status->flag & RX_FLAG_VHT ? "vht" : "",
@@ -842,7 +878,8 @@ static void ath10k_process_rx(struct ath10k *ar,
status->freq,
status->band, status->flag,
!!(status->flag & RX_FLAG_FAILED_FCS_CRC),
- !!(status->flag & RX_FLAG_MMIC_ERROR));
+ !!(status->flag & RX_FLAG_MMIC_ERROR),
+ !!(status->flag & RX_FLAG_AMSDU_MORE));
ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "rx skb: ",
skb->data, skb->len);

--
1.7.9.5



2014-07-28 08:07:22

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: extend debug code for RX path

Janusz Dziedzic <[email protected]> writes:

> On 28 July 2014 08:35, Varka Bhadram <[email protected]> wrote:
>> On 07/28/2014 11:56 AM, Janusz Dziedzic wrote:
>>>
>>> Print sequence number, AMSDU_MORE flag and AC when additional
>>> debug enabled in RX path. This is usefull for debugging purpose.
>>>
>>> Signed-off-by: Janusz Dziedzic<[email protected]>
>>> ---
>>> drivers/net/wireless/ath/ath10k/htt_rx.c | 41
>>> ++++++++++++++++++++++++++++--
>>> 1 file changed, 39 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> b/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> index d80fcda..f7c8021 100644
>>> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> @@ -818,19 +818,55 @@ static bool ath10k_htt_rx_h_channel(struct ath10k
>>> *ar,
>>> return true;
>>> }
>>> +static const char * const tid_to_ac[] = {
>>
>>
>> No space required '*' and const.
>
> Seems checkpatch didn't find this.

>From a quick grep I saw that kernel has both "* const" and "*const". I
would say keep it as it is.

>>> +static char *ath10k_get_tid(struct ieee80211_hdr *hdr, char *out, size_t
>>> size)
>>> +{
>>> + u8 *qc;
>>> + int tid;
>>> +
>>> + if (!ieee80211_is_data_qos(hdr->frame_control))
>>> + return "";
>>
>>
>> return NULL would be more readable...?
>>
> We don't need extra "(null)" in printf, empty string is what I expect
> here (same as other params in this dbg info).
> Anyway I can return "out" here and initialize as empty before calling
> ath10k_get_tid().

IMHO return "" is more readable here.

>>> + is_multicast_ether_addr(ieee80211_get_DA(hdr)) ?
>>> + "mcast" : "ucast",
>>
>>
>> should match open paranthesis...
>>
>> is_multicast_ether_addr(ieee80211_get_DA(hdr)) ?
>>
>> "mcast" : "ucast",
>>
> Could you describe this more (I didn't find clear description about this)?

AFAICS there is no open parenthesis here to which align the indentation.

--
Kalle Valo

2014-07-29 09:48:54

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: extend debug code for RX path

Janusz Dziedzic <[email protected]> writes:

> Print sequence number, AMSDU_MORE flag and AC when additional
> debug enabled in RX path. This is usefull for debugging purpose.
>
> Signed-off-by: Janusz Dziedzic <[email protected]>

Thanks, applied.

--
Kalle Valo

2014-07-28 07:52:31

by Janusz Dziedzic

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: extend debug code for RX path

On 28 July 2014 08:35, Varka Bhadram <[email protected]> wrote:
> On 07/28/2014 11:56 AM, Janusz Dziedzic wrote:
>>
>> Print sequence number, AMSDU_MORE flag and AC when additional
>> debug enabled in RX path. This is usefull for debugging purpose.
>>
>> Signed-off-by: Janusz Dziedzic<[email protected]>
>> ---
>> drivers/net/wireless/ath/ath10k/htt_rx.c | 41
>> ++++++++++++++++++++++++++++--
>> 1 file changed, 39 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c
>> b/drivers/net/wireless/ath/ath10k/htt_rx.c
>> index d80fcda..f7c8021 100644
>> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
>> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
>> @@ -818,19 +818,55 @@ static bool ath10k_htt_rx_h_channel(struct ath10k
>> *ar,
>> return true;
>> }
>> +static const char * const tid_to_ac[] = {
>
>
> No space required '*' and const.
>
Seems checkpatch didn't find this.

> static const char *const tid_to_ac[] = {
>
>
>
>> + "BE",
>> + "BK",
>> + "BK",
>> + "BE",
>> + "VI",
>> + "VI",
>> + "VO",
>> + "VO",
>> +};
>> +
>> +static char *ath10k_get_tid(struct ieee80211_hdr *hdr, char *out, size_t
>> size)
>> +{
>> + u8 *qc;
>> + int tid;
>> +
>> + if (!ieee80211_is_data_qos(hdr->frame_control))
>> + return "";
>
>
> return NULL would be more readable...?
>
We don't need extra "(null)" in printf, empty string is what I expect
here (same as other params in this dbg info).
Anyway I can return "out" here and initialize as empty before calling
ath10k_get_tid().

>
>> +
>> + qc = ieee80211_get_qos_ctl(hdr);
>> + tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
>> + if (tid < 8)
>> + snprintf(out, size, "tid %d (%s)", tid, tid_to_ac[tid]);
>> + else
>> + snprintf(out, size, "tid %d", tid);
>> +
>> + return out;
>> +}
>> +
>> static void ath10k_process_rx(struct ath10k *ar,
>> struct ieee80211_rx_status *rx_status,
>> struct sk_buff *skb)
>> {
>> struct ieee80211_rx_status *status;
>> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>> + char tid[32];
>> status = IEEE80211_SKB_RXCB(skb);
>> *status = *rx_status;
>> ath10k_dbg(ATH10K_DBG_DATA,
>> - "rx skb %p len %u %s%s%s%s%s %srate_idx %u vht_nss %u
>> freq %u band %u flag 0x%x fcs-err %imic-err %i\n",
>> + "rx skb %p len %u peer %pM %s %s sn %u %s%s%s%s%s
>> %srate_idx %u vht_nss %u freq %u band %u flag 0x%x fcs-err %i mic-err %i
>> amsdu-more %i\n",
>> skb,
>> skb->len,
>> + ieee80211_get_SA(hdr),
>> + ath10k_get_tid(hdr, tid, sizeof(tid)),
>> + is_multicast_ether_addr(ieee80211_get_DA(hdr)) ?
>> + "mcast" : "ucast",
>
>
> should match open paranthesis...
>
> is_multicast_ether_addr(ieee80211_get_DA(hdr)) ?
>
> "mcast" : "ucast",
>
Could you describe this more (I didn't find clear description about this)?

BR
Janusz

2014-07-28 06:36:56

by Varka Bhadram

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: extend debug code for RX path

On 07/28/2014 11:56 AM, Janusz Dziedzic wrote:
> Print sequence number, AMSDU_MORE flag and AC when additional
> debug enabled in RX path. This is usefull for debugging purpose.
>
> Signed-off-by: Janusz Dziedzic<[email protected]>
> ---
> drivers/net/wireless/ath/ath10k/htt_rx.c | 41 ++++++++++++++++++++++++++++--
> 1 file changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
> index d80fcda..f7c8021 100644
> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
> @@ -818,19 +818,55 @@ static bool ath10k_htt_rx_h_channel(struct ath10k *ar,
> return true;
> }
>
> +static const char * const tid_to_ac[] = {

No space required '*' and const.

static const char *const tid_to_ac[] = {


> + "BE",
> + "BK",
> + "BK",
> + "BE",
> + "VI",
> + "VI",
> + "VO",
> + "VO",
> +};
> +
> +static char *ath10k_get_tid(struct ieee80211_hdr *hdr, char *out, size_t size)
> +{
> + u8 *qc;
> + int tid;
> +
> + if (!ieee80211_is_data_qos(hdr->frame_control))
> + return "";

return NULL would be more readable...?

> +
> + qc = ieee80211_get_qos_ctl(hdr);
> + tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
> + if (tid < 8)
> + snprintf(out, size, "tid %d (%s)", tid, tid_to_ac[tid]);
> + else
> + snprintf(out, size, "tid %d", tid);
> +
> + return out;
> +}
> +
> static void ath10k_process_rx(struct ath10k *ar,
> struct ieee80211_rx_status *rx_status,
> struct sk_buff *skb)
> {
> struct ieee80211_rx_status *status;
> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> + char tid[32];
>
> status = IEEE80211_SKB_RXCB(skb);
> *status = *rx_status;
>
> ath10k_dbg(ATH10K_DBG_DATA,
> - "rx skb %p len %u %s%s%s%s%s %srate_idx %u vht_nss %u freq %u band %u flag 0x%x fcs-err %imic-err %i\n",
> + "rx skb %p len %u peer %pM %s %s sn %u %s%s%s%s%s %srate_idx %u vht_nss %u freq %u band %u flag 0x%x fcs-err %i mic-err %i amsdu-more %i\n",
> skb,
> skb->len,
> + ieee80211_get_SA(hdr),
> + ath10k_get_tid(hdr, tid, sizeof(tid)),
> + is_multicast_ether_addr(ieee80211_get_DA(hdr)) ?
> + "mcast" : "ucast",

should match open paranthesis...

is_multicast_ether_addr(ieee80211_get_DA(hdr)) ?
"mcast" : "ucast",

> + (__le16_to_cpu(hdr->seq_ctrl) & IEEE80211_SCTL_SEQ) >> 4,
> status->flag == 0 ? "legacy" : "",
> status->flag & RX_FLAG_HT ? "ht" : "",
> status->flag & RX_FLAG_VHT ? "vht" : "",
> @@ -842,7 +878,8 @@ static void ath10k_process_rx(struct ath10k *ar,
> status->freq,
> status->band, status->flag,
> !!(status->flag & RX_FLAG_FAILED_FCS_CRC),
> - !!(status->flag & RX_FLAG_MMIC_ERROR));
> + !!(status->flag & RX_FLAG_MMIC_ERROR),
> + !!(status->flag & RX_FLAG_AMSDU_MORE));
> ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "rx skb: ",
> skb->data, skb->len);
>


--
Regards,
Varka Bhadram.