Implementation of the IEEE80211_RADIOTAP_RATE option

PATCH NAK Rafael Laufer 4 participants 15 messages Aug 21, 2009 – Aug 23, 2009 mac80211

Share & tools

Rafael Laufer

author

2009-08-21 00:40 UTC #0 lore raw

PATCH 1/1 Implementation of the IEEE80211_RADIOTAP_RATE option

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 <[email protected]>

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

net/mac80211/tx.c +17 -1

index 10a1099..41d636b 100644
@@ -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

Signature · 1 line

1.6.0.4

Johannes Berg

maintainer NAK

2009-08-21 08:18 UTC #1 lore raw

Quoted 19 lines · 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 <[email protected]>
---
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(-)
net/mac80211/tx.c +0 -0
index 10a1099..41d636b 100644

net/mac80211/tx.c ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx) · line 555

Quoted 8 lines

net/mac80211/tx.c +4 -1

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

Gábor Stefanik

2009-08-21 13:30 UTC #2 lore raw

Quoted 32 lines · Johannes Berg

Quoted 19 lines · 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 <[email protected]>
---
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(-)
net/mac80211/tx.c +0 -0
index 10a1099..41d636b 100644
net/mac80211/tx.c ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx) · line 555
Quoted 8 lines
net/mac80211/tx.c +4 -1
@@ -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.

Quoted 2 lines

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.

Signature · 1 line

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

Rafael Laufer

author

2009-08-21 18:03 UTC #3 lore raw

Quoted 4 lines · Gábor Stefanik

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 <[email protected]>

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

include/net/mac80211.h +4 -0

index aac84d7..819b01e 100644
@@ -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),
 };
 
 /**

net/mac80211/tx.c +21 -1

index 10a1099..f675844 100644
@@ -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

Signature · 1 line

1.6.0.4

Gábor Stefanik

2009-08-21 18:52 UTC #4 lore raw

2009/8/21 Rafael Laufer <[email protected]>:

Quoted 10 lines

Quoted 4 lines · Gábor Stefanik
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.)

Quoted 86 lines

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 <[email protected]>

include/net/mac80211.h | 4 ++++ net/mac80211/tx.c | 22 +++++++++++++++++++++-

 2 files changed, 25 insertions(+), 1 deletions(-)

include/net/mac80211.h +4 -0

index aac84d7..819b01e 100644
@@ -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),
  };
 
  /**

net/mac80211/tx.c +21 -2

index 10a1099..f675844 100644
@@ -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

Signature · 1 line

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

Rafael Laufer

author

2009-08-21 19:06 UTC #5 lore raw

Quoted 14 lines · Gábor Stefanik

2009/8/21 Rafael Laufer <[email protected]>:
Quoted 11 lines
Quoted 4 lines · Gábor Stefanik
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.

Quoted 5 lines

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

Gábor Stefanik

2009-08-21 19:58 UTC #6 lore raw

2009/8/21 Rafael Laufer <[email protected]>:

Quoted 21 lines

Quoted 14 lines · Gábor Stefanik
2009/8/21 Rafael Laufer <[email protected]>:
Quoted 11 lines
Quoted 4 lines · Gábor Stefanik
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.
Quoted 5 lines
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.)

Quoted 9 lines

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.

Signature · 1 line

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

Rafael Laufer

author

2009-08-21 20:21 UTC #7 lore raw

Quoted 40 lines · Gábor Stefanik

2009/8/21 Rafael Laufer <[email protected]>:
Quoted 25 lines
Quoted 17 lines · Gábor Stefanik
2009/8/21 Rafael Laufer <[email protected]>:
Quoted 8 lines
Quoted 5 lines · Gábor Stefanik
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.
Quoted 6 lines
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.
Quoted 6 lines
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.)
Quoted 10 lines
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.

Quoted 9 lines

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

Rafael Laufer

author

2009-08-21 20:24 UTC #8 lore raw

Quoted 70 lines · Rafael Laufer

Quoted 47 lines · Gábor Stefanik
2009/8/21 Rafael Laufer <[email protected]>:
Quoted 31 lines
Quoted 21 lines · Gábor Stefanik
2009/8/21 Rafael Laufer <[email protected]>:
Quoted 10 lines
Quoted 6 lines · Gábor Stefanik
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.
Quoted 7 lines
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.
Quoted 7 lines
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.)
Quoted 11 lines
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.
Quoted 10 lines
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?

Quoted 9 lines

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

Johannes Berg

maintainer

2009-08-22 07:50 UTC #9 lore raw

In reply to Rafael Laufer

Quoted 4 lines · Rafael Laufer

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.

Quoted 3 lines

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

Quoted 4 lines

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

Rafael Laufer

author

2009-08-22 22:18 UTC #10 lore raw

Quoted 8 lines · Johannes Berg

Quoted 5 lines · Rafael Laufer
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?

Quoted 9 lines

I can't tell if you're kidding or not. This also doesn't get a single rate, but the entire rate control setup.
Quoted 4 lines
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?

Quoted 8 lines

Quoted 6 lines
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

Johannes Berg

maintainer

2009-08-22 07:48 UTC #11 lore raw

In reply to Rafael Laufer

Quoted 3 lines · Rafael Laufer

+ * @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.

net/mac80211/tx.c ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx) · line 555

Quoted 3 lines

net/mac80211/tx.c +3 -0

+	/* 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

net/mac80211/tx.c static bool __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx, · line 982

Quoted 3 lines

net/mac80211/tx.c +3 -0

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

coding style

net/mac80211/tx.c static bool __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx, · line 992

Quoted 10 lines

net/mac80211/tx.c +10 -0

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

net/mac80211/tx.c static bool __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx, · line 997

Quoted 3 lines

net/mac80211/tx.c +0 -0

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

And you even quote the instructions.

johannes

Rafael Laufer

author

2009-08-22 22:18 UTC #12 lore raw

Quoted 10 lines · Johannes Berg

Quoted 4 lines · Rafael Laufer
+ * @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?

Quoted 9 lines

net/mac80211/tx.c ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx) · line 555
Quoted 5 lines
net/mac80211/tx.c +3 -0
+	/* 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

Quoted 7 lines

net/mac80211/tx.c static bool __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx, · line 982
Quoted 4 lines
net/mac80211/tx.c +3 -0
+		/* 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

Quoted 15 lines

net/mac80211/tx.c static bool __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx, · line 992
Quoted 12 lines
net/mac80211/tx.c +10 -0
+		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?

Quoted 8 lines

net/mac80211/tx.c static bool __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx, · line 997
Quoted 5 lines
net/mac80211/tx.c +0 -0
 		/*
 		 * Please update the file
 		 * Documentation/networking/mac80211-injection.txt
And you even quote the instructions.

come on...

Rafael

Kalle Valo

2009-08-23 08:06 UTC #13 lore raw

Quoted 8 lines · Rafael Laufer

Quoted 6 lines
Quoted 3 lines
(quoted diff) +1 -0
+		/* 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.

Quoted 2 lines

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 */

Signature · 1 line

Kalle Valo

Johannes Berg

maintainer

2009-08-23 09:11 UTC #14 lore raw

In reply to Rafael Laufer

Quoted 9 lines · Rafael Laufer

Quoted 7 lines
Quoted 4 lines
+ * @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.

Quoted 10 lines

Quoted 7 lines
net/mac80211/tx.c static bool __ieee80211_parse_tx_radiotap(struct ieee80211_tx_data *tx, · line 982
Quoted 4 lines
net/mac80211/tx.c +3 -0
+		/* 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.

Quoted 4 lines

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