Re: [PATCH v2 5/6] mac80211: Add more ethtools stats: survey, rates, etc

[Ben Greear <greearb@candelatech.com>]

On 04/17/2012 06:41 PM, Johannes Berg wrote:
> On Tue, 2012-04-17 at 10:46 -0700, greearb@candelatech.com wrote:
>> +	/* Get survey stats for current channel */
>> +	survey.filled = 0;
>> +	if (drv_get_survey(local, 0,&survey) != 0) {
>> +		survey.filled = 0;
>> +		data[i++] = 0;
>> +	} else {
>> +		/* ath9k (and maybe others??) only updates internal stats
>> +		 * when you get channel index 0, so if
>> +		 * we are NOT on channel zero, get the real stats
>> +		 * now.
>> +		 */
>> +		int ch_idx = ieee80211_get_channel_idx(local->hw.wiphy,
>> +			local->oper_channel->center_freq);
>> +		if (ch_idx == 0) {
>> +			data[i++] = survey.channel->center_freq;
>> +		} else {
>> +			survey.filled = 0;
>> +			if (drv_get_survey(local, ch_idx,&survey) != 0) {
>> +				survey.filled = 0;
>> +				data[i++] = 0;
>> +			} else {
>> +				data[i++] = survey.channel->center_freq;
>> +			}
>> +		}
>> +	}
>
> This is completely incomprehensible to me. I don't think the channel
> index is what you think it is?

It is ugly, but as far as I can tell it works.  The survey method
wants an index, not channel number or frequency, from what I can tell
(I just looked at and tested ath9k).

Based on the freq returned as part of the survey results, the logic
above appears to function as I would hope.

That said, a new get-survey() method that took a channel object instead of an
index would make this code cleaner.  Or, we could store the channel
index in the channel object for fast lookup, but I was afraid storing the
index would be too risky in case channels are ever moved around for some reason
or another.

And, I could be missing something yet....

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com