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.
> --- 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.
> --- 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
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
