[PATCH V2] mac80211: Implementation of SIOCSIWRATE

[PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Larry Finger]

From: Mohamed Abbas <mabbas@linux.intel.com>

The WEXT ioctl SIOCSIWRATE is not implemented in mac80211. This patch
adds the missing routine. It supports the 'auto' keyword, fixed rates,
and the combination of 'auto' and a fixed rate to select an upper bound.

Signed-off-by: Mohamed Abbas <mabbas@linux.intel.com>
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

 ieee80211_ioctl.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

Index: wireless-dev/net/mac80211/ieee80211_ioctl.c
===================================================================
--- wireless-dev.orig/net/mac80211/ieee80211_ioctl.c
+++ wireless-dev/net/mac80211/ieee80211_ioctl.c
@@ -2013,6 +2013,55 @@ static int ieee80211_ioctl_giwscan(struc
 }
 
 
+static int ieee80211_ioctl_siwrate(struct net_device *dev,
+				  struct iw_request_info *info,
+				  struct iw_param *rate, char *extra)
+{
+	struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
+	struct ieee80211_hw_mode *mode;
+	int i;
+	u32 target_rate = rate->value / 100000;
+	u32 supp = 0;
+	struct ieee80211_sub_if_data *sdata;
+	struct sta_info *sta;
+
+	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+	mode = local->oper_hw_mode;
+	/* value = -1, rate->fixed = 0 means auto only, so use all rates
+	 * value = X, rate->fixed = 1 means only rate X
+	 * value = X, rate->fixed = 0 means all rates <= X */
+	for (i=0; i< mode->num_rates; i++) {
+		struct ieee80211_rate *rates = &mode->rates[i];
+		int target = rates->rate;
+
+		if (mode->mode == MODE_ATHEROS_TURBO ||
+		    mode->mode == MODE_ATHEROS_TURBOG)
+			target *= 2;
+		if ((target_rate == target) && rate->fixed) {
+			supp |= BIT(i);
+			sdata->u.ap.force_unicast_rateidx = i;
+			sdata->u.ap.max_ratectrl_rateidx = i;
+			break;
+		}
+		if ((!rate->fixed && (target <= target_rate)) || (rate->value < 0)) {
+			supp |= BIT(i);
+			sdata->u.ap.force_unicast_rateidx = -1;
+			if (target_rate == target)
+				sdata->u.ap.max_ratectrl_rateidx = i;
+		}
+	}
+	sdata->u.sta.supp_rates_bits = supp;
+	if (sdata->type == IEEE80211_IF_TYPE_STA) {
+		sta = sta_info_get(local, sdata->u.sta.bssid);
+		if (sta) {
+			sta->txrate = sdata->u.ap.max_ratectrl_rateidx;
+			sta->supp_rates = supp;
+			sta_info_put(sta);
+		}
+	}
+	return 0;
+}
+
 static int ieee80211_ioctl_giwrate(struct net_device *dev,
 				  struct iw_request_info *info,
 				  struct iw_param *rate, char *extra)
@@ -3159,7 +3208,7 @@ static const iw_handler ieee80211_handle
 	(iw_handler) NULL,				/* SIOCGIWNICKN */
 	(iw_handler) NULL,				/* -- hole -- */
 	(iw_handler) NULL,				/* -- hole -- */
-	(iw_handler) NULL,				/* SIOCSIWRATE */
+	(iw_handler) ieee80211_ioctl_siwrate,		/* SIOCSIWRATE */
 	(iw_handler) ieee80211_ioctl_giwrate,		/* SIOCGIWRATE */
 	(iw_handler) ieee80211_ioctl_siwrts,		/* SIOCSIWRTS */
 	(iw_handler) ieee80211_ioctl_giwrts,		/* SIOCGIWRTS */

Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Michael Wu]

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

On Thursday 07 June 2007 17:43, Larry Finger wrote:
> +	sdata->u.sta.supp_rates_bits = supp;
Can't do this unless you verify sdata->type is IEEE80211_IF_TYPE_STA or 
IEEE80211_IF_TYPE_IBSS.

Also, what rate(s) we choose to TX at doesn't have much to do with what rates 
we actually support. I don't think this should be set.

> +	if (sdata->type == IEEE80211_IF_TYPE_STA) {
> +		sta = sta_info_get(local, sdata->u.sta.bssid);
> +		if (sta) {
> +			sta->txrate = sdata->u.ap.max_ratectrl_rateidx;
This is automatically set as soon as the next frame is TXed, as said before in 
the previous review.

> +			sta->supp_rates = supp;
This is definitely wrong. What rates we support has nothing to do with what 
rates the AP supports.

One last thing - max_ratectrl_rateidx should be set to -1 to indicate no 
maximum. Not doing this will cause problems when switching from one hw mode 
to another with a different number of rates.

-Michael Wu

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

Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Larry Finger]

Michael Wu wrote:
> On Thursday 07 June 2007 17:43, Larry Finger wrote:
>> +	if (sdata->type == IEEE80211_IF_TYPE_STA) {
>> +		sta = sta_info_get(local, sdata->u.sta.bssid);
>> +		if (sta) {
>> +			sta->txrate = sdata->u.ap.max_ratectrl_rateidx;
> This is automatically set as soon as the next frame is TXed, as said before in 
> the previous review.

When I leave this part out, the rate _NEVER_ gets set, no matter what you say. My experiments say 
that it is needed.

Larry

Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Michael Wu]

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

On Thursday 07 June 2007 19:56, Larry Finger wrote:
> When I leave this part out, the rate _NEVER_ gets set, no matter what you
> say. My experiments say that it is needed.
>
That's because force_unicast_rateidx and max_ratectrl_rateidx is being set in 
the wrong struct ieee80211_if_ap. (how did I miss that..) sdata->u.ap is only 
valid when sdata->type == IEEE80211_IF_TYPE_AP, yet it's being accessed 
without any mode checks. sdata->bss should be used instead.

-Michael Wu

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

Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Larry Finger]

Michael Wu wrote:
> On Thursday 07 June 2007 19:56, Larry Finger wrote:
>> When I leave this part out, the rate _NEVER_ gets set, no matter what you
>> say. My experiments say that it is needed.
>>
> That's because force_unicast_rateidx and max_ratectrl_rateidx is being set in 
> the wrong struct ieee80211_if_ap. (how did I miss that..) sdata->u.ap is only 
> valid when sdata->type == IEEE80211_IF_TYPE_AP, yet it's being accessed 
> without any mode checks. sdata->bss should be used instead.

Does sdata->bss need to be checked to verify that it is not NULL?

Larry

Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Michael Wu]

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

On Monday 11 June 2007 09:19, Larry Finger wrote:
> Does sdata->bss need to be checked to verify that it is not NULL?
>
Yup.

-Michael Wu

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

Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Larry Finger]

Michael Wu wrote:
> On Thursday 07 June 2007 17:43, Larry Finger wrote:

>> +			sta->supp_rates = supp;
> This is definitely wrong. What rates we support has nothing to do with what 
> rates the AP supports.

If sta->supp_rates is not changed, how do you lock in a single rate as would be expected from an 
'iwconfig ethX rate 11M'? Won't the following code in the rate_control_rate_dec routine of 
rc80211_simple just decrease the rate if it wants to?

         while (i > 0) {
                 i--;
                 if (sta->supp_rates & BIT(i) &&
                     mode->rates[i].flags & IEEE80211_RATE_SUPPORTED) {
                         sta->txrate = i;
                         break;
                 }
         }


Larry

Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Michael Wu]

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

On Thursday 07 June 2007 21:02, Larry Finger wrote:
> If sta->supp_rates is not changed, how do you lock in a single rate as
> would be expected from an 'iwconfig ethX rate 11M'? Won't the following
> code in the rate_control_rate_dec routine of rc80211_simple just decrease
> the rate if it wants to?
>
>          while (i > 0) {
>                  i--;
>                  if (sta->supp_rates & BIT(i) &&
>                      mode->rates[i].flags & IEEE80211_RATE_SUPPORTED) {
>                          sta->txrate = i;
>                          break;
>                  }
>          }
>
No, because the code right above it checks for force_unicast_rateidx and exits 
if it's set.

-Michael Wu

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

Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Larry Finger]

Michael Wu wrote:
> 
> One last thing - max_ratectrl_rateidx should be set to -1 to indicate no 
> maximum. Not doing this will cause problems when switching from one hw mode 
> to another with a different number of rates.

I don't understand this last part. If we use the upper-limit form of the rate command, isn't that 
indicated to the rate-setting routine by having max_ratectrl_index set to the upper limit, and 
force_unicast_rateidx set to -1? If max_ratectrl_rateidx must always be -1, why does it exist as a 
parameter?

Larry

Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Michael Wu]

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

On Tuesday 12 June 2007 13:35, Larry Finger wrote:
> Michael Wu wrote:
> > One last thing - max_ratectrl_rateidx should be set to -1 to indicate no
> > maximum. Not doing this will cause problems when switching from one hw
> > mode to another with a different number of rates.
>
> I don't understand this last part. If we use the upper-limit form of the
> rate command, isn't that indicated to the rate-setting routine by having
> max_ratectrl_index set to the upper limit, and force_unicast_rateidx set to
> -1?
Yes. However, when you're not setting the upper limit, max_ratectrl_index 
needs to be set to -1. The current code sets max_ratectrl_index to the 
highest rate index it can find to configure full auto rate selection or fixed 
rate when it should set -1 instead.

-Michael Wu

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

[PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Larry Finger]

The initial rate for STA's using rc80211_simple is set to the last
rate in the rate table. For situations for which the signal is weak,
the rate may be too high for authentication and association. Although
the rc80211_simple module will adjust the speed, the response may not
be fast enough for a successful connection. This modification sets the
initial rate to the lowest supported value.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---


Index: wireless-dev/net/mac80211/rc80211_simple.c
===================================================================
--- wireless-dev.orig/net/mac80211/rc80211_simple.c
+++ wireless-dev/net/mac80211/rc80211_simple.c
@@ -281,16 +281,21 @@ static void rate_control_simple_rate_ini
 {
 	struct ieee80211_hw_mode *mode;
 	int i;
+	int min_rate = 1000;
+
 	sta->txrate = 0;
 	mode = local->oper_hw_mode;
-	/* TODO: what is a good starting rate for STA? About middle? Maybe not
-	 * the lowest or the highest rate.. Could consider using RSSI from
-	 * previous packets? Need to have IEEE 802.1X auth succeed immediately
-	 * after assoc.. */
-	for (i = 0; i < mode->num_rates; i++) {
+	/* TODO: This routine should consider using RSSI from previous packets
+	 * as we need to have IEEE 802.1X auth succeed immediately after assoc..
+	 * Until that method is implemented, we will use the lowest supported rate
+	 * as a workaround, */
+	for (i = 0; i < mode->num_rates; i++)
 		if ((sta->supp_rates & BIT(i)) &&
-		    (mode->rates[i].flags & IEEE80211_RATE_SUPPORTED))
-			sta->txrate = i;
+		    (mode->rates[i].flags & IEEE80211_RATE_SUPPORTED)) {
+			if (mode->rates[i].rate < min_rate) {
+				min_rate = mode->rates[i].rate;
+				sta->txrate = i;
+			}
 	}
 }
 

Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

[Larry Finger]

Larry Finger wrote:
> The initial rate for STA's using rc80211_simple is set to the last
> rate in the rate table. For situations for which the signal is weak,
> the rate may be too high for authentication and association. Although
> the rc80211_simple module will adjust the speed, the response may not
> be fast enough for a successful connection. This modification sets the
> initial rate to the lowest supported value.
> 
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---

Sorry, I attached the wrong patch. Please ignore.

Larry