2007-06-08 00:44:14

by Larry Finger

[permalink] [raw]
Subject: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

From: Mohamed Abbas <[email protected]>

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 <[email protected]>
Signed-off-by: Larry Finger <[email protected]>
---

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 */


2007-06-08 03:13:38

by Michael Wu

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

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


Attachments:
(No filename) (479.00 B)
(No filename) (189.00 B)
Download all attachments

2007-06-11 16:19:11

by Larry Finger

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

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

2007-06-11 17:11:55

by Michael Wu

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

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


Attachments:
(No filename) (138.00 B)
(No filename) (189.00 B)
Download all attachments

2007-06-08 00:45:08

by Larry Finger

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

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 <[email protected]>
> ---

Sorry, I attached the wrong patch. Please ignore.

Larry

2007-06-08 04:02:14

by Larry Finger

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

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

2007-06-12 20:51:30

by Michael Wu

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

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


Attachments:
(No filename) (788.00 B)
(No filename) (189.00 B)
Download all attachments

2007-06-08 02:56:56

by Larry Finger

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

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

2007-06-08 05:08:13

by Michael Wu

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

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


Attachments:
(No filename) (700.00 B)
(No filename) (189.00 B)
Download all attachments

2007-06-12 20:35:12

by Larry Finger

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

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

2007-06-08 01:20:34

by Michael Wu

[permalink] [raw]
Subject: Re: [PATCH V2] mac80211: Implementation of SIOCSIWRATE

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


Attachments:
(No filename) (938.00 B)
(No filename) (189.00 B)
Download all attachments