Re: [PATCH 2/7] d80211: add support for SIOCSIWRATE and SIOCGIWRATE

[Mohamed Abbas <mabbas@linux.intel.com>]

Jouni Malinen wrote:

>On Mon, Aug 28, 2006 at 01:45:34PM -0700, mabbas wrote:
>
>  
>
>>This patch modify d80211 to add SIOCSIWRATE and SIOCGIWRATE
>>commands. this patch almost does the same thing as in 
>>PRISM2_HOSTAPD_SET_RATE_SETS.
>>    
>>
>
>I don't think I would like to get this applied since this seems to be
>changing the design on how the per-STA TX rate limiting is done in a
>way that does not match the original design and no justification has
>been given for that change so far. Some comments below.
>  
>
per-STA rate limiting is enforced by what the AP beacon supported rates 
values . this code is to enforce rate limiting for this interface 
regarding to any AP we join. looking at the code the overall rate 
restriction will be a combination of these two restrictions. this code 
is to implement for the below description in iwconfig man page.

rate/bit[rate]
For cards supporting multiple bit rates, set the bit-rate in b/s. The 
bit-rate is the speed at
which bits are transmitted over the medium, the user speed of the link 
is lower due to medium
sharing and various overhead.
You may append the suffix k, M or G to the value (decimal multiplier : 
10^3, 10^6 and 10^9 b/s),
or add enough ق0ق. Values below 1000 are card specific, usually an index 
in the bit-rate list. Use
auto to select automatic bit-rate mode (fallback to lower rate on noisy 
channels), which is the
default for most cards, and fixed to revert back to fixed setting. If 
you specify a bit-rate value
and append auto, the driver will use all bit-rates lower and equal than 
this value.
Examples :
iwconfig eth0 rate 11M
iwconfig eth0 rate auto
iwconfig eth0 rate 5.5M auto

>
>  
>
>>--- a/net/d80211/ieee80211_i.h
>>@@ -280,6 +280,9 @@ #define IEEE80211_AUTH_ALG_LEAP BIT(2)
>>    struct sk_buff *probe_resp; /* ProbeResp template for IBSS */
>>    u32 supp_rates_bits;
>> 
>>+	u32 last_rate; /* last tx data rate value. management and multi cast frame
>>+			* wont be used. */
>>    
>>
>
>Is this information valuable enough to collect with all the extra code
>in ieee80211_tx_h_rate_ctrl()? The last rate can be fetched from the STA
>entry. It won't be the exact same value as this one here, but then
>again, I don't really see much point in reporting the last used TX rate
>since it can be changing a lot. Some sort of average over the last N
>frames could be more useful information to collect, if that level of
>detail is needed.
>  
>
The idea here is to report the last rate performed with this link once 
requested by iwconfig so the user
can know the performance of the link. I can add some averaging but 
really the user want to see last value. I thought of using STA entry
but in case IBSS mode there maybe many of them which once to report 
adding this entry will report last rate used by all of the station.

>
>  
>
>>--- a/net/d80211/ieee80211_ioctl.c
>>+++ b/net/d80211/ieee80211_ioctl.c
>>@@ -2138,6 +2138,103 @@ static int ieee80211_ioctl_giwretry(stru
>> 	return 0;
>> }
>> 
>>+static int ieee80211_ioctl_siwrate(struct net_device *dev,
>>+                                struct iw_request_info *info,
>>+                                union iwreq_data *wrqu, char *extra)
>>+{
>>+	struct ieee80211_local *local = dev->ieee80211_ptr;
>>+	int i, j;
>>+	u32 target_rate = wrqu->bitrate.value /100000;
>>+	u32 fixed;
>>+	int *old_supp = local->supp_rates[local->conf.phymode];
>>+	int *supp = NULL;
>>+
>>+	/* value = -1, fixed = 0 means auto only, so we should use
>>+	 * all rates offered by AP
>>+	 * value = X, fixed = 1 means only rate X
>>+	* value = X, fixed = 0 means all rates lower equal X */
>>    
>>
>
>Please keep in mind that this function can also be called in AP mode.
>  
>
I can check for STA and IBSS only

>  
>
>>+	fixed = wrqu->bitrate.fixed;
>>+	supp = (int *) kmalloc((local->num_curr_rates + 1) *
>>+					sizeof(int), GFP_KERNEL);
>>+	if (!supp)
>>+		return 0;
>>    
>>
>
>return -ENOMEM
>  
>
I will change it to return -ENOMEM

>  
>
>>+	j = 0;
>>+	for (i=0; i< local->num_curr_rates; i++) {
>>+		struct ieee80211_rate *rate = &local->curr_rates[i];
>>+
>>+		if (target_rate == rate->rate) {
>>+			supp[j++] = rate->rate;
>>+			break;
>>+		} else if (!fixed)
>>+			supp[j++] = rate->rate;
>>+	}
>>    
>>
>
>This can allow number of invalid configurations; especially so, since
>there is no synchronization with basic reate sets here. In addition, I
>would not really want to change the supported/basic rate sets this way.
>If there is desire to limit what rates the TX rate control algorithm is
>using, this should be done by modifying per-STA entry data
>(sta->supp_rates), not the per-radio rate table.
>  
>
I am calling ieee80211_prepare_rates which will sync basic rate for us. 
again per-STA will be a problem for IBSS since I need to go to each 
station and apply this, and what about new added station I need to 
remember to apply this again, I think here is where it belong.

>  
>
>>+	/* number of supported rate equal to all current supported rate
>>+	 * this equal like supp_rates = NULL so save process time and set
>>+	* supp to NULL
>>+	*/
>>+	if ((j >= local->num_curr_rates) || (j == 0)) {
>>+		kfree(supp);
>>+		supp = NULL;
>>    
>>
>
>Shouldn't these return an error and not replace the current rate
>configuration?
>  
>
if we reach here then we support all rates available at this point it is 
the same as setting supp_rates = null, this will be faster so we
dont need to search supp _rates. if supp_rates is null then all rate 
supported. as this code in ieee80211_prepare_rates show

if (local->supp_rates[local->conf.phymode]) {
if (!rate_list_match(local->supp_rates
[local->conf.phymode],
rate->rate))
continue;
}

>  
>
>>+	if (old_supp)
>>+		kfree(old_supp);
>>    
>>
>
>  
>
this pointer could be NULL

>No need for 'if (old_supp)' before calling kfree(old_supp).
>
>  
>