[PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Rafael Laufer]

This patch implements the IEEE80211_RADIOTAP_RATE 
option when parsing radiotap headers to allow rate 
selection on a per-packet basis.


Signed-off-by: Rafael Laufer <rlaufer@cs.ucla.edu>
---
 Implementation of the IEEE80211_RADIOTAP_RATE 
 option when parsing radiotap headers to allow
 rate selection on a per-packet basis.

 net/mac80211/tx.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 10a1099..41d636b 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -549,7 +549,10 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
 	 * If we're associated with the sta at this point we know we can at
 	 * least send the frame at the lowest bit rate.
 	 */
-	rate_control_get_rate(tx->sdata, tx->sta, &txrc);
+
+	/* in monitor mode, we already have the rate from the radiotap header */
+	if (likely(!(info->flags & IEEE80211_TX_CTL_INJECTED)))
+		rate_control_get_rate(tx->sdata, tx->sta, &txrc);
 
 	if (unlikely(info->control.rates[0].idx < 0))
 		return TX_DROP;
@@ -972,6 +975,19 @@ static bool __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx,
 				tx->flags |= IEEE80211_TX_FRAGMENTED;
 			break;
 
+		/* Get the rate parameter from the radiotap header, 
+		 * allowing rate selection on a per-packet basis 
+		 */
+		case IEEE80211_RADIOTAP_RATE:
+			bitrate = (*iterator.this_arg) * 5;
+			for (i = 0; i < sband->n_bitrates; i++) {
+				if (sband->bitrates[i].bitrate == bitrate)
+					break;
+			}
+			if (i != sband->n_bitrates)
+				info->control.rates[0].idx = i;
+			break;
+
 		/*
 		 * Please update the file
 		 * Documentation/networking/mac80211-injection.txt
-- 
1.6.0.4

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On Thu, 2009-08-20 at 17:40 -0700, Rafael Laufer wrote:
> This patch implements the IEEE80211_RADIOTAP_RATE 
> option when parsing radiotap headers to allow rate 
> selection on a per-packet basis.
> 
> 
> Signed-off-by: Rafael Laufer <rlaufer@cs.ucla.edu>
> ---
>  Implementation of the IEEE80211_RADIOTAP_RATE 
>  option when parsing radiotap headers to allow
>  rate selection on a per-packet basis.
> 
>  net/mac80211/tx.c |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 10a1099..41d636b 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -549,7 +549,10 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
>  	 * If we're associated with the sta at this point we know we can at
>  	 * least send the frame at the lowest bit rate.
>  	 */
> -	rate_control_get_rate(tx->sdata, tx->sta, &txrc);
> +
> +	/* in monitor mode, we already have the rate from the radiotap header */
> +	if (likely(!(info->flags & IEEE80211_TX_CTL_INJECTED)))
> +		rate_control_get_rate(tx->sdata, tx->sta, &txrc);

NAK, the rate is optional in the radiotap header, and if not given then
regular rate control must be used.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Gábor Stefanik]

On Fri, Aug 21, 2009 at 10:18 AM, Johannes
Berg<johannes@sipsolutions.net> wrote:
> On Thu, 2009-08-20 at 17:40 -0700, Rafael Laufer wrote:
>> This patch implements the IEEE80211_RADIOTAP_RATE
>> option when parsing radiotap headers to allow rate
>> selection on a per-packet basis.
>>
>>
>> Signed-off-by: Rafael Laufer <rlaufer@cs.ucla.edu>
>> ---
>>  Implementation of the IEEE80211_RADIOTAP_RATE
>>  option when parsing radiotap headers to allow
>>  rate selection on a per-packet basis.
>>
>>  net/mac80211/tx.c |   18 +++++++++++++++++-
>>  1 files changed, 17 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index 10a1099..41d636b 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -549,7 +549,10 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
>>        * If we're associated with the sta at this point we know we can at
>>        * least send the frame at the lowest bit rate.
>>        */
>> -     rate_control_get_rate(tx->sdata, tx->sta, &txrc);
>> +
>> +     /* in monitor mode, we already have the rate from the radiotap header */
>> +     if (likely(!(info->flags & IEEE80211_TX_CTL_INJECTED)))
>> +             rate_control_get_rate(tx->sdata, tx->sta, &txrc);
>
> NAK, the rate is optional in the radiotap header, and if not given then
> regular rate control must be used.
>
> johannes
>

Also, I think something more powerful than the current "rate" field
would be needed, with support for MCS indexes, channel width, retry
count, etc. - one that can configure all values rate_control_get_rate
would perform. I'm planning a Radiotap meeting on Freenode with the
radiotap.h maintainers in various OSes participating, so the field 14+
mess can be cleaned up once for all - that's when I'll probably
propose this field.

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.

-- 
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Rafael Laufer]

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.

Note that in the future there must be other flags like this to 
indicate if other parameters, such as power, was also set in
the radiotap header.


Signed-off-by: Rafael Laufer <rlaufer@cs.ucla.edu>
---
 Implementation of the IEEE80211_RADIOTAP_RATE
 option when parsing radiotap headers to allow
 rate selection on a per-packet basis. A new
 flag IEEE80211_TX_CTL_RATE_RADIOTAP is also
 included to indicate that the rate was set in
 the radiotap header and therefore the rate
 control algorithm should not change it.

 include/net/mac80211.h |    4 ++++
 net/mac80211/tx.c      |   22 +++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index aac84d7..819b01e 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -272,6 +272,9 @@ struct ieee80211_bss_conf {
  *	transmit function after the current frame, this can be used
  *	by drivers to kick the DMA queue only if unset or when the
  *	queue gets full.
+ * @IEEE80211_TX_CTL_RATE_RADIOTAP: completely internal to mac80211,
+ *	used to indicate that the rate was defined in the received radiotap
+ *	header and therefore the rate control algorithm should not change it.
  */
 enum mac80211_tx_control_flags {
 	IEEE80211_TX_CTL_REQ_TX_STATUS		= BIT(0),
@@ -293,6 +296,7 @@ enum mac80211_tx_control_flags {
 	IEEE80211_TX_INTFL_DONT_ENCRYPT		= BIT(16),
 	IEEE80211_TX_CTL_PSPOLL_RESPONSE	= BIT(17),
 	IEEE80211_TX_CTL_MORE_FRAMES		= BIT(18),
+	IEEE80211_TX_CTL_RATE_RADIOTAP 		= BIT(19),
 };
 
 /**
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 10a1099..f675844 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -549,7 +549,12 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
 	 * If we're associated with the sta at this point we know we can at
 	 * least send the frame at the lowest bit rate.
 	 */
-	rate_control_get_rate(tx->sdata, tx->sta, &txrc);
+
+	/* In monitor mode, if the IEEE80211_RADIOTAP_RATE option is set in 
+	 * the received radiotap header, do not call the rate control algorithm.
+	 */
+	if (likely(!(info->flags & IEEE80211_TX_CTL_RATE_RADIOTAP)))
+		rate_control_get_rate(tx->sdata, tx->sta, &txrc);
 
 	if (unlikely(info->control.rates[0].idx < 0))
 		return TX_DROP;
@@ -972,6 +977,21 @@ static bool __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx,
 				tx->flags |= IEEE80211_TX_FRAGMENTED;
 			break;
 
+		/* Get the rate parameter from the radiotap header, 
+		 * allowing rate selection on a per-packet basis 
+		 */
+		case IEEE80211_RADIOTAP_RATE:
+			bitrate = (*iterator.this_arg) * 5;
+			for (i = 0; i < sband->n_bitrates; i++) {
+				if (sband->bitrates[i].bitrate == bitrate)
+					break;
+			}
+			if (i != sband->n_bitrates) {
+				info->control.rates[0].idx = i;
+				info->flags |= IEEE80211_TX_CTL_RATE_RADIOTAP;
+			}
+			break;
+
 		/*
 		 * Please update the file
 		 * Documentation/networking/mac80211-injection.txt
-- 
1.6.0.4

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Gábor Stefanik]

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.)

>
> Note that in the future there must be other flags like this to
> indicate if other parameters, such as power, was also set in
> the radiotap header.
>
>
> Signed-off-by: Rafael Laufer <rlaufer@cs.ucla.edu>
> ---
>  Implementation of the IEEE80211_RADIOTAP_RATE
>  option when parsing radiotap headers to allow
>  rate selection on a per-packet basis. A new
>  flag IEEE80211_TX_CTL_RATE_RADIOTAP is also
>  included to indicate that the rate was set in
>  the radiotap header and therefore the rate
>  control algorithm should not change it.
>
>  include/net/mac80211.h |    4 ++++
>  net/mac80211/tx.c      |   22 +++++++++++++++++++++-
>  2 files changed, 25 insertions(+), 1 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index aac84d7..819b01e 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -272,6 +272,9 @@ struct ieee80211_bss_conf {
>  *     transmit function after the current frame, this can be used
>  *     by drivers to kick the DMA queue only if unset or when the
>  *     queue gets full.
> + * @IEEE80211_TX_CTL_RATE_RADIOTAP: completely internal to mac80211,
> + *     used to indicate that the rate was defined in the received radiotap
> + *     header and therefore the rate control algorithm should not change it.
>  */
>  enum mac80211_tx_control_flags {
>        IEEE80211_TX_CTL_REQ_TX_STATUS          = BIT(0),
> @@ -293,6 +296,7 @@ enum mac80211_tx_control_flags {
>        IEEE80211_TX_INTFL_DONT_ENCRYPT         = BIT(16),
>        IEEE80211_TX_CTL_PSPOLL_RESPONSE        = BIT(17),
>        IEEE80211_TX_CTL_MORE_FRAMES            = BIT(18),
> +       IEEE80211_TX_CTL_RATE_RADIOTAP          = BIT(19),
>  };
>
>  /**
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 10a1099..f675844 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -549,7 +549,12 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
>         * If we're associated with the sta at this point we know we can at
>         * least send the frame at the lowest bit rate.
>         */
> -       rate_control_get_rate(tx->sdata, tx->sta, &txrc);
> +
> +       /* In monitor mode, if the IEEE80211_RADIOTAP_RATE option is set in
> +        * the received radiotap header, do not call the rate control algorithm.
> +        */
> +       if (likely(!(info->flags & IEEE80211_TX_CTL_RATE_RADIOTAP)))
> +               rate_control_get_rate(tx->sdata, tx->sta, &txrc);
>
>        if (unlikely(info->control.rates[0].idx < 0))
>                return TX_DROP;
> @@ -972,6 +977,21 @@ static bool __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx,
>                                tx->flags |= IEEE80211_TX_FRAGMENTED;
>                        break;
>
> +               /* Get the rate parameter from the radiotap header,
> +                * allowing rate selection on a per-packet basis
> +                */
> +               case IEEE80211_RADIOTAP_RATE:
> +                       bitrate = (*iterator.this_arg) * 5;
> +                       for (i = 0; i < sband->n_bitrates; i++) {
> +                               if (sband->bitrates[i].bitrate == bitrate)
> +                                       break;
> +                       }
> +                       if (i != sband->n_bitrates) {
> +                               info->control.rates[0].idx = i;
> +                               info->flags |= IEEE80211_TX_CTL_RATE_RADIOTAP;
> +                       }
> +                       break;
> +
>                /*
>                 * Please update the file
>                 * Documentation/networking/mac80211-injection.txt
> --
> 1.6.0.4
>
>



-- 
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Rafael Laufer]

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

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Gábor Stefanik]

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.

-- 
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Rafael Laufer]

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

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Rafael Laufer]

Rafael Laufer wrote:
> 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?
>   

ops, I meant

ref->ops->get_rate(ref->priv, ista, priv_sta, txrc);

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1217 bytes --]

On Fri, 2009-08-21 at 11:03 -0700, Rafael Laufer wrote:

> + * @IEEE80211_TX_CTL_RATE_RADIOTAP: completely internal to mac80211,
> + *	used to indicate that the rate was defined in the received radiotap
> + *	header and therefore the rate control algorithm should not change it.

This should be an internal flag, the driver doesn't care.

> +	/* In monitor mode, if the IEEE80211_RADIOTAP_RATE option is set in 
> +	 * the received radiotap header, do not call the rate control algorithm.
> +	 */

coding style

> +		/* Get the rate parameter from the radiotap header, 
> +		 * allowing rate selection on a per-packet basis 
> +		 */

coding style

> +		case IEEE80211_RADIOTAP_RATE:
> +			bitrate = (*iterator.this_arg) * 5;
> +			for (i = 0; i < sband->n_bitrates; i++) {
> +				if (sband->bitrates[i].bitrate == bitrate)
> +					break;
> +			}
> +			if (i != sband->n_bitrates) {
> +				info->control.rates[0].idx = i;
> +				info->flags |= IEEE80211_TX_CTL_RATE_RADIOTAP;
> +			}

You never set the counter, or any other fields.

>  		/*
>  		 * Please update the file
>  		 * Documentation/networking/mac80211-injection.txt

And you even quote the instructions.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 994 bytes --]

On Fri, 2009-08-21 at 13:21 -0700, Rafael Laufer wrote:

> 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?

I can't tell if you're kidding or not. This also doesn't get a single
rate, but the entire rate control setup.

> 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.

Right. And now look again what's in control.rates[].

> 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?

Ick, no.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Rafael Laufer]

Johannes Berg wrote:
> On Fri, 2009-08-21 at 11:03 -0700, Rafael Laufer wrote:
>
>   
>> + * @IEEE80211_TX_CTL_RATE_RADIOTAP: completely internal to mac80211,
>> + *	used to indicate that the rate was defined in the received radiotap
>> + *	header and therefore the rate control algorithm should not change it.
>>     
>
> This should be an internal flag, the driver doesn't care.
>   

right, and where are those set?
>   
>> +	/* In monitor mode, if the IEEE80211_RADIOTAP_RATE option is set in 
>> +	 * the received radiotap header, do not call the rate control algorithm.
>> +	 */
>>     
>
> coding style
>
>   
>> +		/* Get the rate parameter from the radiotap header, 
>> +		 * allowing rate selection on a per-packet basis 
>> +		 */
>>     
>
> coding style
>   

I am a newbie, I am gonna look into the coding style, but I assume you 
are talking about the missing blank line in the beginning

>   
>> +		case IEEE80211_RADIOTAP_RATE:
>> +			bitrate = (*iterator.this_arg) * 5;
>> +			for (i = 0; i < sband->n_bitrates; i++) {
>> +				if (sband->bitrates[i].bitrate == bitrate)
>> +					break;
>> +			}
>> +			if (i != sband->n_bitrates) {
>> +				info->control.rates[0].idx = i;
>> +				info->flags |= IEEE80211_TX_CTL_RATE_RADIOTAP;
>> +			}
>>     
>
> You never set the counter, or any other fields.
>   

This is a good point and now I see that Gábor pointed this out as well. 
There are other fields in the radiotap header that define the RTS and 
DATA retries. However, if those fields are not set, there must be a 
default value in this case. Are there any?

>   
>>  		/*
>>  		 * Please update the file
>>  		 * Documentation/networking/mac80211-injection.txt
>>     
>
> And you even quote the instructions.
>   

come on...

Rafael

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Rafael Laufer]

Johannes Berg wrote:
> On Fri, 2009-08-21 at 13:21 -0700, Rafael Laufer wrote:
>
>   
>> 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?
>>     
> I can't tell if you're kidding or not. This also doesn't get a single
> rate, but the entire rate control setup.
>   
>> 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.
>>     
> Right. And now look again what's in control.rates[].
>   

ok, I get it now. The flags and count also need to be set. Anything else?

>   
>> 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?
>>     
> Ick, no.
>   
though so, it's so ugly

Rafael

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Kalle Valo]

Rafael Laufer <rlaufer@CS.UCLA.EDU> writes:

>>> +		/* Get the rate parameter from the radiotap header, +
>>> * allowing rate selection on a per-packet basis +		 */
>>>     
>>
>> coding style
>>   
>
> I am a newbie, I am gonna look into the coding style, 

In case you didn't know, Documentation/CodingStyle is the file you want
to need.

> but I assume you are talking about the missing blank line in the
> beginning

I think Johannes' means the comment style:

/*
 * The prefer multiline comments to look like this. blah blah blah blah
 * blah blah blah blah blah blah blah blah blah blah blah blah
 */

-- 
Kalle Valo

Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]

On Sat, 2009-08-22 at 15:03 -0700, Rafael Laufer wrote:

> >> + * @IEEE80211_TX_CTL_RATE_RADIOTAP: completely internal to mac80211,
> >> + *	used to indicate that the rate was defined in the received radiotap
> >> + *	header and therefore the rate control algorithm should not change it.
> >>     
> >
> > This should be an internal flag, the driver doesn't care.
> >   
> 
> right, and where are those set?

It's just a naming thing really. Look at the other flags.

> >> +		/* Get the rate parameter from the radiotap header, 
> >> +		 * allowing rate selection on a per-packet basis 
> >> +		 */
> >>     
> >
> > coding style
> >   
> 
> I am a newbie, I am gonna look into the coding style, but I assume you 
> are talking about the missing blank line in the beginning

What Kalle said. Sorry to be a bit terse at times.

> This is a good point and now I see that Gábor pointed this out as well. 
> There are other fields in the radiotap header that define the RTS and 
> DATA retries. However, if those fields are not set, there must be a 
> default value in this case. Are there any?

Well there's the short and long retry counts, that could be used I
guess. It would make some sense to do that. And the RTS/CTS-to-self
determination should probably be made by the network in absence of
explicit configuration, but the tx_h_rate_ctrl function should do that
already I think, even if you skip the rate control algorithm. Same with
short preamble or not. If you want to support only the rate, I suspect
that leaving the flags and setting just the rate index/count will be
sufficient. Unfortunately, you'll have to take a closer look at the code
to determine it.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]