Re: [PATCH 2/2] ath9k: add HT40 spectral scan capability

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

[-- Attachment #1: Type: text/plain, Size: 4226 bytes --]

Hello Lorenzo,

I've reviewed and tested your patch, this looks good! The old format still works, and
for a HT40+ channel I get your new format. Please find a few minor comments inline,
and feel free to add my Tested-by/Reviewed-by in the next round.

BTW, i failed to compile your UI[1]. I'm not familiar with QT, maybe you can add some
installation document (or Makefile or whatever)? :)

[1] https://github.com/LorenzoBianconi/ath_spectral

On Sat, Oct 05, 2013 at 11:16:09PM +0200, Lorenzo Bianconi wrote:
> @@ -905,6 +906,33 @@ struct fft_sample_ht20 {
>  	u8 data[SPECTRAL_HT20_NUM_BINS];
>  } __packed;
>  
> +struct fft_sample_ht20_40 {
> +	struct fft_sample_tlv tlv;
> +
> +	u8 max_exp;
> +
> +	__be16 freq;

What does the frequency tell us? This is just the frequency of the primary
channel, isn't it? You could either:
 1) make this the center frequency of both channels (e.g. for 2412 HT40+ this would be 2422)
 2) add a field to indicate HT40+ or HT40- or
 3) use lower_freq and upper_freq

Option 2 would have the advantage that the channel type is clear, not sure if this matters. :)

> +
> +	s8 lower_rssi;
> +	s8 upper_rssi;
> +
> +	__be64 tsf;
> +
> +	s8 lower_noise;
> +	s8 upper_noise;
> +
> +	__be16 lower_max_magnitude;
> +	__be16 upper_max_magnitude;
> +
> +	u8 lower_max_index;
> +	u8 upper_max_index;
> +
> +	u8 lower_bitmap_weight;
> +	u8 upper_bitmap_weight;
> +
> +	u8 data[SPECTRAL_HT20_40_NUM_BINS];
> +} __packed;
> +
>  void ath9k_tasklet(unsigned long data);
>  int ath_cabq_update(struct ath_softc *);
>  
> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
> index ab9e3a8..b49fd13 100644
> --- a/drivers/net/wireless/ath/ath9k/recv.c
> +++ b/drivers/net/wireless/ath/ath9k/recv.c
> @@ -972,14 +972,14 @@ static int ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
>  {
>  #ifdef CONFIG_ATH9K_DEBUGFS
>  	struct ath_hw *ah = sc->sc_ah;
> -	u8 bins[SPECTRAL_HT20_NUM_BINS];
> -	u8 *vdata = (u8 *)hdr;
> -	struct fft_sample_ht20 fft_sample;
> +	u8 num_bins, *bins, *sample, *vdata = (u8 *)hdr;
> +	struct fft_sample_ht20 fft_sample_20;
> +	struct fft_sample_ht20_40 fft_sample_40;
>  	struct ath_radar_info *radar_info;
> -	struct ath_ht20_mag_info *mag_info;
>  	int len = rs->rs_datalen;
>  	int dc_pos;
> -	u16 length, max_magnitude;
> +	u16 fft_len, length, freq = ah->curchan->chan->center_freq;
> +	enum nl80211_channel_type chan_type;
>  
>  	/* AR9280 and before report via ATH9K_PHYERR_RADAR, AR93xx and newer
>  	 * via ATH9K_PHYERR_SPECTRAL. Haven't seen ATH9K_PHYERR_FALSE_RADAR_EXT
> @@ -997,45 +997,44 @@ static int ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
>  	if (!(radar_info->pulse_bw_info & SPECTRAL_SCAN_BITMASK))
>  		return 0;
>  
> -	/* Variation in the data length is possible and will be fixed later.
> -	 * Note that we only support HT20 for now.
> -	 *
> -	 * TODO: add HT20_40 support as well.
> -	 */
> -	if ((len > SPECTRAL_HT20_TOTAL_DATA_LEN + 2) ||
> -	    (len < SPECTRAL_HT20_TOTAL_DATA_LEN - 1))
> -		return 1;
> -
> -	fft_sample.tlv.type = ATH_FFT_SAMPLE_HT20;
> -	length = sizeof(fft_sample) - sizeof(fft_sample.tlv);
> -	fft_sample.tlv.length = __cpu_to_be16(length);
> +	chan_type = cfg80211_get_chandef_type(&sc->hw->conf.chandef);
> +	if ((chan_type == NL80211_CHAN_HT40MINUS) ||
> +	    (chan_type == NL80211_CHAN_HT40PLUS)) {
> +		fft_len = SPECTRAL_HT20_40_TOTAL_DATA_LEN;
> +		num_bins = SPECTRAL_HT20_40_NUM_BINS;
> +		bins = (u8 *) fft_sample_40.data;

I get a few checkpatch errors here, like:

0002-ath9k-add-HT40-spectral-scan-capability.patch
CHECK: No space is necessary after a cast
#101: FILE: drivers/net/wireless/ath/ath9k/recv.c:1005:
+		num_bins = SPECTRAL_HT20_40_NUM_BINS;
+		bins = (u8 *) fft_sample_40.data;

CHECK: No space is necessary after a cast
#105: FILE: drivers/net/wireless/ath/ath9k/recv.c:1009:
+		num_bins = SPECTRAL_HT20_NUM_BINS;
+		bins = (u8 *) fft_sample_20.data;

There are a few more, please check using ./scripts/checkpatch.pl --strict.

(I think you can ignore the camelcase errors though).

Thanks,
	Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]