d80211: add support for SIOCSIWRATE and SIOCGIWRATE
Subsystems (2)
PATCH 2/7 d80211: add support for SIOCSIWRATE and SIOCGIWRATE
This patch modify d80211 to add SIOCSIWRATE and SIOCGIWRATE commands. this patch almost does the same thing as in PRISM2_HOSTAPD_SET_RATE_SETS.
net/d80211/ieee80211.c +15 -0
@@ -395,6 +395,21 @@ ieee80211_tx_h_rate_ctrl(struct ieee8021 tx->u.tx.control->tx_rate = tx->u.tx.rate->val2; } +/* only data unicast frame */ +if ((tx->u.tx.rate) && tx->skb && tx->sdata && tx->u.tx.unicast && + (tx->sdata->type ==IEEE80211_IF_TYPE_STA || + tx->sdata->type ==IEEE80211_IF_TYPE_IBSS )&& !extra.mgmt_data) { +struct ieee80211_hdr *hdr; +u16 fc; + + hdr = (struct ieee80211_hdr *) tx->skb->data; + fc =le16_to_cpu (hdr->frame_control); + +if ((fc &IEEE80211_FCTL_FTYPE ) ==IEEE80211_FTYPE_DATA ) + tx->sdata->u.sta.last_rate = tx->u.tx.rate->rate * +100000 ; + } +return TXRX_CONTINUE ; }
net/d80211/ieee80211_i.h +3 -0
@@ -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. */ +int wmm_last_param_set; };
net/d80211/ieee80211_ioctl.c +99 -2
113 lines not on this page · raw message
Quoted 3 lines · mabbas
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.
net/d80211/ieee80211_i.h #define IEEE80211_AUTH_ALG_LEAP BIT(2) · line 284
Quoted 7 lines
---
a/net/d80211/ieee80211_i.hnet/d80211/ieee80211_i.h +2
-0@@ -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.
Quoted 11 lines
net/d80211/ieee80211_ioctl.c +5
-0@@ -2138,6 +2138,103 @@ static int ieee80211_ioctl_giwretry(strureturn 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;
net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwretry(stru · line 2155
Quoted 10 lines
net/d80211/ieee80211_ioctl.c +10
-0+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.
net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwretry(stru · line 2166
Quoted 5 lines
net/d80211/ieee80211_ioctl.c +5
-0+ fixed = wrqu->bitrate.fixed; + supp = (int *)kmalloc ((local->num_curr_rates +1 ) * +sizeof (int ),GFP_KERNEL ); +if (!supp) +return 0 ;
return -ENOMEM
net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwretry(stru · line 2177
Quoted 10 lines
net/d80211/ieee80211_ioctl.c +10
-0+ 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.
net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwretry(stru · line 2187
Quoted 7 lines
net/d80211/ieee80211_ioctl.c +7
-0+/* 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?
net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwretry(stru · line 2196
Quoted 2 lines
net/d80211/ieee80211_ioctl.c +2
-0+if (old_supp) +kfree (old_supp);
No need for 'if (old_supp)' before calling kfree(old_supp).
Signature · 1 line
Jouni Malinen PGP id EFC895FA
Quoted 9 lines · Jouni Malinen
Quoted 4 lines · mabbas
This patch modify d80211 to add SIOCSIWRATE and SIOCGIWRATE
commands. this patch almost does the same thing as inPRISM2_HOSTAPD_SET_RATE_SETS.
Quoted 6 lines
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
Quoted 8 lines
Quoted 8 lines
---
a/net/d80211/ieee80211_i.hnet/d80211/ieee80211_i.h +0
-0@@ -280,6 +280,9 @@ #define IEEE80211_AUTH_ALG_LEAP BIT(2)struct sk_buff *probe_resp;/* ProbeResp template for IBSS */ u32 supp_rates_bits;
Quoted 14 lines
net/d80211/ieee80211_i.h #define IEEE80211_AUTH_ALG_LEAP BIT(2)
Quoted 4 lines
net/d80211/ieee80211_i.h +2
-0+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.
Quoted 20 lines
Quoted 20 lines
net/d80211/ieee80211_ioctl.c +11
-0@@ -2138,6 +2138,103 @@ static int ieee80211_ioctl_giwretry(strureturn 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 ; +
Quoted 10 lines
net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwretry(stru
Quoted 6 lines
net/d80211/ieee80211_ioctl.c +4
-0+/* 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
Quoted 13 lines
net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwretry(stru
Quoted 9 lines
net/d80211/ieee80211_ioctl.c +5
-0+ 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
Quoted 13 lines
Quoted 13 lines
net/d80211/ieee80211_ioctl.c +10
-0+ 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; + }
Quoted 10 lines
Quoted 1 line
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.
Quoted 16 lines
net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwretry(stru · line 2187
Quoted 11 lines
net/d80211/ieee80211_ioctl.c +7
-0+/* 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;
}
Quoted 9 lines
net/d80211/ieee80211_ioctl.c static int ieee80211_ioctl_giwretry(stru · line 2196
Quoted 6 lines
net/d80211/ieee80211_ioctl.c +2
-0+if (old_supp) +kfree (old_supp);
this pointer could be NULL
Quoted 4 lines
No need for 'if (
old_supp)' before callingkfree(old_supp).
Quoted 10 lines · Jouni Malinen
Quoted 3 lines · mabbas
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.
I agree with Jouni. We definitely want similar feature but it needs to be carefully designed (and no, we are not limited by the limitations of WE). I'm not going to apply the patch.
Jiri
Signature · 2 lines
Jiri Benc SUSE Labs
Quoted 14 lines · Jiri Benc
Quoted 11 lines · Jouni Malinen
Quoted 3 lines · mabbas
This patch modify d80211 to add SIOCSIWRATE and SIOCGIWRATE
commands. this patch almost does the same thing as inPRISM2_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.
Quoted 7 lines
I agree with Jouni. We definitely want similar feature but it needs to be carefully designed (and no, we are not limited by the limitations of WE). I'm not going to apply the patch.
Jiri
I can not see how does it break per-STA TX rate limit, especially PRISM2_HOSTAPD_SET_RATE_SETS almost doing the same thing. I am not saying the patch is correct I just want to know how to fix it to get it in.
Mohamed
Quoted 3 lines · mabbas
I can not see how does it break per-STA TX rate limit, especially
PRISM2_HOSTAPD_SET_RATE_SETSalmost doing the same thing. I am not saying the patch is correct I just want to know how to fix it to get it in.
As Jouni wrote, it's not useful to change the per-radio rate table. You want to limit the rates you are using to communicate with the current AP while not limiting other virtual interfaces. (Imagine you have the card that is capable to associate to two APs at the same time. You don't want to limit rates for both APs with SIOCSIWRATE.)
To do that I think the following is needed:
1. Add 'allowed_rates' field to struct sta_info. It defaults to 0xffffffff. (Or perhaps call it 'disabled_rates' and make it default to 0.)
2. The SIOCSIWRATE handler: If the interface is not in a STA mode, return -EOPNOTSUPP. Otherwise, modify the allowed_rates field of the sta entry belonging to the current AP.
3. Bitmask sta->curr_rates with sta->allowed_rates (or ~sta->disabled_rates) in various places (ieee80211_ioctl_add_sta, ieee80211_rx_mgmt_assoc_resp, ieee80211_rx_bss_info; please check for other places).
In IBSS and AP mode setting this (per-STA, of course, which is not supported by WE, btw.) can be useful as well but it can be done later.
Jiri
Signature · 2 lines
Jiri Benc SUSE Labs
I am sorry for the late response. please read my comment bellow.
Quoted 12 lines · Jiri Benc
Quoted 3 lines · mabbas
I can not see how does it break per-STA TX rate limit, especially
PRISM2_HOSTAPD_SET_RATE_SETSalmost doing the same thing. I am not saying the patch is correct I just want to know how to fix it to get it in.As Jouni wrote, it's not useful to change the per-radio rate table. You want to limit the rates you are using to communicate with the current AP while not limiting other virtual interfaces. (Imagine you have the card that is capable to associate to two APs at the same time. You don't want to limit rates for both APs with SIOCSIWRATE.)
Quoted 5 lines
To do that I think the following is needed:
1. Add '
allowed_rates' field to structsta_info. It defaults to0xffffffff. (Or perhaps call it 'disabled_rates' and make it default to 0.)
Should I add the new field to sta_info or to ieee80211_sub_if_data. If we added to sta_info then it wont be persistent. We will loose SIOCSIWRATE restriction once we associate with new AP. Then in 3
we bitmask sta->curr_rates with ieee80211_sub_if_data::allowed_rates and this will solve the problem for IBSS as well.
Quoted 9 lines
2. The SIOCSIWRATE handler: If the interface is not in a STA mode, return
-EOPNOTSUPP. Otherwise, modify theallowed_ratesfield of the sta entry belonging to the current AP.3. Bitmask
sta->curr_rateswithsta->allowed_rates(or ~sta->disabled_rates) in various places (ieee80211_ioctl_add_sta,ieee80211_rx_mgmt_assoc_resp,ieee80211_rx_bss_info; please check for other places).
Quoted 6 lines
In IBSS and AP mode setting this (per-STA, of course, which is not supported by WE, btw.) can be useful as well but it can be done later.
Jiri
Thanks
Mohamed
Quoted 6 lines · mabbas
Should I add the new field to
sta_infoor toieee80211_sub_if_data. If we added tosta_infothen it wont be persistent.
We will loose SIOCSIWRATE restriction once we associate with new AP. Then in 3
we bitmasksta->curr_rateswithieee80211_sub_if_data::allowed_ratesand this will solve the problem for IBSS as well.
Fine with me. Though I'm not sure that Jouni will agree :-)
On the other hand, as WE are considered obsolete now, it's more important to have a nice design than to support all WE calls in an intuitive way. But we will have to convert d80211 to cfg80211 anyway, so it will depend on the way rate limiting is implemented in cfg80211 in the end.
Jiri
Signature · 2 lines
Jiri Benc SUSE Labs
Quoted 3 lines · Jiri Benc
But we will have to convert d80211 to cfg80211 anyway,
so it will depend on the way rate limiting is implemented in cfg80211 in the end.
I'd think it should be more the other way round with d80211/cfg80211 doing whatever makes most sense...
johannes
Quoted 7 lines · Johannes Berg
Quoted 3 lines · Jiri Benc
But we will have to convert d80211 to cfg80211 anyway,
so it will depend on the way rate limiting is implemented in cfg80211 in the end.I'd think it should be more the other way round with d80211/cfg80211 doing whatever makes most sense...
Current WE implementation of rate limiting (SIOCSIWRATE) doesn't make much sense with d80211. Hopefully we'll invent a better solution for cfg80211. Then we will probably need to put some constraints on SIOCSIWRATE emulation (like "rate limiting is lost when you are disassociated") - hence the comment.
Jiri
Signature · 2 lines
Jiri Benc SUSE Labs
Quoted 2 lines · Jiri Benc
Current WE implementation of rate limiting (SIOCSIWRATE) doesn't make much sense with d80211.
Right.
Quoted 2 lines
Hopefully we'll invent a better solution for
cfg80211.
We could do it right here and now then. I haven't understood the matter though, may I delegate it to you (plural "you" intended here)
Quoted 3 lines
Then we will probably need to put some constraints on SIOCSIWRATE emulation (like "rate limiting is lost when you are disassociated") - hence the comment.
That's ok.
johannes