Re: [PATCH 8/8] d80211/bcm43xx: Modify ieee80211_ioctl.c for wireless statistics.

[Larry Finger <Larry.Finger@lwfinger.net>]

Dan Williams wrote:
> On Tue, 2006-08-08 at 19:00 -0500, Larry Finger wrote:
>> Part 8 of 8 to add wireless statistics to the bcm43xx-d80211 system.
>> This patch adds the appropriate range parameters and routine
>> ieee80211_get_wireless_stats to ieee80211_ioctl.c.
>>
>> The patch is for the August 8 version of Linville's wireless-dev tree.
>>
>> Signed-Off-By: Larry Finger <Larry.Finger@lwfinger.net>
>>
>>
>> diff --git a/net/d80211/ieee80211_ioctl.c b/net/d80211/ieee80211_ioctl.c
>> index dd52555..3a1b97f 100644
>> --- a/net/d80211/ieee80211_ioctl.c
>> +++ b/net/d80211/ieee80211_ioctl.c
>> @@ -1525,6 +1525,16 @@ static int ieee80211_ioctl_giwrange(stru
>>   	range->min_frag = 256;
>>   	range->max_frag = 2346;
>>
>> +	range->max_qual.qual = 100;
>> +	range->max_qual.level = 152;  /* set floor at -104 dBm (152 - 256) */
>> +	range->max_qual.noise = 152;
>> +	range->max_qual.updated = IW_QUAL_ALL_UPDATED;
>> +
>> +	range->avg_qual.qual = 50;
>> +	range->avg_qual.level = 0;
>> +	range->avg_qual.noise = 0;
>> +	range->avg_qual.updated = IW_QUAL_ALL_UPDATED;
>> +
>>   	return 0;
>>   }
>>
>> @@ -2963,6 +2973,39 @@ static int ieee80211_ioctl_giwauth(struc
>>   	return ret;
>>   }
>>
>> +/* Get wireless statistics.  Called by /proc/net/wireless and by SIOCGIWSTATS */
>> +static struct iw_statistics *ieee80211_get_wireless_stats(struct net_device *net_dev)
>> +{
>> +	struct ieee80211_local *local = net_dev->ieee80211_ptr;
>> +	struct iw_statistics * wstats = &local->wstats;
>> +	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(net_dev);
>> +	struct sta_info *sta;
>> +	static int tmp_level = 0;
>> +	static int tmp_qual = 0;
>> +
>> +	sta = sta_info_get(local, sdata->u.wds.remote_addr);
>> +	if (!sta) {
>> +		wstats->discard.fragment = 0;
>> +		wstats->discard.misc = 0;
>> +		wstats->qual.qual = 0;
>> +		wstats->qual.level = 0;
>> +		wstats->qual.noise = 0;
>> +		wstats->qual.updated |= IW_QUAL_ALL_UPDATED;
> 
> What does the 'if (!sta)' check do?  What's the case here?  If the card
> is not connected an access point or otherwise unconnected to anything
> else, you should use IW_QUAL_ALL_INVALID here I think, because signal
> quality has no meaning when you're disconnected (except possibly
> background noise).

The check is mostly to prevent NULL pointers in the else branch, but your 
suggestion of INVALID is good.

> 
>> +	} else {
>> +		if (!tmp_level) {       /* get initial values */
>> +			tmp_level = sta->last_signal;
>> +			tmp_qual = sta->last_rssi;
>> +		} else {                        /* smooth results */
>> +			tmp_level = (15 * tmp_level + sta->last_signal)/16;
>> +			tmp_qual = (15 * tmp_qual + sta->last_rssi)/16;
>> +		}
>> +		wstats->qual.level = tmp_level;
>> +		wstats->qual.qual = 100*tmp_qual/sta->max_rssi;
>> +		wstats->qual.noise = sta->last_noise;
>> +		wstats->qual.updated = IW_QUAL_ALL_UPDATED | IW_QUAL_DBM;
> 
> By using IW_QUAL_DBM, you're forcing all cards that use d80211 to report
> statistics in dBm.  I think that's a _good_ thing personally (it brings
> some consistency to the whole wireless quality pit-of-doom), but I want
> to make sure this is actually what you want to do here.

 From what I learned in our previous discussions, that is exactly what I meant.

Thanks for your comments.

Larry