Re: [PATCH 1/3] ath10k: add phyerr/dfs handling

[Kalle Valo <kvalo@qca.qualcomm.com>]

Marek Puzyniak <marek.puzyniak@tieto.com> writes:

> From: Janusz Dziedzic <janusz.dziedzic@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@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.

> 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.

> +
> +	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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k