Summary: | [ath] AR5416: radar pulse PHY errors sometimes include the CRC Error bit set as well as the PHY errors | ||
---|---|---|---|
Product: | Base System | Reporter: | Adrian Chadd <adrian> |
Component: | wireless | Assignee: | freebsd-wireless (Nobody) <wireless> |
Status: | Closed FIXED | ||
Severity: | Affects Only Me | CC: | emaste, gonzo |
Priority: | Normal | Flags: | bugmeister:
mfc-stable10?
bugmeister: mfc-stable9? bugmeister: mfc-stable8? |
Version: | 9.0-STABLE | ||
Hardware: | Any | ||
OS: | Any |
Description
Adrian Chadd
![]() ![]() Responsible Changed From-To: freebsd-bugs->freebsd-wireless Over to maintainer(s). 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" 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" 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.) 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" Adrian is this complete? 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. |