[PATCH 1/3] ath10k: add phyerr/dfs handling
Janusz Dziedzic
janusz.dziedzic at tieto.com
Wed Nov 6 08:52:09 EST 2013
On 6 November 2013 10:47, Kalle Valo <kvalo at qca.qualcomm.com> wrote:
> Marek Puzyniak <marek.puzyniak at tieto.com> writes:
>
>> From: Janusz Dziedzic <janusz.dziedzic at tieto.com>
>>
>> Handle phyerr, dfs event, radar_report and fft_report.
>> Add also debugfs dfs_simulate_radar and dfs_stats files.
>> Use ath dfs pattern detector.
>>
>> Signed-off-by: Janusz Dziedzic <janusz.dziedzic at tieto.com>
>
> Are there any dependencies to mac80211 or cfg80211 patches? There has
> been quite a lot of changes with DFS lately and it would be good to have
> all those patches in ath-next branch before I apply these.
>
>> --- a/drivers/net/wireless/ath/ath10k/debug.c
>> +++ b/drivers/net/wireless/ath/ath10k/debug.c
>> @@ -21,6 +21,14 @@
>> #include "core.h"
>> #include "debug.h"
>>
>> +#define ATH10K_DFS_STAT(s, p) (\
>> + len += scnprintf(buf + len, size - len, "%28s : %10u\n", s, \
>> + ar->debug.dfs_stats.p))
>> +
>> +#define ATH10K_DFS_POOL_STAT(s, p) (\
>> + len += scnprintf(buf + len, size - len, "%28s : %10u\n", s, \
>> + ar->debug.dfs_pool_stats.p))
>
> As these are only used by ath10k_read_file_dfs() better to move those
> just to top of that function.
>
>> +static ssize_t ath10k_read_file_dfs(struct file *file, char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>
> ath10k_read_dfs_stats()?
>
>> + int retval = 0, size = 8000, len = 0;
>
> Like Joe said, size can be const.
>
>> + struct ath10k *ar = file->private_data;
>> + char *buf;
>> +
>> + buf = kzalloc(size, GFP_KERNEL);
>> + if (buf == NULL)
>> + return -ENOMEM;
>> +
>> + if (!ar->dfs_detector) {
>> + len += scnprintf(buf + len, size - len, "DFS not enabled\n");
>> + goto exit;
>> + }
>> +
>> + ar->debug.dfs_pool_stats = ar->dfs_detector->get_stats(ar->dfs_detector);
>
> I think we need to take conf_mutex to make sure ar->dfs_detector is not
> destroyed while we use it.
>
We deregister dfs_detector in ath10k_mac_unregister() so we will first
destroy debugfs,
then we don't need any mutex here.
BTW I see we don't call debugfs_remove*() - so, currently mac clear
debugfs for us - ieee80211_unregister_hw() -> wiphy_unregister I
think.
After that we deregister dfs_detector.
>> diff --git a/drivers/net/wireless/ath/ath10k/debug.h b/drivers/net/wireless/ath/ath10k/debug.h
>> index 46e640a..cde53d6 100644
>> --- a/drivers/net/wireless/ath/ath10k/debug.h
>> +++ b/drivers/net/wireless/ath/ath10k/debug.h
>> @@ -33,6 +33,7 @@ enum ath10k_debug_mask {
>> ATH10K_DBG_MGMT = 0x00000100,
>> ATH10K_DBG_DATA = 0x00000200,
>> ATH10K_DBG_BMI = 0x00000400,
>> + ATH10K_DBG_REGULATORY = 0x00000800,
>> ATH10K_DBG_ANY = 0xffffffff,
>> };
>>
>> @@ -53,6 +54,7 @@ void ath10k_debug_read_service_map(struct ath10k *ar,
>> void ath10k_debug_read_target_stats(struct ath10k *ar,
>> struct wmi_stats_event *ev);
>>
>> +#define ATH10K_DFS_STAT_INC(ar, c) (ar->debug.dfs_stats.c++)
>> #else
>
> Empty line after #define
>
>> static inline int ath10k_debug_start(struct ath10k *ar)
>> {
>> @@ -82,6 +84,7 @@ static inline void ath10k_debug_read_target_stats(struct ath10k *ar,
>> struct wmi_stats_event *ev)
>> {
>> }
>> +#define ATH10K_DFS_STAT_INC(ar, c) do { } while (0)
>> #endif /* CONFIG_ATH10K_DEBUGFS */
>
> Empty line before and after #define.
>
>>
>> #ifdef CONFIG_ATH10K_DEBUG
>> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
>> index bbb0efa..79f8bfd 100644
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -1438,9 +1438,20 @@ static void ath10k_reg_notifier(struct wiphy *wiphy,
>> {
>> struct ieee80211_hw *hw = wiphy_to_ieee80211_hw(wiphy);
>> struct ath10k *ar = hw->priv;
>> + bool result;
>>
>> ath_reg_notifier_apply(wiphy, request, &ar->ath_common.regulatory);
>>
>> + if (config_enabled(CONFIG_ATH10K_DFS_CERTIFIED) && ar->dfs_detector) {
>> + ath10k_dbg(ATH10K_DBG_REGULATORY, "dfs region 0x%X\n",
>> + request->dfs_region);
>
> "0x%x" (also applies to elsewhere in the patch)
>
>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>> @@ -1383,9 +1383,251 @@ static void ath10k_wmi_event_tbttoffset_update(struct ath10k *ar,
>> ath10k_dbg(ATH10K_DBG_WMI, "WMI_TBTTOFFSET_UPDATE_EVENTID\n");
>> }
>>
>> +static void ath10k_dfs_radar_report(struct ath10k *ar,
>> + struct wmi_single_phyerr_rx_event *event,
>> + struct phyerr_radar_report *rr,
>> + u64 tsf)
>> +{
>> + u32 reg0, reg1, tsf32l;
>> + struct pulse_event pe;
>> + u64 tsf64;
>> + u8 rssi, width;
>
> What about locking? Does this function assume that conf_mutex is held?
> If yes, please document that with lockdep_assert_held(). If no, we have
> a problem :)
>
> (Reads wmi.c)
>
> Ah, wmi events don't sleep anymore, forgot that. So we can't really use
> conf_mutex here. I guess our options are bring back worker for wmi
> events or use spinlock.
>
I think we can use here spin_lock_bh(&ar->data_lock) when setting
ar->debug.dfs_stats and when reading this from debugfs.
But, is that really needed (in worst case we will get older values via debugfs)?
>> +
>> + reg0 = __le32_to_cpu(rr->reg0);
>> + reg1 = __le32_to_cpu(rr->reg1);
>> +
>> + ath10k_dbg(ATH10K_DBG_REGULATORY,
>> + "wmi phyerr radar report chirp %d max_width %d agc_total_gain %d pulse_delta_diff %d\n",
>> + MS(reg0, RADAR_REPORT_REG0_PULSE_IS_CHIRP),
>> + MS(reg0, RADAR_REPORT_REG0_PULSE_IS_MAX_WIDTH),
>> + MS(reg0, RADAR_REPORT_REG0_AGC_TOTAL_GAIN),
>> + MS(reg0, RADAR_REPORT_REG0_PULSE_DELTA_DIFF));
>> + ath10k_dbg(ATH10K_DBG_REGULATORY,
>> + "wmi phyerr radar report pulse_delta_pean %d pulse_sidx %d fft_valid %d agc_mb_gain %d subchan_mask %d\n",
>> + MS(reg0, RADAR_REPORT_REG0_PULSE_DELTA_PEAK),
>> + MS(reg0, RADAR_REPORT_REG0_PULSE_SIDX),
>> + MS(reg1, RADAR_REPORT_REG1_PULSE_SRCH_FFT_VALID),
>> + MS(reg1, RADAR_REPORT_REG1_PULSE_AGC_MB_GAIN),
>> + MS(reg1, RADAR_REPORT_REG1_PULSE_SUBCHAN_MASK));
>> + ath10k_dbg(ATH10K_DBG_REGULATORY,
>> + "wmi phyerr radar report pulse_tsf_offset 0x%X pulse_dur: %d\n",
>> + MS(reg1, RADAR_REPORT_REG1_PULSE_TSF_OFFSET),
>> + MS(reg1, RADAR_REPORT_REG1_PULSE_DUR));
>> +
>> + if (!ar->dfs_detector)
>> + return;
>> +
>> + /* report event to DFS pattern detector */
>> + tsf32l = __le32_to_cpu(event->hdr.tsf_timestamp);
>> + tsf64 = tsf & (~0xFFFFFFFFULL);
>> + tsf64 |= tsf32l;
>> +
>> + width = MS(reg1, RADAR_REPORT_REG1_PULSE_DUR);
>> + rssi = event->hdr.rssi_combined;
>> +
>> + /*
>> + * hardware store this as 8 bit signed value,
>> + * set to zero if negative number
>> + */
>
> to be consistent with rest of the comments in ath10k:
>
> "/* hardware...."
>
>> + if (rssi & 0x80)
>> + rssi = 0;
>> +
>> + pe.ts = tsf64;
>> + pe.freq = ar->hw->conf.chandef.chan->center_freq;
>> + pe.width = width;
>> + pe.rssi = rssi;
>> +
>> + ath10k_dbg(ATH10K_DBG_REGULATORY,
>> + "dfs add pulse freq: %d, width: %d, rssi %d, tsf: %llX\n",
>> + pe.freq, pe.width, pe.rssi, pe.ts);
>> +
>> + ATH10K_DFS_STAT_INC(ar, pulses_detected);
>> +
>> + if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe)) {
>> + ath10k_dbg(ATH10K_DBG_REGULATORY,
>> + "dfs no pulse pattern detected, yet\n");
>> + return;
>> + }
>> +
>> + ath10k_dbg(ATH10K_DBG_REGULATORY, "dfs radar detected\n");
>> + ATH10K_DFS_STAT_INC(ar, radar_detected);
>> + ieee80211_radar_detected(ar->hw);
>> +}
>> +
>> +static int ath10k_dfs_fft_report(struct ath10k *ar,
>> + struct wmi_single_phyerr_rx_event *event,
>> + struct phyerr_fft_report *fftr,
>> + u64 tsf)
>> +{
>> + u32 reg0, reg1;
>> + u8 rssi, peak_mag;
>> +
>> + reg0 = __le32_to_cpu(fftr->reg0);
>> + reg1 = __le32_to_cpu(fftr->reg1);
>> + rssi = event->hdr.rssi_combined;
>
> locking?
>
>> + ath10k_dbg(ATH10K_DBG_REGULATORY,
>> + "wmi phyerr fft report total_gain_db %d base_pwr_db %d fft_chn_idx %d peak_sidx %d\n",
>> + MS(reg0, SEARCH_FFT_REPORT_REG0_TOTAL_GAIN_DB),
>> + MS(reg0, SEARCH_FFT_REPORT_REG0_BASE_PWR_DB),
>> + MS(reg0, SEARCH_FFT_REPORT_REG0_FFT_CHN_IDX),
>> + MS(reg0, SEARCH_FFT_REPORT_REG0_PEAK_SIDX));
>> + ath10k_dbg(ATH10K_DBG_REGULATORY,
>> + "wmi phyerr fft report rel_pwr_db %d avgpwr_db %d peak_mag %d num_store_bin %d\n",
>> + MS(reg1, SEARCH_FFT_REPORT_REG1_RELPWR_DB),
>> + MS(reg1, SEARCH_FFT_REPORT_REG1_AVGPWR_DB),
>> + MS(reg1, SEARCH_FFT_REPORT_REG1_PEAK_MAG),
>> + MS(reg1, SEARCH_FFT_REPORT_REG1_NUM_STR_BINS_IB));
>> +
>> + peak_mag = MS(reg1, SEARCH_FFT_REPORT_REG1_PEAK_MAG);
>> +
>> + /* false event detection */
>> + if (rssi == DFS_RSSI_POSSIBLY_FALSE &&
>> + peak_mag < 2 * DFS_PEAK_MAG_THOLD_POSSIBLY_FALSE) {
>> + ath10k_dbg(ATH10K_DBG_REGULATORY, "dfs false pulse detected\n");
>> + ATH10K_DFS_STAT_INC(ar, pulses_discarded);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void ath10k_wmi_event_dfs(struct ath10k *ar,
>> + struct wmi_single_phyerr_rx_event *event,
>> + u64 tsf)
>> +{
>> + int buf_len, tlv_len, res, i = 0;
>> + struct phyerr_tlv *tlv;
>> + struct phyerr_radar_report *rr;
>> + struct phyerr_fft_report *fftr;
>> + u8 *tlv_buf;
>
> locking?
>
> --
> Kalle Valo
More information about the ath10k
mailing list