[PATCH 1/4] ath10k: Print out firmware feature bits from IE.

[PATCH 1/4] ath10k: Print out firmware feature bits from IE.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Aids in understanding excactly what a firmware is
offering.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 3b59af3..56048b1 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -470,8 +470,12 @@ static int ath10k_core_fetch_firmware_api_n(struct ath10k *ar, const char *name)
 				if (index == ie_len)
 					break;
 
-				if (data[index] & (1 << bit))
+				if (data[index] & (1 << bit)) {
+					ath10k_dbg(ATH10K_DBG_BOOT,
+						   "Enabling feature bit: %i\n",
+						   i);
 					__set_bit(i, ar->fw_features);
+				}
 			}
 
 			ath10k_dbg_dump(ATH10K_DBG_BOOT, "features", "",
-- 
1.7.11.7


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 2/4] ath10k: Print out size of wmi-ready-event message.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/wmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 712a606..c99bca6 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -2031,11 +2031,11 @@ static int ath10k_wmi_ready_event_rx(struct ath10k *ar, struct sk_buff *skb)
 	memcpy(ar->mac_addr, ev->mac_addr.addr, ETH_ALEN);
 
 	ath10k_dbg(ATH10K_DBG_WMI,
-		   "wmi event ready sw_version %u abi_version %u mac_addr %pM status %d\n",
+		   "wmi event ready sw_version %u abi_version %u mac_addr %pM status %d  skb->len: %i  ev-sz: %lu\n",
 		   __le32_to_cpu(ev->sw_version),
 		   __le32_to_cpu(ev->abi_version),
 		   ev->mac_addr.addr,
-		   __le32_to_cpu(ev->status));
+		   __le32_to_cpu(ev->status), skb->len, sizeof(*ev));
 
 	complete(&ar->wmi.unified_ready);
 	return 0;
-- 
1.7.11.7


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 3/4] ath10k: Add more debugging for receive errors.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 8 ++++++++
 drivers/net/wireless/ath/ath10k/txrx.c   | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index fe8bd1b..49e6c4e 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -937,6 +937,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 			}
 
 			if (ath10k_htt_rx_has_decrypt_err(msdu_head)) {
+				ath10k_warn("htt rx dropping due to decrypt-err\n");
 				ath10k_htt_rx_free_msdu_chain(msdu_head);
 				continue;
 			}
@@ -975,6 +976,13 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 			info.skb     = msdu_head;
 			info.fcs_err = ath10k_htt_rx_has_fcs_err(msdu_head);
 			info.mic_err = ath10k_htt_rx_has_mic_err(msdu_head);
+
+			if (info.fcs_err)
+				ath10k_warn("htt rx has FCS err\n");
+
+			if (info.mic_err)
+				ath10k_warn("htt rx has MIC err\n");
+
 			info.signal  = ATH10K_DEFAULT_NOISE_FLOOR;
 			info.signal += rx->ppdu.combined_rssi;
 
diff --git a/drivers/net/wireless/ath/ath10k/txrx.c b/drivers/net/wireless/ath/ath10k/txrx.c
index e404124..fc3a651 100644
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -259,7 +259,7 @@ void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info)
 	status->freq = ch->center_freq;
 
 	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\n",
+		   "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: %i\n",
 		   info->skb,
 		   info->skb->len,
 		   status->flag == 0 ? "legacy" : "",
@@ -271,7 +271,7 @@ void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info)
 		   status->rate_idx,
 		   status->vht_nss,
 		   status->freq,
-		   status->band);
+		   status->band, status->flag, info->fcs_err);
 	ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "rx skb: ",
 			info->skb->data, info->skb->len);
 
-- 
1.7.11.7


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 4/4] ath10k: Better tx/rx debugging.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Make it easier to grep for htt rx errors.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/htt_rx.c | 18 ++++++++++--------
 drivers/net/wireless/ath/ath10k/htt_tx.c |  4 ++--
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 49e6c4e..7b0b51f 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -324,7 +324,7 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 				 msdu->len + skb_tailroom(msdu),
 				 DMA_FROM_DEVICE);
 
-		ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "htt rx: ",
+		ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "htt rx pop: ",
 				msdu->data, msdu->len + skb_tailroom(msdu));
 
 		rx_desc = (struct htt_rx_desc *)msdu->data;
@@ -417,8 +417,8 @@ static int ath10k_htt_rx_amsdu_pop(struct ath10k_htt *htt,
 					 next->len + skb_tailroom(next),
 					 DMA_FROM_DEVICE);
 
-			ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "htt rx: ",
-					next->data,
+			ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL,
+					"htt rx chained: ", next->data,
 					next->len + skb_tailroom(next));
 
 			skb_trim(next, 0);
@@ -751,7 +751,7 @@ static void ath10k_htt_rx_msdu(struct ath10k_htt *htt, struct htt_rx_info *info)
 
 	/* This shouldn't happen. If it does than it may be a FW bug. */
 	if (skb->next) {
-		ath10k_warn("received chained non A-MSDU frame\n");
+		ath10k_warn("htt rx: received chained non A-MSDU frame\n");
 		ath10k_htt_rx_free_msdu_chain(skb->next);
 		skb->next = NULL;
 	}
@@ -937,7 +937,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 			}
 
 			if (ath10k_htt_rx_has_decrypt_err(msdu_head)) {
-				ath10k_warn("htt rx dropping due to decrypt-err\n");
+				ath10k_warn("htt rx dropping due to decrypt error\n");
 				ath10k_htt_rx_free_msdu_chain(msdu_head);
 				continue;
 			}
@@ -946,6 +946,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 
 			/* Skip mgmt frames while we handle this in WMI */
 			if (status == HTT_RX_IND_MPDU_STATUS_MGMT_CTRL) {
+				ath10k_dbg(ATH10K_DBG_HTT, "htt rx mgmt ctrl\n");
 				ath10k_htt_rx_free_msdu_chain(msdu_head);
 				continue;
 			}
@@ -961,6 +962,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 			}
 
 			if (test_bit(ATH10K_CAC_RUNNING, &htt->ar->dev_flags)) {
+				ath10k_warn("htt rx: CAC running\n");
 				ath10k_htt_rx_free_msdu_chain(msdu_head);
 				continue;
 			}
@@ -968,7 +970,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
 			/* FIXME: we do not support chaining yet.
 			 * this needs investigation */
 			if (msdu_chaining) {
-				ath10k_warn("msdu_chaining is true\n");
+				ath10k_warn("htt rx: msdu_chaining is true\n");
 				ath10k_htt_rx_free_msdu_chain(msdu_head);
 				continue;
 			}
@@ -1103,7 +1105,7 @@ static void ath10k_htt_rx_frag_handler(struct ath10k_htt *htt,
 
 	skb_trim(info.skb, info.skb->len - trim);
 
-	ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "htt frag mpdu: ",
+	ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "htt rx frag mpdu: ",
 			info.skb->data, info.skb->len);
 	ath10k_process_rx(htt->ar, &info);
 
@@ -1124,7 +1126,7 @@ void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
 	if (!IS_ALIGNED((unsigned long)skb->data, 4))
 		ath10k_warn("unaligned htt message, expect trouble\n");
 
-	ath10k_dbg(ATH10K_DBG_HTT, "HTT RX, msg_type: 0x%0X\n",
+	ath10k_dbg(ATH10K_DBG_HTT, "htt rx, msg_type: 0x%0X\n",
 		   resp->hdr.msg_type);
 	switch (resp->hdr.msg_type) {
 	case HTT_T2H_MSG_TYPE_VERSION_CONF: {
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index f1d36d2..acaa046 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -460,9 +460,9 @@ int ath10k_htt_tx(struct ath10k_htt *htt, struct sk_buff *msdu)
 					   DMA_TO_DEVICE);
 	}
 
-	ath10k_dbg(ATH10K_DBG_HTT, "msdu 0x%llx\n",
+	ath10k_dbg(ATH10K_DBG_HTT, "tx-msdu 0x%llx\n",
 		   (unsigned long long) ATH10K_SKB_CB(msdu)->paddr);
-	ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "msdu: ",
+	ath10k_dbg_dump(ATH10K_DBG_HTT_DUMP, NULL, "tx-msdu: ",
 			msdu->data, msdu->len);
 
 	skb_put(txdesc, desc_len);
-- 
1.7.11.7


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/4] ath10k: Print out size of wmi-ready-event message.

[Kalle Valo]

greearb@candelatech.com writes:

> From: Ben Greear <greearb@candelatech.com>
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>

I don't like empty commit messages. Just write even a short description
why the patch is needed.

And I guess you saw the compiler warning from the build bot for this
patch?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/4] ath10k: Print out size of wmi-ready-event message.

[Ben Greear]

On 01/29/2014 05:16 AM, Kalle Valo wrote:
> greearb@candelatech.com writes:
> 
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
> 
> I don't like empty commit messages. Just write even a short description
> why the patch is needed.
> 
> And I guess you saw the compiler warning from the build bot for this
> patch?

Actually, if it's convenient, just drop this patch.  It was to debug
what turned out to be improperly packaged firmware, and there is little
chance of ever hitting that in the field.

If instead you need me to fix the compile warning, I'll send an updated
patch.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/4] ath10k: Print out size of wmi-ready-event message.

[Kalle Valo]

Ben Greear <greearb@candelatech.com> writes:

> On 01/29/2014 05:16 AM, Kalle Valo wrote:
>> greearb@candelatech.com writes:
>
>> And I guess you saw the compiler warning from the build bot for this
>> patch?
>
> Actually, if it's convenient, just drop this patch.  It was to debug
> what turned out to be improperly packaged firmware, and there is little
> chance of ever hitting that in the field.
>
> If instead you need me to fix the compile warning, I'll send an updated
> patch.

Up to you. I was planning to take the patch, but had to drop it because
of the warning.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/4] ath10k: Print out firmware feature bits from IE.

[Kalle Valo]

greearb@candelatech.com writes:

> From: Ben Greear <greearb@candelatech.com>
>
> Aids in understanding excactly what a firmware is
> offering.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>

Patch 1 applied, still reviewing the rest.

> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -470,8 +470,12 @@ static int ath10k_core_fetch_firmware_api_n(struct ath10k *ar, const char *name)
>  				if (index == ie_len)
>  					break;
>  
> -				if (data[index] & (1 << bit))
> +				if (data[index] & (1 << bit)) {
> +					ath10k_dbg(ATH10K_DBG_BOOT,
> +						   "Enabling feature bit: %i\n",
> +						   i);
>  					__set_bit(i, ar->fw_features);
> +				}
>  			}

I was actually hoping to have something better like ath6kl has which
prints a name for the feature:

	{ ATH6KL_FW_CAPABILITY_HOST_P2P, "host-p2p" },
	{ ATH6KL_FW_CAPABILITY_SCHED_SCAN, "sched-scan" },
	{ ATH6KL_FW_CAPABILITY_STA_P2PDEV_DUPLEX, "sta-p2pdev-duplex" },
	{ ATH6KL_FW_CAPABILITY_INACTIVITY_TIMEOUT, "inactivity-timeout" },
	{ ATH6KL_FW_CAPABILITY_RSN_CAP_OVERRIDE, "rsn-cap-override" },
	{ ATH6KL_FW_CAPABILITY_WOW_MULTICAST_FILTER, "wow-mc-filter" },

But this is something for the future.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 3/4] ath10k: Add more debugging for receive errors.

[Kalle Valo]

greearb@candelatech.com writes:

> From: Ben Greear <greearb@candelatech.com>
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>

[...]

> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
> @@ -937,6 +937,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
>  			}
>  
>  			if (ath10k_htt_rx_has_decrypt_err(msdu_head)) {
> +				ath10k_warn("htt rx dropping due to decrypt-err\n");
>  				ath10k_htt_rx_free_msdu_chain(msdu_head);
>  				continue;
>  			}
> @@ -975,6 +976,13 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
>  			info.skb     = msdu_head;
>  			info.fcs_err = ath10k_htt_rx_has_fcs_err(msdu_head);
>  			info.mic_err = ath10k_htt_rx_has_mic_err(msdu_head);
> +
> +			if (info.fcs_err)
> +				ath10k_warn("htt rx has FCS err\n");
> +
> +			if (info.mic_err)
> +				ath10k_warn("htt rx has MIC err\n");
> +

Do we really want to print warning messages to the user when these
happen? I would consider these as "business as usual" in 802.11 world,
not a problem in driver.

> --- a/drivers/net/wireless/ath/ath10k/txrx.c
> +++ b/drivers/net/wireless/ath/ath10k/txrx.c
> @@ -259,7 +259,7 @@ void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info)
>  	status->freq = ch->center_freq;
>  
>  	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\n",
> +		   "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: %i\n",
>  		   info->skb,
>  		   info->skb->len,
>  		   status->flag == 0 ? "legacy" : "",
> @@ -271,7 +271,7 @@ void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info)
>  		   status->rate_idx,
>  		   status->vht_nss,
>  		   status->freq,
> -		   status->band);
> +		   status->band, status->flag, info->fcs_err);

This makes sense, but please use the style used in ath10k debug messages:

"... band %u flag 0x%x fcs-err %i\n"

And as status->flag is u32, maybe 0x%08x makes it easier to read and
compare?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 4/4] ath10k: Better tx/rx debugging.

[Kalle Valo]

greearb@candelatech.com writes:

> From: Ben Greear <greearb@candelatech.com>
>
> Make it easier to grep for htt rx errors.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>

[...]

> @@ -961,6 +962,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
>  			}
>  
>  			if (test_bit(ATH10K_CAC_RUNNING, &htt->ar->dev_flags)) {
> +				ath10k_warn("htt rx: CAC running\n");
>  				ath10k_htt_rx_free_msdu_chain(msdu_head);
>  				continue;
>  			}

I'm a bit worried about this one. Do we guarantee that we will never
receive frames while CAC is running? In other words, is this a normal
event or really an error in the driver which requires a warning message?

Janusz, what do you think?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 3/4] ath10k: Add more debugging for receive errors.

[Ben Greear]

On 02/04/2014 10:51 AM, Kalle Valo wrote:
> greearb@candelatech.com writes:
> 
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
> 
> [...]
> 
>> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
>> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
>> @@ -937,6 +937,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
>>  			}
>>  
>>  			if (ath10k_htt_rx_has_decrypt_err(msdu_head)) {
>> +				ath10k_warn("htt rx dropping due to decrypt-err\n");
>>  				ath10k_htt_rx_free_msdu_chain(msdu_head);
>>  				continue;
>>  			}
>> @@ -975,6 +976,13 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
>>  			info.skb     = msdu_head;
>>  			info.fcs_err = ath10k_htt_rx_has_fcs_err(msdu_head);
>>  			info.mic_err = ath10k_htt_rx_has_mic_err(msdu_head);
>> +
>> +			if (info.fcs_err)
>> +				ath10k_warn("htt rx has FCS err\n");
>> +
>> +			if (info.mic_err)
>> +				ath10k_warn("htt rx has MIC err\n");
>> +
> 
> Do we really want to print warning messages to the user when these
> happen? I would consider these as "business as usual" in 802.11 world,
> not a problem in driver.

Maybe I should make them a debug message instead of warning message?  That
way they are only shown if the user enables debugging?

>> --- a/drivers/net/wireless/ath/ath10k/txrx.c
>> +++ b/drivers/net/wireless/ath/ath10k/txrx.c
>> @@ -259,7 +259,7 @@ void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info)
>>  	status->freq = ch->center_freq;
>>  
>>  	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\n",
>> +		   "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: %i\n",
>>  		   info->skb,
>>  		   info->skb->len,
>>  		   status->flag == 0 ? "legacy" : "",
>> @@ -271,7 +271,7 @@ void ath10k_process_rx(struct ath10k *ar, struct htt_rx_info *info)
>>  		   status->rate_idx,
>>  		   status->vht_nss,
>>  		   status->freq,
>> -		   status->band);
>> +		   status->band, status->flag, info->fcs_err);
> 
> This makes sense, but please use the style used in ath10k debug messages:
> 
> "... band %u flag 0x%x fcs-err %i\n"
> 
> And as status->flag is u32, maybe 0x%08x makes it easier to read and
> compare?

Sure, can do that.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 4/4] ath10k: Better tx/rx debugging.

[Marek Puzyniak]

On 4 February 2014 19:55, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> greearb@candelatech.com writes:
>
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Make it easier to grep for htt rx errors.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>
> [...]
>
>> @@ -961,6 +962,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
>>                       }
>>
>>                       if (test_bit(ATH10K_CAC_RUNNING, &htt->ar->dev_flags)) {
>> +                             ath10k_warn("htt rx: CAC running\n");
>>                               ath10k_htt_rx_free_msdu_chain(msdu_head);
>>                               continue;
>>                       }
>
> I'm a bit worried about this one. Do we guarantee that we will never
> receive frames while CAC is running? In other words, is this a normal
> event or really an error in the driver which requires a warning message?
Actually this happens quite often during CAC. I remember we had some
print there and we got it frequently (let say 10/s).
So probably it would be better to have choice to enable this log print
when someone need this information otherwise by default it should be
disabled.
>
> Janusz, what do you think?
>
> --
> Kalle Valo
>
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 4/4] ath10k: Better tx/rx debugging.

[Kalle Valo]

Marek Puzyniak <marek.puzyniak@tieto.com> writes:

> On 4 February 2014 19:55, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> greearb@candelatech.com writes:
>>
>>> From: Ben Greear <greearb@candelatech.com>
>>>
>>> Make it easier to grep for htt rx errors.
>>>
>>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>>
>> [...]
>>
>>> @@ -961,6 +962,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
>>>                       }
>>>
>>>                       if (test_bit(ATH10K_CAC_RUNNING, &htt->ar->dev_flags)) {
>>> +                             ath10k_warn("htt rx: CAC running\n");
>>>                               ath10k_htt_rx_free_msdu_chain(msdu_head);
>>>                               continue;
>>>                       }
>>
>> I'm a bit worried about this one. Do we guarantee that we will never
>> receive frames while CAC is running? In other words, is this a normal
>> event or really an error in the driver which requires a warning
>> message?
>
> Actually this happens quite often during CAC. I remember we had some
> print there and we got it frequently (let say 10/s).

Yeah, this was exactly what I was wondering.

> So probably it would be better to have choice to enable this log print
> when someone need this information otherwise by default it should be
> disabled.

Maybe a debug message is better?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 3/4] ath10k: Add more debugging for receive errors.

[Kalle Valo]

Ben Greear <greearb@candelatech.com> writes:

>>> @@ -975,6 +976,13 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
>>>  			info.skb     = msdu_head;
>>>  			info.fcs_err = ath10k_htt_rx_has_fcs_err(msdu_head);
>>>  			info.mic_err = ath10k_htt_rx_has_mic_err(msdu_head);
>>> +
>>> +			if (info.fcs_err)
>>> +				ath10k_warn("htt rx has FCS err\n");
>>> +
>>> +			if (info.mic_err)
>>> +				ath10k_warn("htt rx has MIC err\n");
>>> +
>> 
>> Do we really want to print warning messages to the user when these
>> happen? I would consider these as "business as usual" in 802.11 world,
>> not a problem in driver.
>
> Maybe I should make them a debug message instead of warning message?  That
> way they are only shown if the user enables debugging?

Yeah, having them as debug messages makes sense.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 4/4] ath10k: Better tx/rx debugging.

[Marek Puzyniak]

On 5 February 2014 07:29, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Marek Puzyniak <marek.puzyniak@tieto.com> writes:
>
>> On 4 February 2014 19:55, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>>> greearb@candelatech.com writes:
>>>
>>>> From: Ben Greear <greearb@candelatech.com>
>>>>
>>>> Make it easier to grep for htt rx errors.
>>>>
>>>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>>>
>>> [...]
>>>
>>>> @@ -961,6 +962,7 @@ static void ath10k_htt_rx_handler(struct ath10k_htt *htt,
>>>>                       }
>>>>
>>>>                       if (test_bit(ATH10K_CAC_RUNNING, &htt->ar->dev_flags)) {
>>>> +                             ath10k_warn("htt rx: CAC running\n");
>>>>                               ath10k_htt_rx_free_msdu_chain(msdu_head);
>>>>                               continue;
>>>>                       }
>>>
>>> I'm a bit worried about this one. Do we guarantee that we will never
>>> receive frames while CAC is running? In other words, is this a normal
>>> event or really an error in the driver which requires a warning
>>> message?
>>
>> Actually this happens quite often during CAC. I remember we had some
>> print there and we got it frequently (let say 10/s).
>
> Yeah, this was exactly what I was wondering.
>
>> So probably it would be better to have choice to enable this log print
>> when someone need this information otherwise by default it should be
>> disabled.
>
> Maybe a debug message is better?
Yes, for example with flag ATH10K_DBG_HTT.
>
> --
> Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 4/4] ath10k: Better tx/rx debugging.

[Ben Greear]

On 02/05/2014 01:10 AM, Marek Puzyniak wrote:

>>> So probably it would be better to have choice to enable this log print
>>> when someone need this information otherwise by default it should be
>>> disabled.
>>
>> Maybe a debug message is better?
> Yes, for example with flag ATH10K_DBG_HTT.

That one is pretty spammy already..maybe I can create a new flag
ATH10K_DBG_RX or something like that that is specifically for RX logic
debugging?

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 4/4] ath10k: Better tx/rx debugging.

[Kalle Valo]

Ben Greear <greearb@candelatech.com> writes:

> On 02/05/2014 01:10 AM, Marek Puzyniak wrote:
>
>>>> So probably it would be better to have choice to enable this log print
>>>> when someone need this information otherwise by default it should be
>>>> disabled.
>>>
>>> Maybe a debug message is better?
>> Yes, for example with flag ATH10K_DBG_HTT.
>
> That one is pretty spammy already..maybe I can create a new flag
> ATH10K_DBG_RX or something like that that is specifically for RX logic
> debugging?

Yeah, that would be good. And then we can later add _TX for transmit
side.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 4/4] ath10k: Better tx/rx debugging.

[Ben Greear]

On 02/05/2014 10:17 PM, Kalle Valo wrote:
> Ben Greear <greearb@candelatech.com> writes:
>
>> On 02/05/2014 01:10 AM, Marek Puzyniak wrote:
>>
>>>>> So probably it would be better to have choice to enable this log print
>>>>> when someone need this information otherwise by default it should be
>>>>> disabled.
>>>>
>>>> Maybe a debug message is better?
>>> Yes, for example with flag ATH10K_DBG_HTT.
>>
>> That one is pretty spammy already..maybe I can create a new flag
>> ATH10K_DBG_RX or something like that that is specifically for RX logic
>> debugging?
>
> Yeah, that would be good. And then we can later add _TX for transmit
> side.

Patches I sent did not have this new bit...but I can add that next time
I work on this code.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k