Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Rafael Laufer <rlaufer@cs.ucla.edu>]

Gábor Stefanik wrote:
> 2009/8/21 Rafael Laufer <rlaufer@cs.ucla.edu>:
>   
>> Gábor Stefanik wrote:
>>     
>>> 2009/8/21 Rafael Laufer <rlaufer@cs.ucla.edu>:
>>>
>>>       
>>>> Gábor Stefanik wrote:
>>>>
>>>>         
>>>>> Maybe a new IEEE80211_TX_CTL_ or IEEE80211_TX_RC_ flag will also be
>>>>> needed, so Radiotap can indicate whether rate_control_get_rate needs
>>>>> to be called.
>>>>>
>>>>>           
>>>> ok, I am resending the patch. I included a new flag called
>>>> IEEE80211_TX_CTL_RATE_RADIOTAP to indicate if the rate has
>>>> been set in the radiotap header. If not, then the rate control
>>>> algorithm is called.
>>>>
>>>>         
>>> Isn't it easier to check whether we already have a rate configured?
>>> (info->control.rates[0].idx is set to an invalid value before the
>>> rate_control_get_rate call AFAIK, unless you set it in the radiotap
>>> decoding function before.)
>>>
>>>       
>> I guess it is also possible, but in that case you rely on the assumption
>> that the rate is invalid before rate_control_get_rate(). If in the
>> future this assumption does not hold, the code will break. If, however,
>> this is always gonna be true, I can change the code to use your
>> suggestion. Personally, I prefer to use another flag so that future
>> changes do not affect this code, but let me know what is best.
>>
>> Rafael
>>
>>     
>
> Actually, that's a good point.
>
> One thing to watch out for is that the actual rate index is not the
> only thing the rate controller sets - it is also responsible for
> things like retry count & RTS/CTS usage. Those are controlled by other
> radiotap fields. So, if any of these values is unset in radiotap, you
> will need to call rate control for them, or auto-generate them in
> other ways. Otherwise you may end up with e.g. an incorrect retry
> count.
>
>   
This is a good point. Where is this done? Is it in the driver-specific
function?

ref->ops <http://localhost/lxr/http/ident?v=2.6.31-rc3;i=ops>->get_rate(ref->priv <http://localhost/lxr/http/ident?v=2.6.31-rc3;i=priv>, ista, priv_sta, txrc);


It is strange that a function called "get_rate" would also change other
fields which are at first sight do not look related to rate. Why not
call other functions for that? What is the reasoning behind this?
Different rates have different retry counts or RTS/CTS usage?

As far as I could tell from a quick look in the code,
rate_control_get_rate only sets the fields of info->control.rates,
except for this driver-specific function.

If this function really does other stuff, then a simple solution is to
check if the IEEE80211_TX_CTL_RATE_RADIOTAP flag is set and, in that
case, store the value of info->control.rates[0].idx before calling
rate_control_get_rate, and restoring it afterwards. Make sense?

Rafael