Comment 1 Mark Linimon 2012-06-24 06:33:43 UTC

Responsible Changed
From-To: freebsd-bugs->freebsd-wireless

Over to maintainer(s).

Comment 2 dfilter service 2012-06-24 06:59:48 UTC

Author: adrian
Date: Sun Jun 24 05:59:32 2012
New Revision: 237519
URL: http://svn.freebsd.org/changeset/base/237519

Log:
  Sometimes the AR5416 sends back radar PHY errors with both the PHY error
  and the CRC error bits set.  The radar payload is correct.
  
  When this happens, the stack doesn't see them PHY error frames and
  isn't interpreted as a PHY error.  So, no radar detection and no radiotap
  PHY error handling.
  
  Now, this may introduce some weird issues if the MAC sends up some other
  combination of CRC error + PHY error frames; this commit would break that
  and mark them as PHY errors instead of CRC errors.
  
  I may tinker with this a little more to pass radar/early radar/spectral
  frames up as PHY errors if the CRC bit is set, to restore the previous
  behaviour (where if CRC is set on a PHY error frame, it's marked as a CRC
  error rather than PHY error.)
  
  Tested on:	AR5416, over the air, to a USRP N200 which is generating a
  		large number of a variety of radar pulses.
  TODO:		Test on AR9130, AR9160, AR9280 (and maybe radar pulses on
  		2GHz on AR9285/AR9287.)
  
  PR:		kern/169362

Modified:
  head/sys/dev/ath/ath_hal/ar5416/ar5416_recv.c

Modified: head/sys/dev/ath/ath_hal/ar5416/ar5416_recv.c
==============================================================================
--- head/sys/dev/ath/ath_hal/ar5416/ar5416_recv.c	Sun Jun 24 04:29:03 2012	(r237518)
+++ head/sys/dev/ath/ath_hal/ar5416/ar5416_recv.c	Sun Jun 24 05:59:32 2012	(r237519)
@@ -228,15 +228,23 @@ ar5416ProcRxDesc(struct ath_hal *ah, str
 		 * Consequently we filter them out here so we don't
 		 * confuse and/or complicate drivers.
 		 */
-		if (ads->ds_rxstatus8 & AR_CRCErr)
-			rs->rs_status |= HAL_RXERR_CRC;
-		else if (ads->ds_rxstatus8 & AR_PHYErr) {
+
+		/*
+		 * The AR5416 sometimes sets both AR_CRCErr and AR_PHYErr
+		 * when reporting radar pulses.  In this instance,
+		 * clear HAL_RXERR_CRC and set HAL_RXERR_PHY.
+		 *
+		 * See PR kern/169362.
+		 */
+		if (ads->ds_rxstatus8 & AR_PHYErr) {
 			u_int phyerr;
 
 			rs->rs_status |= HAL_RXERR_PHY;
 			phyerr = MS(ads->ds_rxstatus8, AR_PHYErrCode);
 			rs->rs_phyerr = phyerr;
-		} else if (ads->ds_rxstatus8 & AR_DecryptCRCErr)
+		} else if (ads->ds_rxstatus8 & AR_CRCErr)
+			rs->rs_status |= HAL_RXERR_CRC;
+		else if (ads->ds_rxstatus8 & AR_DecryptCRCErr)
 			rs->rs_status |= HAL_RXERR_DECRYPT;
 		else if (ads->ds_rxstatus8 & AR_MichaelErr)
 			rs->rs_status |= HAL_RXERR_MIC;
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 3 dfilter service 2012-06-24 07:37:43 UTC

Author: adrian
Date: Sun Jun 24 06:37:28 2012
New Revision: 237521
URL: http://svn.freebsd.org/changeset/base/237521

Log:
  On second thought, let's just set both CRC and PHY errors together on
  frames that have it and let the upper layer sort it out.
  
  PR:		kern/169362

Modified:
  head/sys/dev/ath/ath_hal/ar5416/ar5416_recv.c

Modified: head/sys/dev/ath/ath_hal/ar5416/ar5416_recv.c
==============================================================================
--- head/sys/dev/ath/ath_hal/ar5416/ar5416_recv.c	Sun Jun 24 06:00:29 2012	(r237520)
+++ head/sys/dev/ath/ath_hal/ar5416/ar5416_recv.c	Sun Jun 24 06:37:28 2012	(r237521)
@@ -231,8 +231,9 @@ ar5416ProcRxDesc(struct ath_hal *ah, str
 
 		/*
 		 * The AR5416 sometimes sets both AR_CRCErr and AR_PHYErr
-		 * when reporting radar pulses.  In this instance,
-		 * clear HAL_RXERR_CRC and set HAL_RXERR_PHY.
+		 * when reporting radar pulses.  In this instance
+		 * set HAL_RXERR_PHY as well as HAL_RXERR_CRC and
+		 * let the driver layer figure out what to do.
 		 *
 		 * See PR kern/169362.
 		 */
@@ -242,7 +243,9 @@ ar5416ProcRxDesc(struct ath_hal *ah, str
 			rs->rs_status |= HAL_RXERR_PHY;
 			phyerr = MS(ads->ds_rxstatus8, AR_PHYErrCode);
 			rs->rs_phyerr = phyerr;
-		} else if (ads->ds_rxstatus8 & AR_CRCErr)
+		}
+
+		if (ads->ds_rxstatus8 & AR_CRCErr)
 			rs->rs_status |= HAL_RXERR_CRC;
 		else if (ads->ds_rxstatus8 & AR_DecryptCRCErr)
 			rs->rs_status |= HAL_RXERR_DECRYPT;
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 4 Adrian Chadd 2012-07-20 21:17:42 UTC

State Changed
From-To: open->patched

Patched and verified, but I should do some further verification to ensure 
CRC error frames aren't being incorrectly handled sa PHY error frames 
(and thus the upper layers don't see them as CRC error frames.)

Comment 5 dfilter service 2012-09-01 06:43:45 UTC

Author: adrian
Date: Sat Sep  1 05:43:30 2012
New Revision: 239966
URL: http://svn.freebsd.org/changeset/base/239966

Log:
  Fix the PHY / CRC error bug in the AR5212 HAL, which apparently also pops
  up on (at least) the AR5413.
  
  The 30 second summary - if a CRC error frame comes in during PHY error
  processing, that CRC bit will be set for all subsequent frames until
  a non-CRC error frame is processed.
  
  So to allow for accurate PHY error processing (Radar, and ANI on the AR5212
  HAL chips) just tag the frame as being both CRC and PHY - let the driver
  decide what to do with it.
  
  PR:		kern/169362

Modified:
  head/sys/dev/ath/ath_hal/ar5212/ar5212_recv.c

Modified: head/sys/dev/ath/ath_hal/ar5212/ar5212_recv.c
==============================================================================
--- head/sys/dev/ath/ath_hal/ar5212/ar5212_recv.c	Sat Sep  1 05:35:48 2012	(r239965)
+++ head/sys/dev/ath/ath_hal/ar5212/ar5212_recv.c	Sat Sep  1 05:43:30 2012	(r239966)
@@ -276,6 +276,14 @@ ar5212ProcRxDesc(struct ath_hal *ah, str
 	rs->rs_antenna  = MS(ads->ds_rxstatus0, AR_RcvAntenna);
 	rs->rs_more = (ads->ds_rxstatus0 & AR_More) ? 1 : 0;
 
+	/*
+	 * The AR5413 (at least) sometimes sets both AR_CRCErr and
+	 * AR_PHYErr when reporting radar pulses.  In this instance
+	 * set HAL_RXERR_PHY as well as HAL_RXERR_CRC and
+	 * let the driver layer figure out what to do.
+	 *
+	 * See PR kern/169362.
+	 */
 	if ((ads->ds_rxstatus1 & AR_FrmRcvOK) == 0) {
 		/*
 		 * These four bits should not be set together.  The
@@ -286,9 +294,7 @@ ar5212ProcRxDesc(struct ath_hal *ah, str
 		 * Consequently we filter them out here so we don't
 		 * confuse and/or complicate drivers.
 		 */
-		if (ads->ds_rxstatus1 & AR_CRCErr)
-			rs->rs_status |= HAL_RXERR_CRC;
-		else if (ads->ds_rxstatus1 & AR_PHYErr) {
+		if (ads->ds_rxstatus1 & AR_PHYErr) {
 			u_int phyerr;
 
 			rs->rs_status |= HAL_RXERR_PHY;
@@ -297,7 +303,11 @@ ar5212ProcRxDesc(struct ath_hal *ah, str
 			if (!AH5212(ah)->ah_hasHwPhyCounters &&
 			    phyerr != HAL_PHYERR_RADAR)
 				ar5212AniPhyErrReport(ah, rs);
-		} else if (ads->ds_rxstatus1 & AR_DecryptCRCErr)
+		}
+
+		if (ads->ds_rxstatus1 & AR_CRCErr)
+			rs->rs_status |= HAL_RXERR_CRC;
+		else if (ads->ds_rxstatus1 & AR_DecryptCRCErr)
 			rs->rs_status |= HAL_RXERR_DECRYPT;
 		else if (ads->ds_rxstatus1 & AR_MichaelErr)
 			rs->rs_status |= HAL_RXERR_MIC;
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 6 Ed Maste 2018-10-04 00:46:19 UTC

Adrian is this complete?

Comment 7 Oleksandr Tymoshenko 2019-01-20 01:23:26 UTC

There was a commit referencing this bug, but it's still not closed and has been inactive for some time. Closing as fixed. Please re-open it if the issue hasn't been completely resolved.