[PATCH] ath9k: Save spectral scan data in network byteorder

[PATCH] ath9k: Save spectral scan data in network byteorder

[Sven Eckelmann]

The sample data received through the spectral scan can be either in big or
little endian byteorder. This information isn't stored in the output file.
Therefore it is not possible for the analyzer software to find the correct byte
order.

It is relative common to get the data from a low end AP in big endian mode and
transfer it to another computer in little endian mode to analyze it. Therefore,
it would be better to store it in network (big endian) byte order.

Signed-off-by: Sven Eckelmann <sven@open-mesh.com>
Cc: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
---
A patch for FFT_eval was also sent to the author.

 drivers/net/wireless/ath/ath9k/recv.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index d7c129b..2fac787 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1064,8 +1064,9 @@ static void ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
 
 	fft_sample.tlv.type = ATH_FFT_SAMPLE_HT20;
 	fft_sample.tlv.length = sizeof(fft_sample) - sizeof(fft_sample.tlv);
+	fft_sample.tlv.length = __cpu_to_be16(fft_sample.tlv.length);
 
-	fft_sample.freq = ah->curchan->chan->center_freq;
+	fft_sample.freq = __cpu_to_be16(ah->curchan->chan->center_freq);
 	fft_sample.rssi = fix_rssi_inv_only(rs->rs_rssi_ctl0);
 	fft_sample.noise = ah->noise;
 
@@ -1106,13 +1107,16 @@ static void ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
 	mag_info = ((struct ath_ht20_mag_info *)radar_info) - 1;
 
 	/* Apply exponent and grab further auxiliary information. */
-	for (i = 0; i < SPECTRAL_HT20_NUM_BINS; i++)
+	for (i = 0; i < SPECTRAL_HT20_NUM_BINS; i++) {
 		fft_sample.data[i] = bins[i] << mag_info->max_exp;
+		fft_sample.data[i] = __cpu_to_be16(fft_sample.data[i]);
+	}
 
 	fft_sample.max_magnitude = spectral_max_magnitude(mag_info->all_bins);
+	fft_sample.max_magnitude = __cpu_to_be16(fft_sample.max_magnitude);
 	fft_sample.max_index = spectral_max_index(mag_info->all_bins);
 	fft_sample.bitmap_weight = spectral_bitmap_weight(mag_info->all_bins);
-	fft_sample.tsf = tsf;
+	fft_sample.tsf = __cpu_to_be64(tsf);
 
 	ath_debug_send_fft_sample(sc, &fft_sample.tlv);
 #endif
-- 
1.7.10.4

Re: [PATCH] ath9k: Save spectral scan data in network byteorder

[Simon Wunderlich]

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

On Tue, Jan 15, 2013 at 01:02:43PM +0100, Sven Eckelmann wrote:
> The sample data received through the spectral scan can be either in big or
> little endian byteorder. This information isn't stored in the output file.
> Therefore it is not possible for the analyzer software to find the correct byte
> order.
> 
> It is relative common to get the data from a low end AP in big endian mode and
> transfer it to another computer in little endian mode to analyze it. Therefore,
> it would be better to store it in network (big endian) byte order.

I'd agree that changing the byteorder to the "general" network order format is
a good idea, although we will break compatibility again - which should hopefully
be no problem as the original patch is pretty new anyway.

I was pondering about performance loss when we run spectral on the same machine
(i.e. maybe adding two useless byteswaps per value). OTOH the byteswaps are pretty
cheap and we do some computations anyway, so the performance difference should be quite
small.

Therefore,

Acked-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>

(I've CCd some more people in the reply so they can scream if they don't like it. ;])

Thanks,
	Simon

> 
> Signed-off-by: Sven Eckelmann <sven@open-mesh.com>
> Cc: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
> ---
> A patch for FFT_eval was also sent to the author.
> 
>  drivers/net/wireless/ath/ath9k/recv.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
> index d7c129b..2fac787 100644
> --- a/drivers/net/wireless/ath/ath9k/recv.c
> +++ b/drivers/net/wireless/ath/ath9k/recv.c
> @@ -1064,8 +1064,9 @@ static void ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
>  
>  	fft_sample.tlv.type = ATH_FFT_SAMPLE_HT20;
>  	fft_sample.tlv.length = sizeof(fft_sample) - sizeof(fft_sample.tlv);
> +	fft_sample.tlv.length = __cpu_to_be16(fft_sample.tlv.length);
>  
> -	fft_sample.freq = ah->curchan->chan->center_freq;
> +	fft_sample.freq = __cpu_to_be16(ah->curchan->chan->center_freq);
>  	fft_sample.rssi = fix_rssi_inv_only(rs->rs_rssi_ctl0);
>  	fft_sample.noise = ah->noise;
>  
> @@ -1106,13 +1107,16 @@ static void ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
>  	mag_info = ((struct ath_ht20_mag_info *)radar_info) - 1;
>  
>  	/* Apply exponent and grab further auxiliary information. */
> -	for (i = 0; i < SPECTRAL_HT20_NUM_BINS; i++)
> +	for (i = 0; i < SPECTRAL_HT20_NUM_BINS; i++) {
>  		fft_sample.data[i] = bins[i] << mag_info->max_exp;
> +		fft_sample.data[i] = __cpu_to_be16(fft_sample.data[i]);
> +	}
>  
>  	fft_sample.max_magnitude = spectral_max_magnitude(mag_info->all_bins);
> +	fft_sample.max_magnitude = __cpu_to_be16(fft_sample.max_magnitude);
>  	fft_sample.max_index = spectral_max_index(mag_info->all_bins);
>  	fft_sample.bitmap_weight = spectral_bitmap_weight(mag_info->all_bins);
> -	fft_sample.tsf = tsf;
> +	fft_sample.tsf = __cpu_to_be64(tsf);
>  
>  	ath_debug_send_fft_sample(sc, &fft_sample.tlv);
>  #endif
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH] ath9k: Save spectral scan data in network byteorder

[Zefir Kurtisi]

On 01/16/2013 09:59 AM, Simon Wunderlich wrote:
> On Tue, Jan 15, 2013 at 01:02:43PM +0100, Sven Eckelmann wrote:
>> The sample data received through the spectral scan can be either in big or
>> little endian byteorder. This information isn't stored in the output file.
>> Therefore it is not possible for the analyzer software to find the correct byte
>> order.
>>
>> It is relative common to get the data from a low end AP in big endian mode and
>> transfer it to another computer in little endian mode to analyze it. Therefore,
>> it would be better to store it in network (big endian) byte order.
> 
> I'd agree that changing the byteorder to the "general" network order format is
> a good idea, although we will break compatibility again - which should hopefully
> be no problem as the original patch is pretty new anyway.
> 
> I was pondering about performance loss when we run spectral on the same machine
> (i.e. maybe adding two useless byteswaps per value). OTOH the byteswaps are pretty
> cheap and we do some computations anyway, so the performance difference should be quite
> small.
> 
> Therefore,
> 
> Acked-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
> 
> (I've CCd some more people in the reply so they can scream if they don't like it. ;])
> 
> Thanks,
> 	Simon
> 
Hi Simon,

at this point, I'd like to advocate the approach I am currently using that does
the conversion fully in user space. There, each sample provided consists of the
fft data as bytes plus the values required to scale the magnitude values back to
absolute power numbers (i.e. the 'bug-fixed' raw data).

Since for the scaling an user space component is mandatory (FP calculations), I
miss to see the benefit of splitting post-processing in the kernel (shifting bins)
and user space. This step doubles fft data size and introduces endianess issues
for no (at least to me) evident reason.

Maybe you have a host application in mind where it does not really matter where
the processing is done. Whereas, if you operate an AP as continuous spectral
scanner, shifting the post-processing to the point where it is evaluated /
displayed (usually your fat PC) significantly impacts your AP's load and its
capability to provide higher sampling rates.

I'd assume Adrian's proposal to just push out the raw spectral data to user-space
is also targeting at that, which I fully support. The only thing I would keep in
the driver is fixing the fft data corruption caused by known chip bugs.


Cheers,
Zefir
>>
>> Signed-off-by: Sven Eckelmann <sven@open-mesh.com>
>> Cc: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
>> ---
>> A patch for FFT_eval was also sent to the author.
>>
>>  drivers/net/wireless/ath/ath9k/recv.c |   10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
>> index d7c129b..2fac787 100644
>> --- a/drivers/net/wireless/ath/ath9k/recv.c
>> +++ b/drivers/net/wireless/ath/ath9k/recv.c
>> @@ -1064,8 +1064,9 @@ static void ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
>>  
>>  	fft_sample.tlv.type = ATH_FFT_SAMPLE_HT20;
>>  	fft_sample.tlv.length = sizeof(fft_sample) - sizeof(fft_sample.tlv);
>> +	fft_sample.tlv.length = __cpu_to_be16(fft_sample.tlv.length);
>>  
>> -	fft_sample.freq = ah->curchan->chan->center_freq;
>> +	fft_sample.freq = __cpu_to_be16(ah->curchan->chan->center_freq);
>>  	fft_sample.rssi = fix_rssi_inv_only(rs->rs_rssi_ctl0);
>>  	fft_sample.noise = ah->noise;
>>  
>> @@ -1106,13 +1107,16 @@ static void ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
>>  	mag_info = ((struct ath_ht20_mag_info *)radar_info) - 1;
>>  
>>  	/* Apply exponent and grab further auxiliary information. */
>> -	for (i = 0; i < SPECTRAL_HT20_NUM_BINS; i++)
>> +	for (i = 0; i < SPECTRAL_HT20_NUM_BINS; i++) {
>>  		fft_sample.data[i] = bins[i] << mag_info->max_exp;
>> +		fft_sample.data[i] = __cpu_to_be16(fft_sample.data[i]);
>> +	}
>>  
>>  	fft_sample.max_magnitude = spectral_max_magnitude(mag_info->all_bins);
>> +	fft_sample.max_magnitude = __cpu_to_be16(fft_sample.max_magnitude);
>>  	fft_sample.max_index = spectral_max_index(mag_info->all_bins);
>>  	fft_sample.bitmap_weight = spectral_bitmap_weight(mag_info->all_bins);
>> -	fft_sample.tsf = tsf;
>> +	fft_sample.tsf = __cpu_to_be64(tsf);
>>  
>>  	ath_debug_send_fft_sample(sc, &fft_sample.tlv);
>>  #endif
>> -- 
>> 1.7.10.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ath9k: Save spectral scan data in network byteorder

[Simon Wunderlich]

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

Hey Zefir,

On Wed, Jan 16, 2013 at 10:53:15AM +0100, Zefir Kurtisi wrote:
> On 01/16/2013 09:59 AM, Simon Wunderlich wrote:
> > On Tue, Jan 15, 2013 at 01:02:43PM +0100, Sven Eckelmann wrote:
> >> The sample data received through the spectral scan can be either in big or
> >> little endian byteorder. This information isn't stored in the output file.
> >> Therefore it is not possible for the analyzer software to find the correct byte
> >> order.
> >>
> >> It is relative common to get the data from a low end AP in big endian mode and
> >> transfer it to another computer in little endian mode to analyze it. Therefore,
> >> it would be better to store it in network (big endian) byte order.
> > 
> > I'd agree that changing the byteorder to the "general" network order format is
> > a good idea, although we will break compatibility again - which should hopefully
> > be no problem as the original patch is pretty new anyway.
> > 
> > I was pondering about performance loss when we run spectral on the same machine
> > (i.e. maybe adding two useless byteswaps per value). OTOH the byteswaps are pretty
> > cheap and we do some computations anyway, so the performance difference should be quite
> > small.
> > 
> > Therefore,
> > 
> > Acked-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
> > 
> > (I've CCd some more people in the reply so they can scream if they don't like it. ;])
> > 
> > Thanks,
> > 	Simon
> > 
> Hi Simon,
> 
> at this point, I'd like to advocate the approach I am currently using that does
> the conversion fully in user space. There, each sample provided consists of the
> fft data as bytes plus the values required to scale the magnitude values back to
> absolute power numbers (i.e. the 'bug-fixed' raw data).
> 
> Since for the scaling an user space component is mandatory (FP calculations), I
> miss to see the benefit of splitting post-processing in the kernel (shifting bins)
> and user space. This step doubles fft data size and introduces endianess issues
> for no (at least to me) evident reason.

Well, that's right. The idea was to not send so "obscure" data, but I guess providing
sample code which shifts the bins etc should be easy enough for people integrating this
in userspace.

> 
> Maybe you have a host application in mind where it does not really matter where
> the processing is done. Whereas, if you operate an AP as continuous spectral
> scanner, shifting the post-processing to the point where it is evaluated /
> displayed (usually your fat PC) significantly impacts your AP's load and its
> capability to provide higher sampling rates.

I didn't see any performance limits yet. Shifting the data is probably not so expensive
(what we do right now in the kernel), but doubled sample size and other aspects
will certainly have an impact for this kind of "streaming" application for (low-end) APs.

> 
> I'd assume Adrian's proposal to just push out the raw spectral data to user-space
> is also targeting at that, which I fully support. The only thing I would keep in
> the driver is fixing the fft data corruption caused by known chip bugs.

As far as I understood, Adrian returns the whole packet with all flags and bugs, but I might
be wrong. I'd agree to at least fix the length bug in the kernel (newer chips should not
have this flaw anymore).

In any case, I don't want to change the format multiple times again, so let's discuss and
fix it once and for all[tm] now. There are some parties already experimenting with and integrating
this feature now. And if I'm not mistaken we have ~3 weeks left to add changes for
the next kernel version.

My suggestions for the new format:
 * fix length bugs (as you said)
 * use a TLV format to hand out samples (as done now) - we can use a new type
 * each "sample packet" holds one sample, i.e. aggregated sample packets from the chip (we are not processing
   them in the current version, but in later versions maybe) are split into multiple "sample packets"
 * the bins are not shifted (8 bit per bin)
 * auxiliary information which is larger than 8 bit (tsf, frequency, etc) is converted into network byte order

Anything else?

Cheers,
	Simon

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

Re: [PATCH] ath9k: Save spectral scan data in network byteorder

[Zefir Kurtisi]

On 01/16/2013 02:18 PM, Simon Wunderlich wrote:
> [...]
>> Since for the scaling an user space component is mandatory (FP calculations), I
>> miss to see the benefit of splitting post-processing in the kernel (shifting bins)
>> and user space. This step doubles fft data size and introduces endianess issues
>> for no (at least to me) evident reason.
> 
> Well, that's right. The idea was to not send so "obscure" data, but I guess providing
> sample code which shifts the bins etc should be easy enough for people integrating this
> in userspace.
> 
Understood, but providing 'semi-obscure' data is not really worth the overhead ;)
>>
>> Maybe you have a host application in mind where it does not really matter where
>> the processing is done. Whereas, if you operate an AP as continuous spectral
>> scanner, shifting the post-processing to the point where it is evaluated /
>> displayed (usually your fat PC) significantly impacts your AP's load and its
>> capability to provide higher sampling rates.
> 
> I didn't see any performance limits yet. Shifting the data is probably not so expensive
> (what we do right now in the kernel), but doubled sample size and other aspects
> will certainly have an impact for this kind of "streaming" application for (low-end) APs.
> 
Right now, with a 500MHz ARM SoC I am limited to ~12k-samples/s with resources
fully utilized. That's far from the 250kS/s the chip could theoretically provide.
But I'm still with my netlink based interface and need to check if relay-fs is
more efficient.
>>
>> I'd assume Adrian's proposal to just push out the raw spectral data to user-space
>> is also targeting at that, which I fully support. The only thing I would keep in
>> the driver is fixing the fft data corruption caused by known chip bugs.
> 
> As far as I understood, Adrian returns the whole packet with all flags and bugs, but I might
> be wrong. I'd agree to at least fix the length bug in the kernel (newer chips should not
> have this flaw anymore).
> 
Right, that's why the 'bug-fixing' should remain part of the driver.

> In any case, I don't want to change the format multiple times again, so let's discuss and
> fix it once and for all[tm] now. There are some parties already experimenting with and integrating
> this feature now. And if I'm not mistaken we have ~3 weeks left to add changes for
> the next kernel version.
> 
Beside the required modifications being trivial, since the user space
post-processing is mandatory - could a library for data conversion help to
decouple driver from test development?

> My suggestions for the new format:
>  * fix length bugs (as you said)
>  * use a TLV format to hand out samples (as done now) - we can use a new type
>  * each "sample packet" holds one sample, i.e. aggregated sample packets from the chip (we are not processing
>    them in the current version, but in later versions maybe) are split into multiple "sample packets"
What are your plans for aggregation here, given that you need to convert the data
won't (easily) work in the kernel?
>  * the bins are not shifted (8 bit per bin)
>  * auxiliary information which is larger than 8 bit (tsf, frequency, etc) is converted into network byte order
> 
> Anything else?
> 
With the rssi and noise-floor values already included: no, looks complete.

> Cheers,
> 	Simon
> 

Re: [PATCH] ath9k: Save spectral scan data in network byteorder

[Simon Wunderlich]

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

Hey Zefir,

On Wed, Jan 16, 2013 at 05:00:09PM +0100, Zefir Kurtisi wrote:
> [...] 
> >>
> >> Maybe you have a host application in mind where it does not really matter where
> >> the processing is done. Whereas, if you operate an AP as continuous spectral
> >> scanner, shifting the post-processing to the point where it is evaluated /
> >> displayed (usually your fat PC) significantly impacts your AP's load and its
> >> capability to provide higher sampling rates.
> > 
> > I didn't see any performance limits yet. Shifting the data is probably not so expensive
> > (what we do right now in the kernel), but doubled sample size and other aspects
> > will certainly have an impact for this kind of "streaming" application for (low-end) APs.
> > 
> Right now, with a 500MHz ARM SoC I am limited to ~12k-samples/s with resources
> fully utilized. That's far from the 250kS/s the chip could theoretically provide.
> But I'm still with my netlink based interface and need to check if relay-fs is
> more efficient.

Well, give it a try. My application is not requiring maximum output, but I've changed to
relayfs because it was suggested for performance reasons. Let's hope it can keep
this promise. :)

> [...] 
> > In any case, I don't want to change the format multiple times again, so let's discuss and
> > fix it once and for all[tm] now. There are some parties already experimenting with and integrating
> > this feature now. And if I'm not mistaken we have ~3 weeks left to add changes for
> > the next kernel version.
> > 
> Beside the required modifications being trivial, since the user space
> post-processing is mandatory - could a library for data conversion help to
> decouple driver from test development?
> 

Might be a good thing, although I don't plan to write this now - The fft_eval
program is good enough for a reference right now. A library could handle both FreeBSD
and Linux in the future. Feel free to contribute. ;)

> > My suggestions for the new format:
> >  * fix length bugs (as you said)
> >  * use a TLV format to hand out samples (as done now) - we can use a new type
> >  * each "sample packet" holds one sample, i.e. aggregated sample packets from the chip (we are not processing
> >    them in the current version, but in later versions maybe) are split into multiple "sample packets"
> What are your plans for aggregation here, given that you need to convert the data
> won't (easily) work in the kernel?

I haven't seen the aggregated spectral reports myself yet, only seen comments from Adrian.
http://article.gmane.org/gmane.linux.kernel.wireless.general/102030

The plan would be to split them so userspace always gets one sample per TLV, and we don't have
to create yet another format. So we would have two types: HT20 report and HT40 report that
userspace can support.

But we are not handling them now anyway, just to have the format somewhat future proof. :)

> >  * the bins are not shifted (8 bit per bin)
> >  * auxiliary information which is larger than 8 bit (tsf, frequency, etc) is converted into network byte order
> > 
> > Anything else?
> > 
> With the rssi and noise-floor values already included: no, looks complete.

OK, if no one else objects or has further comments I can provide patches for this one.

Cheers,
	Simon

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

[RFC] ath9k: Don't preshift spectral scan bins

[Sven Eckelmann]

The extension of the 8 bit bins for each bin to 16 bit is not necessary. This
operation can be done in userspace or on a different machine. Instead the
max_exp defining the amount of shifting required for each bin is exported to
userspace.

The change of the output format requires a change of the type in the sample
tlv to allow the userspace program to correctly detect the bin format.

Reported-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
Signed-off-by: Sven Eckelmann <sven@open-mesh.com>
---
 drivers/net/wireless/ath/ath9k/ath9k.h |    6 +++---
 drivers/net/wireless/ath/ath9k/recv.c  |   10 ++++------
 2 files changed, 7 insertions(+), 9 deletions(-)

This patch requires the previous patch
"[PATCH] ath9k: Save spectral scan data in network byteorder"

Please discuss the final format before this patch is applied.

diff --git a/drivers/net/wireless/ath/ath9k/ath9k.h b/drivers/net/wireless/ath/ath9k/ath9k.h
index 8250330..cc6b786 100644
--- a/drivers/net/wireless/ath/ath9k/ath9k.h
+++ b/drivers/net/wireless/ath/ath9k/ath9k.h
@@ -864,7 +864,7 @@ static inline u8 spectral_bitmap_weight(u8 *bins)
  * interface.
  */
 enum ath_fft_sample_type {
-	ATH_FFT_SAMPLE_HT20 = 0,
+	ATH_FFT_SAMPLE_HT20 = 1,
 };
 
 struct fft_sample_tlv {
@@ -876,7 +876,7 @@ struct fft_sample_tlv {
 struct fft_sample_ht20 {
 	struct fft_sample_tlv tlv;
 
-	u8 __alignment;
+	u8 max_exp;
 
 	u16 freq;
 	s8 rssi;
@@ -888,7 +888,7 @@ struct fft_sample_ht20 {
 
 	u64 tsf;
 
-	u16 data[SPECTRAL_HT20_NUM_BINS];
+	u8 data[SPECTRAL_HT20_NUM_BINS];
 } __packed;
 
 void ath9k_tasklet(unsigned long data);
diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index 2fac787..717ca9d 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -1035,7 +1035,7 @@ static void ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
 	struct ath_radar_info *radar_info;
 	struct ath_ht20_mag_info *mag_info;
 	int len = rs->rs_datalen;
-	int i, dc_pos;
+	int dc_pos;
 
 	/* AR9280 and before report via ATH9K_PHYERR_RADAR, AR93xx and newer
 	 * via ATH9K_PHYERR_SPECTRAL. Haven't seen ATH9K_PHYERR_FALSE_RADAR_EXT
@@ -1106,11 +1106,9 @@ static void ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
 	/* mag data is at the end of the frame, in front of radar_info */
 	mag_info = ((struct ath_ht20_mag_info *)radar_info) - 1;
 
-	/* Apply exponent and grab further auxiliary information. */
-	for (i = 0; i < SPECTRAL_HT20_NUM_BINS; i++) {
-		fft_sample.data[i] = bins[i] << mag_info->max_exp;
-		fft_sample.data[i] = __cpu_to_be16(fft_sample.data[i]);
-	}
+	/* copy raw bins without scaling them */
+	memcpy(fft_sample.data, bins, SPECTRAL_HT20_NUM_BINS);
+	fft_sample.max_exp = mag_info->max_exp & 0xf;
 
 	fft_sample.max_magnitude = spectral_max_magnitude(mag_info->all_bins);
 	fft_sample.max_magnitude = __cpu_to_be16(fft_sample.max_magnitude);
-- 
1.7.10.4

RE: [PATCH] ath9k: Save spectral scan data in network byteorder

[Chadd, Adrian]

Hi,

Yes, I advocate doing it all in userspace.

I'm just exporting the raw PHY errors via bpf, so I can just expose all PHY errors this way and let the userland application do what it wants with this.

I do the same with radar data too, fwiw.



Adrian

________________________________________
From: Zefir Kurtisi [zefir.kurtisi@neratec.com]
Sent: Wednesday, 16 January 2013 1:53 AM
To: Simon Wunderlich
Cc: Sven Eckelmann; linux-wireless@vger.kernel.org; ath9k-devel@lists.ath9k.org; Simon Wunderlich; tobias.steinicke@net.t-labs.tu-berlin.de; mathias.kretschmer@fokus.fraunhofer.de; Chadd, Adrian
Subject: Re: [PATCH] ath9k: Save spectral scan data in network byteorder

On 01/16/2013 09:59 AM, Simon Wunderlich wrote:
> On Tue, Jan 15, 2013 at 01:02:43PM +0100, Sven Eckelmann wrote:
>> The sample data received through the spectral scan can be either in big or
>> little endian byteorder. This information isn't stored in the output file.
>> Therefore it is not possible for the analyzer software to find the correct byte
>> order.
>>
>> It is relative common to get the data from a low end AP in big endian mode and
>> transfer it to another computer in little endian mode to analyze it. Therefore,
>> it would be better to store it in network (big endian) byte order.
>
> I'd agree that changing the byteorder to the "general" network order format is
> a good idea, although we will break compatibility again - which should hopefully
> be no problem as the original patch is pretty new anyway.
>
> I was pondering about performance loss when we run spectral on the same machine
> (i.e. maybe adding two useless byteswaps per value). OTOH the byteswaps are pretty
> cheap and we do some computations anyway, so the performance difference should be quite
> small.
>
> Therefore,
>
> Acked-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
>
> (I've CCd some more people in the reply so they can scream if they don't like it. ;])
>
> Thanks,
>       Simon
>
Hi Simon,

at this point, I'd like to advocate the approach I am currently using that does
the conversion fully in user space. There, each sample provided consists of the
fft data as bytes plus the values required to scale the magnitude values back to
absolute power numbers (i.e. the 'bug-fixed' raw data).

Since for the scaling an user space component is mandatory (FP calculations), I
miss to see the benefit of splitting post-processing in the kernel (shifting bins)
and user space. This step doubles fft data size and introduces endianess issues
for no (at least to me) evident reason.

Maybe you have a host application in mind where it does not really matter where
the processing is done. Whereas, if you operate an AP as continuous spectral
scanner, shifting the post-processing to the point where it is evaluated /
displayed (usually your fat PC) significantly impacts your AP's load and its
capability to provide higher sampling rates.

I'd assume Adrian's proposal to just push out the raw spectral data to user-space
is also targeting at that, which I fully support. The only thing I would keep in
the driver is fixing the fft data corruption caused by known chip bugs.


Cheers,
Zefir
>>
>> Signed-off-by: Sven Eckelmann <sven@open-mesh.com>
>> Cc: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
>> ---
>> A patch for FFT_eval was also sent to the author.
>>
>>  drivers/net/wireless/ath/ath9k/recv.c |   10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
>> index d7c129b..2fac787 100644
>> --- a/drivers/net/wireless/ath/ath9k/recv.c
>> +++ b/drivers/net/wireless/ath/ath9k/recv.c
>> @@ -1064,8 +1064,9 @@ static void ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
>>
>>      fft_sample.tlv.type = ATH_FFT_SAMPLE_HT20;
>>      fft_sample.tlv.length = sizeof(fft_sample) - sizeof(fft_sample.tlv);
>> +    fft_sample.tlv.length = __cpu_to_be16(fft_sample.tlv.length);
>>
>> -    fft_sample.freq = ah->curchan->chan->center_freq;
>> +    fft_sample.freq = __cpu_to_be16(ah->curchan->chan->center_freq);
>>      fft_sample.rssi = fix_rssi_inv_only(rs->rs_rssi_ctl0);
>>      fft_sample.noise = ah->noise;
>>
>> @@ -1106,13 +1107,16 @@ static void ath_process_fft(struct ath_softc *sc, struct ieee80211_hdr *hdr,
>>      mag_info = ((struct ath_ht20_mag_info *)radar_info) - 1;
>>
>>      /* Apply exponent and grab further auxiliary information. */
>> -    for (i = 0; i < SPECTRAL_HT20_NUM_BINS; i++)
>> +    for (i = 0; i < SPECTRAL_HT20_NUM_BINS; i++) {
>>              fft_sample.data[i] = bins[i] << mag_info->max_exp;
>> +            fft_sample.data[i] = __cpu_to_be16(fft_sample.data[i]);
>> +    }
>>
>>      fft_sample.max_magnitude = spectral_max_magnitude(mag_info->all_bins);
>> +    fft_sample.max_magnitude = __cpu_to_be16(fft_sample.max_magnitude);
>>      fft_sample.max_index = spectral_max_index(mag_info->all_bins);
>>      fft_sample.bitmap_weight = spectral_bitmap_weight(mag_info->all_bins);
>> -    fft_sample.tsf = tsf;
>> +    fft_sample.tsf = __cpu_to_be64(tsf);
>>
>>      ath_debug_send_fft_sample(sc, &fft_sample.tlv);
>>  #endif
>> --
>> 1.7.10.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html