| Message ID | 1427475590-2198-1-git-send-email-poh@qca.qualcomm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 03/27/2015 05:59 PM, Peter Oh wrote: > PRI value is used as divider when DFS detector analyzes candidate > radar pulses. > If PRI deviation is big from its origin PRI, DFS detector could miss > valid radar reports since HW often misses detecting radar pulses and > causes long interval value of pulses. > > For instance from practical results, if runtime PRI is calculated as > 1431 for fixed PRI value of 1428 and delta timestamp logs 15719, > the modular remainder will be 1409 and the delta between the remainder > and runtime PRI is 22 that is bigger than PRI tolerance which is 16. > As a result this radar report will be ignored even though it's valid. > > By using spec defined PRI for fixed PRI, we can correct this error. > Hm, not sure if this is a generic solution or 10k specific. As stated in the source code /* * tolerated deviation of radar time stamp in usecs on both sides * TODO: this might need to be HW-dependent */ #define PRI_TOLERANCE 16 the pri tolerance is assumed to be potentially dependent on used HW. The comment in fact was noted when the detector was written to become generally usable by any HW out there, before it became Atheros specific. From my measurements, the ath9k is far within the given pri tolerance, i.e. if you continuously feed ETSI pattern 0 over a signal generator, it reports PRIs very accurately at 1428us (or multiples thereof). Even under heavy load the jitter of those reported values is within +/- 3us. With that, the ath9k (9580 more specifically) is already more accurate than the given tolerance - increasing it further would harm its operability. The pri tolerance does not only help fine-tuning detection performance, but also heavily defines whether DFS channels are usable or not (afair, it is the most significant parameter to reduce false positives). That's why I believe we need to be very cautious when changing it to fix / improve minor issues. Therefore, if your measurements with 10k reveal that it needs a higher tolerance, it should be made HW dependent and defined individually for 9k and 10k. > Signed-off-by: Peter Oh <poh@qca.qualcomm.com> > --- > drivers/net/wireless/ath/dfs_pri_detector.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/dfs_pri_detector.c b/drivers/net/wireless/ath/dfs_pri_detector.c > index 1b5ad19..fef459a 100644 > --- a/drivers/net/wireless/ath/dfs_pri_detector.c > +++ b/drivers/net/wireless/ath/dfs_pri_detector.c > @@ -25,6 +25,8 @@ struct ath_dfs_pool_stats global_dfs_pool_stats = {}; > > #define DFS_POOL_STAT_INC(c) (global_dfs_pool_stats.c++) > #define DFS_POOL_STAT_DEC(c) (global_dfs_pool_stats.c--) > +#define GET_PRI_TO_USE(MIN, MAX, RUNTIME) \ > + (MIN + 16 == MAX - 16 ? MIN + 16 : RUNTIME) > /* in case of single PRI patterns, take nominal PRI */ #define GET_PRI_TO_USE(MIN, MAX, RUNTIME) \ ((MAX - MIN) == 2 * PRI_TOLERANCE) ? MIN + PRI_TOLERANCE : RUNTIME) If timestamp jitter is an issue for 10k, you might also want to consider adjusting the assumed PRI with every pulse you add to a series. Initially I had it implemented that way that the PRI was re-calculated continuously to minimize the variance between nominal and measured pattern. But for the 9k and its accurate timestamping that turned out to be a lot of calculation for almost no gain and was dropped. Cheers, Zefir
On 03/30/2015 02:55 AM, Zefir Kurtisi wrote: > On 03/27/2015 05:59 PM, Peter Oh wrote: >> PRI value is used as divider when DFS detector analyzes candidate >> radar pulses. >> If PRI deviation is big from its origin PRI, DFS detector could miss >> valid radar reports since HW often misses detecting radar pulses and >> causes long interval value of pulses. >> >> For instance from practical results, if runtime PRI is calculated as >> 1431 for fixed PRI value of 1428 and delta timestamp logs 15719, >> the modular remainder will be 1409 and the delta between the remainder >> and runtime PRI is 22 that is bigger than PRI tolerance which is 16. >> As a result this radar report will be ignored even though it's valid. >> >> By using spec defined PRI for fixed PRI, we can correct this error. >> > Hm, not sure if this is a generic solution or 10k specific. This concept is generic, but applies to 9k and 10k only since these 2 drivers are only using the DFS detector. > As stated in the source code > /* > * tolerated deviation of radar time stamp in usecs on both sides > * TODO: this might need to be HW-dependent > */ > #define PRI_TOLERANCE 16 > > the pri tolerance is assumed to be potentially dependent on used HW. The > comment > in fact was noted when the detector was written to become generally usable > by any > HW out there, before it became Atheros specific. > > From my measurements, the ath9k is far within the given pri tolerance, > i.e. if you > continuously feed ETSI pattern 0 over a signal generator, it reports PRIs > very > accurately at 1428us (or multiples thereof). Even under heavy load the > jitter of > those reported values is within +/- 3us. With that, the ath9k (9580 more > specifically) is already more accurate than the given tolerance - > increasing it > further would harm its operability. as you addressed, current PRI_TOLERANCE value 16 is big enough to cover both of 9k and 10k. the biggest offset of PRI on 10k so far I observed is 9us. Here I'm not increasing PRI tolerance, but using more accurate PRI. > The pri tolerance does not only help fine-tuning detection performance, > but also > heavily defines whether DFS channels are usable or not (afair, it is the > most > significant parameter to reduce false positives). the calculated PRI(ps.pri) at pseq_handler_create_sequences() is used to filter candidate radar pulses in pde_get_multiple() and to get total pulse duration. This change gives more reliable results in filtering candidate radar pulses and does not give defect on calculating total pulse duration since we already add 2* max_pri_tolerance on it. Let me know if I miss something here. > That's why I believe we > need to > be very cautious when changing it to fix / improve minor issues. This patch is not for minor fix. Current DFS detector fails on Japan W53 band which requires at least 50% of data traffic during DFS certificate. So this patch should apply to both of 9k and 10k. > Therefore, if your measurements with 10k reveal that it needs a higher > tolerance, > it should be made HW dependent and defined individually for 9k and 10k. The patch is not to use higher PRI tolerance, but to use more accurate PRI. >> Signed-off-by: Peter Oh <poh@qca.qualcomm.com> >> --- >> drivers/net/wireless/ath/dfs_pri_detector.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/ath/dfs_pri_detector.c > b/drivers/net/wireless/ath/dfs_pri_detector.c >> index 1b5ad19..fef459a 100644 >> --- a/drivers/net/wireless/ath/dfs_pri_detector.c >> +++ b/drivers/net/wireless/ath/dfs_pri_detector.c >> @@ -25,6 +25,8 @@ struct ath_dfs_pool_stats global_dfs_pool_stats = {}; >> >> #define DFS_POOL_STAT_INC(c) (global_dfs_pool_stats.c++) >> #define DFS_POOL_STAT_DEC(c) (global_dfs_pool_stats.c--) >> +#define GET_PRI_TO_USE(MIN, MAX, RUNTIME) \ >> + (MIN + 16 == MAX - 16 ? MIN + 16 : RUNTIME) >> > /* in case of single PRI patterns, take nominal PRI */ > #define GET_PRI_TO_USE(MIN, MAX, RUNTIME) \ > ((MAX - MIN) == 2 * PRI_TOLERANCE) ? MIN + PRI_TOLERANCE : > RUNTIME) > > > If timestamp jitter is an issue for 10k, you might also want to consider > adjusting > the assumed PRI with every pulse you add to a series. Initially I had it > implemented that way that the PRI was re-calculated continuously to > minimize the > variance between nominal and measured pattern. But for the 9k and its > accurate > timestamping that turned out to be a lot of calculation for almost no gain > and was > dropped. I also have looked up tsf offset for the issue, but since tsf offset is applies both of current and previous with similar value, it didn't help to solve this issue. > > Cheers, > Zefir > > > _______________________________________________ > ath10k mailing list > ath10k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath10k Regards, Peter
On 03/30/2015 07:57 PM, Peter Oh wrote: > > On 03/30/2015 02:55 AM, Zefir Kurtisi wrote: >> [...] >> That's why I believe we need to >> be very cautious when changing it to fix / improve minor issues. > This patch is not for minor fix. > Current DFS detector fails on Japan W53 band which requires at least 50% of data > traffic during DFS certificate. > So this patch should apply to both of 9k and 10k. That is the core of my concern: you add changes to fix FCC/JP, which at the same time also affects ETSI. Our company (and maybe others) got ath9k certified for ETSI, and any potential change to the detector relevant for that domain would essentially require to re-certify. There were several patches lately added to the detector that were isolated to specific domains (like the recent updates for FCC pattern 1) which I knew won't affect the ETSI detector performance, since they touched only the FCC configuration but not the algorithm itself. This patch does, and that's why I need to point out that doing so might void certification efforts out there. Unfortunately, I have no good idea how to cope with it. Freezing the driver at the certified state is no option, since we all want to evolve. Having multiple copies of the detector for each regulatory domain would be an option (and essentially will happen since FCC/JP can't be covered by PRI detectors only), but gives unacceptable code duplication. Ideally we would fully separate algorithm from configuration and leave the algorithm untouched ever after, not sure how doable, though. As for your patch at hand, I tested it for ETSI and it does not change detector performance, therefore (please replace 16 with PRI_TOLERANCE in the macro) Acked-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
Hi, On 04/01/2015 03:04 AM, Zefir Kurtisi wrote: > On 03/30/2015 07:57 PM, Peter Oh wrote: >> On 03/30/2015 02:55 AM, Zefir Kurtisi wrote: >>> [...] >>> That's why I believe we need to >>> be very cautious when changing it to fix / improve minor issues. >> This patch is not for minor fix. >> Current DFS detector fails on Japan W53 band which requires at least 50% > of data >> traffic during DFS certificate. >> So this patch should apply to both of 9k and 10k. > That is the core of my concern: you add changes to fix FCC/JP, which at > the same > time also affects ETSI. > > Our company (and maybe others) got ath9k certified for ETSI, and any > potential > change to the detector relevant for that domain would essentially require > to > re-certify. > > There were several patches lately added to the detector that were isolated > to > specific domains (like the recent updates for FCC pattern 1) which I knew > won't > affect the ETSI detector performance, since they touched only the FCC > configuration but not the algorithm itself. This patch does, and that's > why I need > to point out that doing so might void certification efforts out there. I'll try to find a way to not affect ETSI detector to keep the existing certificate. > Unfortunately, I have no good idea how to cope with it. Freezing the > driver at the > certified state is no option, since we all want to evolve. Having multiple > copies > of the detector for each regulatory domain would be an option (and > essentially > will happen since FCC/JP can't be covered by PRI detectors only), but > gives > unacceptable code duplication. Ideally we would fully separate algorithm > from > configuration and leave the algorithm untouched ever after, not sure how > doable, > though. > > > As for your patch at hand, I tested it for ETSI and it does not change > detector > performance, The patch is useful when there are many missing pulses within a burst. It happens almost every time when channel loading rate is higher than 40%, but around 30% channel loading does not miss pulses that much. > therefore (please replace 16 with PRI_TOLERANCE in the macro) I'll do. > Acked-by: Zefir Kurtisi <zefir.kurtisi@neratec.com> > > _______________________________________________ > ath10k mailing list > ath10k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath10k Thanks, Peter
Peter Oh <poh@codeaurora.org> writes: >> As for your patch at hand, I tested it for ETSI and it does not >> change detector performance, > > The patch is useful when there are many missing pulses within a burst. > It happens almost every time when channel loading rate is higher than > 40%, but around 30% channel loading does not miss pulses that much. > >> therefore (please replace 16 with PRI_TOLERANCE in the macro) > > I'll do. > >> Acked-by: Zefir Kurtisi <zefir.kurtisi@neratec.com> So what's the conclusion? Should I wait for v2 or is this good to commit? I didn't quite get Zefir's comment about PRI_TOLERANCE above.
On 04/15/2015 05:41 AM, Kalle Valo wrote: > Peter Oh <poh@codeaurora.org> writes: > >>> As for your patch at hand, I tested it for ETSI and it does not >>> change detector performance, >> The patch is useful when there are many missing pulses within a burst. >> It happens almost every time when channel loading rate is higher than >> 40%, but around 30% channel loading does not miss pulses that much. >> >>> therefore (please replace 16 with PRI_TOLERANCE in the macro) >> I'll do. >> >>> Acked-by: Zefir Kurtisi <zefir.kurtisi@neratec.com> > So what's the conclusion? Should I wait for v2 or is this good to > commit? I didn't quite get Zefir's comment about PRI_TOLERANCE above. > Please wait for 2nd patches. I'll prepare the patches that do not void this current products certificate. Regards, Peter
Hi Zefir, I believe the patch below is good to be checked in since you've already made change in DFS. Can you confirm it? Thanks, Peter On 04/01/2015 03:04 AM, Zefir Kurtisi wrote: > On 03/30/2015 07:57 PM, Peter Oh wrote: >> On 03/30/2015 02:55 AM, Zefir Kurtisi wrote: >>> [...] >>> That's why I believe we need to >>> be very cautious when changing it to fix / improve minor issues. >> This patch is not for minor fix. >> Current DFS detector fails on Japan W53 band which requires at least 50% > of data >> traffic during DFS certificate. >> So this patch should apply to both of 9k and 10k. > That is the core of my concern: you add changes to fix FCC/JP, which at > the same > time also affects ETSI. > > Our company (and maybe others) got ath9k certified for ETSI, and any > potential > change to the detector relevant for that domain would essentially require > to > re-certify. > > There were several patches lately added to the detector that were isolated > to > specific domains (like the recent updates for FCC pattern 1) which I knew > won't > affect the ETSI detector performance, since they touched only the FCC > configuration but not the algorithm itself. This patch does, and that's > why I need > to point out that doing so might void certification efforts out there. > > Unfortunately, I have no good idea how to cope with it. Freezing the > driver at the > certified state is no option, since we all want to evolve. Having multiple > copies > of the detector for each regulatory domain would be an option (and > essentially > will happen since FCC/JP can't be covered by PRI detectors only), but > gives > unacceptable code duplication. Ideally we would fully separate algorithm > from > configuration and leave the algorithm untouched ever after, not sure how > doable, > though. > > > As for your patch at hand, I tested it for ETSI and it does not change > detector > performance, therefore (please replace 16 with PRI_TOLERANCE in the macro) > > Acked-by: Zefir Kurtisi <zefir.kurtisi@neratec.com> > > _______________________________________________ > ath10k mailing list > ath10k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath10k
On 09/03/2015 08:04 PM, Peter Oh wrote: > Hi Zefir, > > I believe the patch below is good to be checked in since you've already made > change in DFS. > Can you confirm it? > > [...] > Hi Peter, yes, after adding the chirp detecting feature to ath9k, I did an internal certification run with all your patches applied. All good for ETSI (FCC testing ongoing), so full ACK for the changes. Cheers, Zefir
Zefir Kurtisi <zefir.kurtisi@neratec.com> writes: > On 09/03/2015 08:04 PM, Peter Oh wrote: >> Hi Zefir, >> >> I believe the patch below is good to be checked in since you've already made >> change in DFS. >> Can you confirm it? >> >> [...] >> > Hi Peter, > > yes, after adding the chirp detecting feature to ath9k, I did an internal > certification run with all your patches applied. All good for ETSI (FCC testing > ongoing), so full ACK for the changes. I don't have any patch like this in my queue anymore, so please resend if I need to take something.
diff --git a/drivers/net/wireless/ath/dfs_pri_detector.c b/drivers/net/wireless/ath/dfs_pri_detector.c index 1b5ad19..fef459a 100644 --- a/drivers/net/wireless/ath/dfs_pri_detector.c +++ b/drivers/net/wireless/ath/dfs_pri_detector.c @@ -25,6 +25,8 @@ struct ath_dfs_pool_stats global_dfs_pool_stats = {}; #define DFS_POOL_STAT_INC(c) (global_dfs_pool_stats.c++) #define DFS_POOL_STAT_DEC(c) (global_dfs_pool_stats.c--) +#define GET_PRI_TO_USE(MIN, MAX, RUNTIME) \ + (MIN + 16 == MAX - 16 ? MIN + 16 : RUNTIME) /** * struct pulse_elem - elements in pulse queue @@ -243,7 +245,8 @@ static bool pseq_handler_create_sequences(struct pri_detector *pde, ps.count_falses = 0; ps.first_ts = p->ts; ps.last_ts = ts; - ps.pri = ts - p->ts; + ps.pri = GET_PRI_TO_USE(pde->rs->pri_min, + pde->rs->pri_max, ts - p->ts); ps.dur = ps.pri * (pde->rs->ppb - 1) + 2 * pde->rs->max_pri_tolerance;
PRI value is used as divider when DFS detector analyzes candidate radar pulses. If PRI deviation is big from its origin PRI, DFS detector could miss valid radar reports since HW often misses detecting radar pulses and causes long interval value of pulses. For instance from practical results, if runtime PRI is calculated as 1431 for fixed PRI value of 1428 and delta timestamp logs 15719, the modular remainder will be 1409 and the delta between the remainder and runtime PRI is 22 that is bigger than PRI tolerance which is 16. As a result this radar report will be ignored even though it's valid. By using spec defined PRI for fixed PRI, we can correct this error. Signed-off-by: Peter Oh <poh@qca.qualcomm.com> --- drivers/net/wireless/ath/dfs_pri_detector.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)