On 15 July 2014 18:32, Simon Wunderlich <sw@xxxxxxxxxxxxxxxxxx> wrote:
> From: Sven Eckelmann <sven@xxxxxxxxxxxxx>
>
> Adds the spectral scan feature for ath10k. The spectral scan is triggered by
> configuring a mode through a debugfs control file. Samples can be gathered via
> another relay debugfs file.
>
> Essentially, to try it out:
>
> ip link dev wlan0 set up
> echo background > /sys/kernel/debug/ieee80211/phy0/ath10k/spectral_scan_ctl
> iw dev wlan0 scan
> echo disable > /sys/kernel/debug/ieee80211/phy0/ath10k/spectral_scan_ctl
> cat /sys/kernel/debug/ieee80211/phy0/ath10k/spectral_scan0 > samples
>
> This feature is still experimental.
>
> Signed-off-by: Sven Eckelmann <sven@xxxxxxxxxxxxx>
> Signed-off-by: Simon Wunderlich <sw@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Mathias Kretschmer <mathias.kretschmer@xxxxxxxxxxxxxxxxxxx>
> ---
> drivers/net/wireless/ath/ath10k/Kconfig | 1 +
> drivers/net/wireless/ath/ath10k/Makefile | 3 +-
> drivers/net/wireless/ath/ath10k/core.h | 6 +
> drivers/net/wireless/ath/ath10k/debug.c | 3 +
> drivers/net/wireless/ath/ath10k/spectral.c | 368 +++++++++++++++++++++++++++++
> drivers/net/wireless/ath/ath10k/spectral.h | 66 ++++++
> drivers/net/wireless/ath/ath10k/wmi.c | 98 +++++++-
> drivers/net/wireless/ath/ath10k/wmi.h | 80 +++++++
> drivers/net/wireless/ath/spectral_common.h | 20 ++
> 9 files changed, 643 insertions(+), 2 deletions(-)
> create mode 100644 drivers/net/wireless/ath/ath10k/spectral.c
> create mode 100644 drivers/net/wireless/ath/ath10k/spectral.h
>
[...]
> +static void ath_debug_send_fft_sample(struct ath10k *ar,
> + struct fft_sample_tlv *fft_sample_tlv)
> +{
Why ath_debug_... instead of ath10k_debug_.. ?
[...]
> +static int ath_get_spectral_vdevid(struct ath10k *ar)
ath_get_..? Shouldn't this be ath10k_get?
> +{
> + struct ath10k_vif *arvif;
> +
> + if (list_empty(&ar->arvifs))
> + return -ENODEV;
> +
> + arvif = list_first_entry(&ar->arvifs, typeof(*arvif), list);
> + return arvif->vdev_id;
> +}
No locks? Access to arvifs must be protected with conf_mutex.
What happens when an interface is removed without spectral scan being stopped?
> +int ath10k_spectral_scan_trigger(struct ath10k *ar)
> +{
> + int ret;
> + int vdev_id;
> +
> + vdev_id = ath_get_spectral_vdevid(ar);
> + if (vdev_id < 0)
> + return vdev_id;
> +
> + if (ar->spectral_mode == SPECTRAL_DISABLED)
> + return 0;
> +
> + ret = ath10k_vdev_spectral_enable(ar, vdev_id,
> + WMI_SPECTRAL_TRIGGER_CMD_CLEAR,
> + WMI_SPECTRAL_ENABLE_CMD_ENABLE);
> + if (ret < 0)
> + return ret;
> +
> + ret = ath10k_vdev_spectral_enable(ar, vdev_id,
> + WMI_SPECTRAL_TRIGGER_CMD_TRIGGER,
> + WMI_SPECTRAL_ENABLE_CMD_ENABLE);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +int ath10k_spectral_scan_config(struct ath10k *ar, enum spectral_mode mode)
> +{
> + int count, ret;
We tend to use `res` in ath10k.
> + struct wmi_vdev_spectral_configure_arg arg;
> + int vdev_id;
> +
> + vdev_id = ath_get_spectral_vdevid(ar);
> + if (vdev_id < 0)
> + return vdev_id;
> +
> + ar->spectral_mode = mode;
> +
> + ath10k_vdev_spectral_enable(ar, vdev_id,
> + WMI_SPECTRAL_TRIGGER_CMD_CLEAR,
> + WMI_SPECTRAL_ENABLE_CMD_DISABLE);
> + if (mode == SPECTRAL_DISABLED)
> + return 0;
> +
> + if (mode == SPECTRAL_BACKGROUND)
> + count = WMI_SPECTRAL_COUNT_DEFAULT;
> + else
> + count = max_t(u8, 1, ar->spec_config.count);
> +
> + arg.vdev_id = vdev_id;
> + arg.scan_count = count;
> + arg.scan_period = WMI_SPECTRAL_PERIOD_DEFAULT;
> + arg.scan_priority = WMI_SPECTRAL_PRIORITY_DEFAULT;
> + arg.scan_fft_size = WMI_SPECTRAL_FFT_SIZE_DEFAULT;
> + arg.scan_gc_ena = WMI_SPECTRAL_GC_ENA_DEFAULT;
> + arg.scan_restart_ena = WMI_SPECTRAL_RESTART_ENA_DEFAULT;
> + arg.scan_noise_floor_ref = WMI_SPECTRAL_NOISE_FLOOR_REF_DEFAULT;
> + arg.scan_init_delay = WMI_SPECTRAL_INIT_DELAY_DEFAULT;
> + arg.scan_nb_tone_thr = WMI_SPECTRAL_NB_TONE_THR_DEFAULT;
> + arg.scan_str_bin_thr = WMI_SPECTRAL_STR_BIN_THR_DEFAULT;
> + arg.scan_wb_rpt_mode = WMI_SPECTRAL_WB_RPT_MODE_DEFAULT;
> + arg.scan_rssi_rpt_mode = WMI_SPECTRAL_RSSI_RPT_MODE_DEFAULT;
> + arg.scan_rssi_thr = WMI_SPECTRAL_RSSI_THR_DEFAULT;
> + arg.scan_pwr_format = WMI_SPECTRAL_PWR_FORMAT_DEFAULT;
> + arg.scan_rpt_mode = WMI_SPECTRAL_RPT_MODE_DEFAULT;
> + arg.scan_bin_scale = WMI_SPECTRAL_BIN_SCALE_DEFAULT;
> + arg.scan_dBm_adj = WMI_SPECTRAL_DBM_ADJ_DEFAULT;
> + arg.scan_chn_mask = WMI_SPECTRAL_CHN_MASK_DEFAULT;
> +
> + ret = ath10k_vdev_spectral_configure(ar, &arg);
> + if (ret < 0)
+ ath10k_warn("failed to configure spectral scan: %d\n", ret);
> + return ret;
> +
> + if (mode == SPECTRAL_BACKGROUND)
> + ath10k_spectral_scan_trigger(ar);
Why isn't ath10k_spectral_scan_trigger() return value not checked?
Also +ath10k_warn.
[...]
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 6f83cae..7ad9c71 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -1659,7 +1659,47 @@ static void ath10k_wmi_event_spectral_scan(struct ath10k *ar,
> struct wmi_single_phyerr_rx_event *event,
> u64 tsf)
> {
> - ath10k_dbg(ATH10K_DBG_WMI, "wmi event spectral scan\n");
> + int buf_len, tlv_len, res, i = 0;
> + struct phyerr_tlv *tlv;
> + u8 *tlv_buf;
> + struct phyerr_fft_report *fftr;
> + size_t fftr_len;
> +
> + buf_len = __le32_to_cpu(event->hdr.buf_len);
> +
> + while (i < buf_len) {
> + if (i + sizeof(*tlv) > buf_len) {
> + ath10k_warn("too short buf for tlv header (%d)\n", i);
"failed to parse phyerr tlv header at byte %d\n", i
> + return;
> + }
> +
> + tlv = (struct phyerr_tlv *)&event->bufp[i];
> + tlv_len = __le16_to_cpu(tlv->len);
> + tlv_buf = &event->bufp[i + sizeof(*tlv)];
> +
> + if (i + sizeof(*tlv) + tlv_len > buf_len) {
> + ath10k_warn("too short buf for tlv (%d)\n", i);
"failed to parse phyerr tlv payload at byte %d\n", i
> + return;
> + }
> +
> + switch (tlv->tag) {
> + case PHYERR_TLV_TAG_SEARCH_FFT_REPORT:
> + if (sizeof(*fftr) > tlv_len) {
> + ath10k_warn("too short fft report (%d)\n", i);
"failed to parse fft report at byte %d\n", i
> + return;
> + }
> +
> + fftr_len = tlv_len - sizeof(*fftr);
> + fftr = (struct phyerr_fft_report *)tlv_buf;
> + res = ath10k_process_fft(ar, event, fftr, fftr_len,
> + tsf);
> + if (res)
+ ath10k_warn("failed to process fft report: %d\n", res)
[...]
> +int ath10k_vdev_spectral_configure(struct ath10k *ar,
> + const struct wmi_vdev_spectral_configure_arg *arg)
ath10k_wmi_vdev_spectral_configure
[...]
> +int ath10k_vdev_spectral_enable(struct ath10k *ar, u32 vdev_id, u32 trigger,
> + u32 enable)
ath10k_wmi_vdev_spectral_enable
[...]
> +struct wmi_vdev_spectral_configure_arg {
> + u32 vdev_id;
> + u32 scan_count;
> + u32 scan_period;
> + u32 scan_priority;
> + u32 scan_fft_size;
> + u32 scan_gc_ena;
> + u32 scan_restart_ena;
> + u32 scan_noise_floor_ref;
> + u32 scan_init_delay;
> + u32 scan_nb_tone_thr;
> + u32 scan_str_bin_thr;
> + u32 scan_wb_rpt_mode;
> + u32 scan_rssi_rpt_mode;
> + u32 scan_rssi_thr;
> + u32 scan_pwr_format;
> + u32 scan_rpt_mode;
> + u32 scan_bin_scale;
> + u32 scan_dBm_adj;
> + u32 scan_chn_mask;
Some stuff isn't self-explanatory. I'd love to see some of these
fields documented to account for possible values (wb_rpt_mode?) and
units (init_delay?).
[...]
> +struct fft_sample_ath10k_ht20 {
> + struct fft_sample_tlv tlv;
> + u8 mhz;
A more suitable name would be "chan_width_mhz" I guess?
> + __be16 freq1;
> + __be16 freq2;
> + __be16 noise;
> + __be16 max_magnitude;
> + __be16 total_gain_db;
> + __be16 base_pwr_db;
> + __be64 tsf;
> + s8 max_index;
> + u8 rssi;
> + u8 relpwr_db;
> + u8 avgpwr_db;
> +
> + u8 data[SPECTRAL_ATH10K_HT20_NUM_BINS];
And as pointed out the size/count of bins is most likely not
bandwidth-dependant. It's just a matter of resolution configured with
fft_size + bin_scale. Maybe this should be a variable-sized array?
Generally this seems to be missing locking (does debugfs guarantee
serialized read/write calls?).
Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
