Re: [PATCH] Add wireless statics to bcm43xx-d80211

2006-08-23 Thread Johannes Berg
On Tue, 2006-08-22 at 16:15 -0500, Larry Finger wrote:
  +  int maxssi;
  
  Why is maxssi here? Can it really change between received frames?
 
 No it cannot change between frames; however, the max value can be very 
 different for different 
 drivers using d80211. On the bcm43xx, it is 60; whereas 100 seems to be a 
 better value for the 
 rt2x00 chips. Adding it here seemed like a good way to handle this situation. 
 Do you suggest 
 something else?

I think the question was intended to be: why is it in
ieee80211_rx_status and not in struct ieee80211_hw?

 Again to pass the differing values for different drivers.

Ditto here. Just stick it into struct ieee80211_hw instead.

  I would suggest using -110 dBm as a floor (to be compatible with RCPI
  definition, see mail from Simon Barber describing it). Or is there any
  particular reason for -104 dBm?
 
 It is the value previously used in the softmac version of bcm43xx. A value of 
 -110 would obviously 
 be better.

Who maintains the softmac version now? :P
I'd suggest to just change it there too for consistency.

johannes
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Add wireless statics to bcm43xx-d80211

2006-08-23 Thread Jiri Benc
On Tue, 22 Aug 2006 16:15:47 -0500, Larry Finger wrote:
 No it cannot change between frames; however, the max value can be very 
 different for different 
 drivers using d80211. On the bcm43xx, it is 60; whereas 100 seems to be a 
 better value for the 
 rt2x00 chips. Adding it here seemed like a good way to handle this situation. 
 Do you suggest 
 something else?

As Johannes already said, it should be in ieee80211_hw. 

  [...]
  --- a/net/d80211/ieee80211_i.h
  +++ b/net/d80211/ieee80211_i.h
  @@ -337,6 +337,9 @@ struct ieee80211_local {
 struct net_device *apdev; /* wlan#ap - management frames (hostapd) */
 int open_count;
 int monitors;
  +  int link_quality;
  +  int noise;
  +  struct iw_statistics wstats;
  
  Why are these three variables in ieee80211_local? They are not used
  anywhere.
 
 You are right about the first two; however, wstats is used in the new
 routine ieee80211_get_wireless_stats.

Oh, get_wireless_stats returns struct iw_statistics, so it needs to be
allocated all the time. Grr, that's stupid...

Okay, wstats really needs to be in ieee80211_local then.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Add wireless statics to bcm43xx-d80211

2006-08-22 Thread Jiri Benc
On Mon, 14 Aug 2006 08:29:08 -0500, Larry Finger wrote:
 This patch implements wireless statistics for bcm43xx using the d80211 
 stack.It 
 also sets a framework for the implementation in other drivers that use the 
 d80211 code. The component parts have been circulated on the netdev mailing 
 list, and all suggested changes have been incorporated. The specific changes 
 are 
 as follows:

Please, separate the d80211 part and the bcm43xx-d80211 part into
two patches.

 --- a/include/net/d80211.h
 +++ b/include/net/d80211.h
 @@ -205,6 +205,9 @@ struct ieee80211_rx_status {
  int channel;
  int phymode;
  int ssi;
 + int maxssi;

Why is maxssi here? Can it really change between received frames?

 [...]
 --- a/net/d80211/ieee80211.c
 +++ b/net/d80211/ieee80211.c
 @@ -3174,6 +3174,9 @@ ieee80211_rx_h_sta_process(struct ieee80
   sta-rx_fragments++;
   sta-rx_bytes += rx-skb-len;
   sta-last_rssi = rx-u.rx.status-ssi;
 + sta-last_signal = rx-u.rx.status-signal;
 + sta-last_noise = rx-u.rx.status-noise;
 + sta-max_rssi = rx-u.rx.status-maxssi;

Again, I see no reason why max_rssi should be in sta structure.

 [...]
 --- a/net/d80211/ieee80211_i.h
 +++ b/net/d80211/ieee80211_i.h
 @@ -337,6 +337,9 @@ struct ieee80211_local {
   struct net_device *apdev; /* wlan#ap - management frames (hostapd) */
   int open_count;
   int monitors;
 + int link_quality;
 + int noise;
 + struct iw_statistics wstats;

Why are these three variables in ieee80211_local? They are not used
anywhere.

 [...]
 --- a/net/d80211/ieee80211_ioctl.c
 +++ b/net/d80211/ieee80211_ioctl.c
 @@ -1580,6 +1580,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) */

I would suggest using -110 dBm as a floor (to be compatible with RCPI
definition, see mail from Simon Barber describing it). Or is there any
particular reason for -104 dBm?

 [...]
 --- a/net/d80211/sta_info.h
 +++ b/net/d80211/sta_info.h
 @@ -82,6 +82,9 @@ struct sta_info {
   unsigned long rx_dropped; /* number of dropped MPDUs from this STA */
  
   int last_rssi; /* RSSI of last received frame from this STA */
 + int last_signal; /* signal of last received frame from this STA */
 + int last_noise; /* noise of last received frame from this STA */

Add these two variables also to sysfs, please.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Add wireless statics to bcm43xx-d80211

2006-08-22 Thread Larry Finger

Jiri Benc wrote:

On Mon, 14 Aug 2006 08:29:08 -0500, Larry Finger wrote:
This patch implements wireless statistics for bcm43xx using the d80211 stack.It 
also sets a framework for the implementation in other drivers that use the 
d80211 code. The component parts have been circulated on the netdev mailing 
list, and all suggested changes have been incorporated. The specific changes are 
as follows:


Please, separate the d80211 part and the bcm43xx-d80211 part into
two patches.


OK. I split it into parts for the initial RFC submission, then combined them after I thought the 
comments were all in. Sorry.



--- a/include/net/d80211.h
+++ b/include/net/d80211.h
@@ -205,6 +205,9 @@ struct ieee80211_rx_status {
 int channel;
 int phymode;
 int ssi;
+   int maxssi;


Why is maxssi here? Can it really change between received frames?


No it cannot change between frames; however, the max value can be very different for different 
drivers using d80211. On the bcm43xx, it is 60; whereas 100 seems to be a better value for the 
rt2x00 chips. Adding it here seemed like a good way to handle this situation. Do you suggest 
something else?



[...]
--- a/net/d80211/ieee80211.c
+++ b/net/d80211/ieee80211.c
@@ -3174,6 +3174,9 @@ ieee80211_rx_h_sta_process(struct ieee80
sta-rx_fragments++;
sta-rx_bytes += rx-skb-len;
sta-last_rssi = rx-u.rx.status-ssi;
+   sta-last_signal = rx-u.rx.status-signal;
+   sta-last_noise = rx-u.rx.status-noise;
+   sta-max_rssi = rx-u.rx.status-maxssi;


Again, I see no reason why max_rssi should be in sta structure.


Again to pass the differing values for different drivers.


[...]
--- a/net/d80211/ieee80211_i.h
+++ b/net/d80211/ieee80211_i.h
@@ -337,6 +337,9 @@ struct ieee80211_local {
struct net_device *apdev; /* wlan#ap - management frames (hostapd) */
int open_count;
int monitors;
+   int link_quality;
+   int noise;
+   struct iw_statistics wstats;


Why are these three variables in ieee80211_local? They are not used
anywhere.


You are right about the first two; however, wstats is used in the new
routine ieee80211_get_wireless_stats.


[...]
--- a/net/d80211/ieee80211_ioctl.c
+++ b/net/d80211/ieee80211_ioctl.c
@@ -1580,6 +1580,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) */


I would suggest using -110 dBm as a floor (to be compatible with RCPI
definition, see mail from Simon Barber describing it). Or is there any
particular reason for -104 dBm?


It is the value previously used in the softmac version of bcm43xx. A value of -110 would obviously 
be better.



[...]
--- a/net/d80211/sta_info.h
+++ b/net/d80211/sta_info.h
@@ -82,6 +82,9 @@ struct sta_info {
unsigned long rx_dropped; /* number of dropped MPDUs from this STA */
 
 	int last_rssi; /* RSSI of last received frame from this STA */

+   int last_signal; /* signal of last received frame from this STA */
+   int last_noise; /* noise of last received frame from this STA */


Add these two variables also to sysfs, please.


Will be done.

Larry
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html