Patchwork Natty: ath9k_hw: partially revert fix dma descriptor rx error bit parsing

login
register
mail settings
Submitter Tim Gardner
Date April 25, 2011, 5:51 p.m.
Message ID <20110425175156.2ADE2F88CF@sepang.rtg.net>
Download mbox | patch
Permalink /patch/92764/
State New
Headers show

Comments

Tim Gardner - April 25, 2011, 5:51 p.m.
From 2b3494e8471c105708f0b69ae7ec2a97bafa6697 Mon Sep 17 00:00:00 2001
From: Felix Fietkau <nbd@openwrt.org>
Date: Fri, 14 Jan 2011 00:06:27 +0100
Subject: [PATCH] (pre stable) ath9k_hw: partially revert "fix dma descriptor rx error bit parsing"

BugLink: http://bugs.launchpad.net/bugs/735171

The rx error bit parsing was changed to consider PHY errors and various
decryption errors separately. While correct according to the documentation,
this is causing spurious decryption error reports in some situations.

Fix this by restoring the original order of the checks in those places,
where the errors are meant to be mutually exclusive.

If a CRC error is reported, then MIC failure and decryption errors
are irrelevant, and a PHY error is unlikely.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
(cherry picked from commit 115dad7a7f42e68840392767323ceb9306dbdb36)

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 drivers/net/wireless/ath/ath9k/ar9003_mac.c |    8 ++++----
 drivers/net/wireless/ath/ath9k/mac.c        |   14 ++++++++++----
 2 files changed, 14 insertions(+), 8 deletions(-)
Seth Forshee - April 25, 2011, 8:56 p.m.
On Mon, Apr 25, 2011 at 11:51:56AM -0600, Tim Gardner wrote:
> From 2b3494e8471c105708f0b69ae7ec2a97bafa6697 Mon Sep 17 00:00:00 2001
> From: Felix Fietkau <nbd@openwrt.org>
> Date: Fri, 14 Jan 2011 00:06:27 +0100
> Subject: [PATCH] (pre stable) ath9k_hw: partially revert "fix dma descriptor rx error bit parsing"
> 
> BugLink: http://bugs.launchpad.net/bugs/735171
> 
> The rx error bit parsing was changed to consider PHY errors and various
> decryption errors separately. While correct according to the documentation,
> this is causing spurious decryption error reports in some situations.
> 
> Fix this by restoring the original order of the checks in those places,
> where the errors are meant to be mutually exclusive.
> 
> If a CRC error is reported, then MIC failure and decryption errors
> are irrelevant, and a PHY error is unlikely.
> 
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> (cherry picked from commit 115dad7a7f42e68840392767323ceb9306dbdb36)
> 
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

Acked-by: Seth Forshee <seth.forshee@canonical.com>

> ---
>  drivers/net/wireless/ath/ath9k/ar9003_mac.c |    8 ++++----
>  drivers/net/wireless/ath/ath9k/mac.c        |   14 ++++++++++----
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_mac.c b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
> index 4ceddbb..038a0cb 100644
> --- a/drivers/net/wireless/ath/ath9k/ar9003_mac.c
> +++ b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
> @@ -615,7 +615,7 @@ int ath9k_hw_process_rxdesc_edma(struct ath_hw *ah, struct ath_rx_status *rxs,
>  		 */
>  		if (rxsp->status11 & AR_CRCErr)
>  			rxs->rs_status |= ATH9K_RXERR_CRC;
> -		if (rxsp->status11 & AR_PHYErr) {
> +		else if (rxsp->status11 & AR_PHYErr) {
>  			phyerr = MS(rxsp->status11, AR_PHYErrCode);
>  			/*
>  			 * If we reach a point here where AR_PostDelimCRCErr is
> @@ -638,11 +638,11 @@ int ath9k_hw_process_rxdesc_edma(struct ath_hw *ah, struct ath_rx_status *rxs,
>  				rxs->rs_phyerr = phyerr;
>  			}
>  
> -		}
> -		if (rxsp->status11 & AR_DecryptCRCErr)
> +		} else if (rxsp->status11 & AR_DecryptCRCErr)
>  			rxs->rs_status |= ATH9K_RXERR_DECRYPT;
> -		if (rxsp->status11 & AR_MichaelErr)
> +		else if (rxsp->status11 & AR_MichaelErr)
>  			rxs->rs_status |= ATH9K_RXERR_MIC;
> +
>  		if (rxsp->status11 & AR_KeyMiss)
>  			rxs->rs_status |= ATH9K_RXERR_DECRYPT;
>  	}
> diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
> index 2915b11..5efc869 100644
> --- a/drivers/net/wireless/ath/ath9k/mac.c
> +++ b/drivers/net/wireless/ath/ath9k/mac.c
> @@ -690,17 +690,23 @@ int ath9k_hw_rxprocdesc(struct ath_hw *ah, struct ath_desc *ds,
>  		rs->rs_flags |= ATH9K_RX_DECRYPT_BUSY;
>  
>  	if ((ads.ds_rxstatus8 & AR_RxFrameOK) == 0) {
> +		/*
> +		 * Treat these errors as mutually exclusive to avoid spurious
> +		 * extra error reports from the hardware. If a CRC error is
> +		 * reported, then decryption and MIC errors are irrelevant,
> +		 * the frame is going to be dropped either way
> +		 */
>  		if (ads.ds_rxstatus8 & AR_CRCErr)
>  			rs->rs_status |= ATH9K_RXERR_CRC;
> -		if (ads.ds_rxstatus8 & AR_PHYErr) {
> +		else if (ads.ds_rxstatus8 & AR_PHYErr) {
>  			rs->rs_status |= ATH9K_RXERR_PHY;
>  			phyerr = MS(ads.ds_rxstatus8, AR_PHYErrCode);
>  			rs->rs_phyerr = phyerr;
> -		}
> -		if (ads.ds_rxstatus8 & AR_DecryptCRCErr)
> +		} else if (ads.ds_rxstatus8 & AR_DecryptCRCErr)
>  			rs->rs_status |= ATH9K_RXERR_DECRYPT;
> -		if (ads.ds_rxstatus8 & AR_MichaelErr)
> +		else if (ads.ds_rxstatus8 & AR_MichaelErr)
>  			rs->rs_status |= ATH9K_RXERR_MIC;
> +
>  		if (ads.ds_rxstatus8 & AR_KeyMiss)
>  			rs->rs_status |= ATH9K_RXERR_DECRYPT;
>  	}
> -- 
> 1.7.0.4
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
John Johansen - April 25, 2011, 8:57 p.m.
On 04/25/2011 10:51 AM, Tim Gardner wrote:
> From 2b3494e8471c105708f0b69ae7ec2a97bafa6697 Mon Sep 17 00:00:00 2001
> From: Felix Fietkau <nbd@openwrt.org>
> Date: Fri, 14 Jan 2011 00:06:27 +0100
> Subject: [PATCH] (pre stable) ath9k_hw: partially revert "fix dma descriptor rx error bit parsing"
> 
> BugLink: http://bugs.launchpad.net/bugs/735171
> 
> The rx error bit parsing was changed to consider PHY errors and various
> decryption errors separately. While correct according to the documentation,
> this is causing spurious decryption error reports in some situations.
> 
> Fix this by restoring the original order of the checks in those places,
> where the errors are meant to be mutually exclusive.
> 
> If a CRC error is reported, then MIC failure and decryption errors
> are irrelevant, and a PHY error is unlikely.
> 
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> (cherry picked from commit 115dad7a7f42e68840392767323ceb9306dbdb36)
> 
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

well it doesn't seem to hurt my ath9k and it looks good so

Acked-by: John Johansen <john.johansen@canonical.com>

> ---
>  drivers/net/wireless/ath/ath9k/ar9003_mac.c |    8 ++++----
>  drivers/net/wireless/ath/ath9k/mac.c        |   14 ++++++++++----
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/ar9003_mac.c b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
> index 4ceddbb..038a0cb 100644
> --- a/drivers/net/wireless/ath/ath9k/ar9003_mac.c
> +++ b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
> @@ -615,7 +615,7 @@ int ath9k_hw_process_rxdesc_edma(struct ath_hw *ah, struct ath_rx_status *rxs,
>  		 */
>  		if (rxsp->status11 & AR_CRCErr)
>  			rxs->rs_status |= ATH9K_RXERR_CRC;
> -		if (rxsp->status11 & AR_PHYErr) {
> +		else if (rxsp->status11 & AR_PHYErr) {
>  			phyerr = MS(rxsp->status11, AR_PHYErrCode);
>  			/*
>  			 * If we reach a point here where AR_PostDelimCRCErr is
> @@ -638,11 +638,11 @@ int ath9k_hw_process_rxdesc_edma(struct ath_hw *ah, struct ath_rx_status *rxs,
>  				rxs->rs_phyerr = phyerr;
>  			}
>  
> -		}
> -		if (rxsp->status11 & AR_DecryptCRCErr)
> +		} else if (rxsp->status11 & AR_DecryptCRCErr)
>  			rxs->rs_status |= ATH9K_RXERR_DECRYPT;
> -		if (rxsp->status11 & AR_MichaelErr)
> +		else if (rxsp->status11 & AR_MichaelErr)
>  			rxs->rs_status |= ATH9K_RXERR_MIC;
> +
>  		if (rxsp->status11 & AR_KeyMiss)
>  			rxs->rs_status |= ATH9K_RXERR_DECRYPT;
>  	}
> diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
> index 2915b11..5efc869 100644
> --- a/drivers/net/wireless/ath/ath9k/mac.c
> +++ b/drivers/net/wireless/ath/ath9k/mac.c
> @@ -690,17 +690,23 @@ int ath9k_hw_rxprocdesc(struct ath_hw *ah, struct ath_desc *ds,
>  		rs->rs_flags |= ATH9K_RX_DECRYPT_BUSY;
>  
>  	if ((ads.ds_rxstatus8 & AR_RxFrameOK) == 0) {
> +		/*
> +		 * Treat these errors as mutually exclusive to avoid spurious
> +		 * extra error reports from the hardware. If a CRC error is
> +		 * reported, then decryption and MIC errors are irrelevant,
> +		 * the frame is going to be dropped either way
> +		 */
>  		if (ads.ds_rxstatus8 & AR_CRCErr)
>  			rs->rs_status |= ATH9K_RXERR_CRC;
> -		if (ads.ds_rxstatus8 & AR_PHYErr) {
> +		else if (ads.ds_rxstatus8 & AR_PHYErr) {
>  			rs->rs_status |= ATH9K_RXERR_PHY;
>  			phyerr = MS(ads.ds_rxstatus8, AR_PHYErrCode);
>  			rs->rs_phyerr = phyerr;
> -		}
> -		if (ads.ds_rxstatus8 & AR_DecryptCRCErr)
> +		} else if (ads.ds_rxstatus8 & AR_DecryptCRCErr)
>  			rs->rs_status |= ATH9K_RXERR_DECRYPT;
> -		if (ads.ds_rxstatus8 & AR_MichaelErr)
> +		else if (ads.ds_rxstatus8 & AR_MichaelErr)
>  			rs->rs_status |= ATH9K_RXERR_MIC;
> +
>  		if (ads.ds_rxstatus8 & AR_KeyMiss)
>  			rs->rs_status |= ATH9K_RXERR_DECRYPT;
>  	}
Tim Gardner - April 26, 2011, 12:40 a.m.
On 04/25/2011 11:51 AM, Tim Gardner wrote:
> BugLink:http://bugs.launchpad.net/bugs/735171

applied

Patch

diff --git a/drivers/net/wireless/ath/ath9k/ar9003_mac.c b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
index 4ceddbb..038a0cb 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_mac.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_mac.c
@@ -615,7 +615,7 @@  int ath9k_hw_process_rxdesc_edma(struct ath_hw *ah, struct ath_rx_status *rxs,
 		 */
 		if (rxsp->status11 & AR_CRCErr)
 			rxs->rs_status |= ATH9K_RXERR_CRC;
-		if (rxsp->status11 & AR_PHYErr) {
+		else if (rxsp->status11 & AR_PHYErr) {
 			phyerr = MS(rxsp->status11, AR_PHYErrCode);
 			/*
 			 * If we reach a point here where AR_PostDelimCRCErr is
@@ -638,11 +638,11 @@  int ath9k_hw_process_rxdesc_edma(struct ath_hw *ah, struct ath_rx_status *rxs,
 				rxs->rs_phyerr = phyerr;
 			}
 
-		}
-		if (rxsp->status11 & AR_DecryptCRCErr)
+		} else if (rxsp->status11 & AR_DecryptCRCErr)
 			rxs->rs_status |= ATH9K_RXERR_DECRYPT;
-		if (rxsp->status11 & AR_MichaelErr)
+		else if (rxsp->status11 & AR_MichaelErr)
 			rxs->rs_status |= ATH9K_RXERR_MIC;
+
 		if (rxsp->status11 & AR_KeyMiss)
 			rxs->rs_status |= ATH9K_RXERR_DECRYPT;
 	}
diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
index 2915b11..5efc869 100644
--- a/drivers/net/wireless/ath/ath9k/mac.c
+++ b/drivers/net/wireless/ath/ath9k/mac.c
@@ -690,17 +690,23 @@  int ath9k_hw_rxprocdesc(struct ath_hw *ah, struct ath_desc *ds,
 		rs->rs_flags |= ATH9K_RX_DECRYPT_BUSY;
 
 	if ((ads.ds_rxstatus8 & AR_RxFrameOK) == 0) {
+		/*
+		 * Treat these errors as mutually exclusive to avoid spurious
+		 * extra error reports from the hardware. If a CRC error is
+		 * reported, then decryption and MIC errors are irrelevant,
+		 * the frame is going to be dropped either way
+		 */
 		if (ads.ds_rxstatus8 & AR_CRCErr)
 			rs->rs_status |= ATH9K_RXERR_CRC;
-		if (ads.ds_rxstatus8 & AR_PHYErr) {
+		else if (ads.ds_rxstatus8 & AR_PHYErr) {
 			rs->rs_status |= ATH9K_RXERR_PHY;
 			phyerr = MS(ads.ds_rxstatus8, AR_PHYErrCode);
 			rs->rs_phyerr = phyerr;
-		}
-		if (ads.ds_rxstatus8 & AR_DecryptCRCErr)
+		} else if (ads.ds_rxstatus8 & AR_DecryptCRCErr)
 			rs->rs_status |= ATH9K_RXERR_DECRYPT;
-		if (ads.ds_rxstatus8 & AR_MichaelErr)
+		else if (ads.ds_rxstatus8 & AR_MichaelErr)
 			rs->rs_status |= ATH9K_RXERR_MIC;
+
 		if (ads.ds_rxstatus8 & AR_KeyMiss)
 			rs->rs_status |= ATH9K_RXERR_DECRYPT;
 	}