Re: [PATCH 2/7] d80211: add support for SIOCSIWRATE and SIOCGIWRATE
From: Jouni Malinen <hidden>
Date: 2006-08-30 17:19:26
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.
quoted hunk ↗ jump to hunk
--- 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.
quoted hunk ↗ jump to hunk
--- 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.
+ fixed = wrqu->bitrate.fixed; + supp = (int *) kmalloc((local->num_curr_rates + 1) * + sizeof(int), GFP_KERNEL); + if (!supp) + return 0;
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.
+ /* 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 (old_supp) + kfree(old_supp);
No need for 'if (old_supp)' before calling kfree(old_supp). -- Jouni Malinen PGP id EFC895FA