* [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option @ 2009-08-21 0:40 Rafael Laufer 2009-08-21 8:18 ` Johannes Berg 0 siblings, 1 reply; 15+ messages in thread From: Rafael Laufer @ 2009-08-21 0:40 UTC (permalink / raw) To: linux-wireless 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-21 0:40 [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option Rafael Laufer @ 2009-08-21 8:18 ` Johannes Berg 2009-08-21 13:30 ` Gábor Stefanik 0 siblings, 1 reply; 15+ messages in thread From: Johannes Berg @ 2009-08-21 8:18 UTC (permalink / raw) To: Rafael Laufer; +Cc: linux-wireless [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-21 8:18 ` Johannes Berg @ 2009-08-21 13:30 ` Gábor Stefanik 2009-08-21 18:03 ` Rafael Laufer 0 siblings, 1 reply; 15+ messages in thread From: Gábor Stefanik @ 2009-08-21 13:30 UTC (permalink / raw) To: Johannes Berg; +Cc: Rafael Laufer, linux-wireless 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. :-) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-21 13:30 ` Gábor Stefanik @ 2009-08-21 18:03 ` Rafael Laufer 2009-08-21 18:52 ` Gábor Stefanik 2009-08-22 7:48 ` Johannes Berg 0 siblings, 2 replies; 15+ messages in thread From: Rafael Laufer @ 2009-08-21 18:03 UTC (permalink / raw) To: Gábor Stefanik; +Cc: Johannes Berg, linux-wireless 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-21 18:03 ` Rafael Laufer @ 2009-08-21 18:52 ` Gábor Stefanik 2009-08-21 19:06 ` Rafael Laufer 2009-08-22 7:48 ` Johannes Berg 1 sibling, 1 reply; 15+ messages in thread From: Gábor Stefanik @ 2009-08-21 18:52 UTC (permalink / raw) To: Rafael Laufer; +Cc: Johannes Berg, linux-wireless 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. :-) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-21 18:52 ` Gábor Stefanik @ 2009-08-21 19:06 ` Rafael Laufer 2009-08-21 19:57 ` Gábor Stefanik 0 siblings, 1 reply; 15+ messages in thread From: Rafael Laufer @ 2009-08-21 19:06 UTC (permalink / raw) To: Gábor Stefanik; +Cc: Johannes Berg, linux-wireless 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-21 19:06 ` Rafael Laufer @ 2009-08-21 19:57 ` Gábor Stefanik 2009-08-21 20:21 ` Rafael Laufer 0 siblings, 1 reply; 15+ messages in thread From: Gábor Stefanik @ 2009-08-21 19:57 UTC (permalink / raw) To: Rafael Laufer; +Cc: Johannes Berg, linux-wireless 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. :-) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-21 19:57 ` Gábor Stefanik @ 2009-08-21 20:21 ` Rafael Laufer 2009-08-21 20:24 ` Rafael Laufer 2009-08-22 7:50 ` Johannes Berg 0 siblings, 2 replies; 15+ messages in thread From: Rafael Laufer @ 2009-08-21 20:21 UTC (permalink / raw) To: Gábor Stefanik; +Cc: Johannes Berg, linux-wireless 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-21 20:21 ` Rafael Laufer @ 2009-08-21 20:24 ` Rafael Laufer 2009-08-22 7:50 ` Johannes Berg 1 sibling, 0 replies; 15+ messages in thread From: Rafael Laufer @ 2009-08-21 20:24 UTC (permalink / raw) To: Gábor Stefanik; +Cc: Johannes Berg, linux-wireless 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); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-21 20:21 ` Rafael Laufer 2009-08-21 20:24 ` Rafael Laufer @ 2009-08-22 7:50 ` Johannes Berg 2009-08-22 22:05 ` Rafael Laufer 1 sibling, 1 reply; 15+ messages in thread From: Johannes Berg @ 2009-08-22 7:50 UTC (permalink / raw) To: Rafael Laufer; +Cc: Gábor Stefanik, linux-wireless [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-22 7:50 ` Johannes Berg @ 2009-08-22 22:05 ` Rafael Laufer 0 siblings, 0 replies; 15+ messages in thread From: Rafael Laufer @ 2009-08-22 22:05 UTC (permalink / raw) To: Johannes Berg; +Cc: Gábor Stefanik, linux-wireless 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-21 18:03 ` Rafael Laufer 2009-08-21 18:52 ` Gábor Stefanik @ 2009-08-22 7:48 ` Johannes Berg 2009-08-22 22:03 ` Rafael Laufer 1 sibling, 1 reply; 15+ messages in thread From: Johannes Berg @ 2009-08-22 7:48 UTC (permalink / raw) To: Rafael Laufer; +Cc: Gábor Stefanik, linux-wireless [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-22 7:48 ` Johannes Berg @ 2009-08-22 22:03 ` Rafael Laufer 2009-08-23 8:06 ` Kalle Valo 2009-08-23 9:11 ` Johannes Berg 0 siblings, 2 replies; 15+ messages in thread From: Rafael Laufer @ 2009-08-22 22:03 UTC (permalink / raw) To: Johannes Berg; +Cc: Gábor Stefanik, linux-wireless 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-22 22:03 ` Rafael Laufer @ 2009-08-23 8:06 ` Kalle Valo 2009-08-23 9:11 ` Johannes Berg 1 sibling, 0 replies; 15+ messages in thread From: Kalle Valo @ 2009-08-23 8:06 UTC (permalink / raw) To: Rafael Laufer; +Cc: Johannes Berg, Gábor Stefanik, linux-wireless 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option 2009-08-22 22:03 ` Rafael Laufer 2009-08-23 8:06 ` Kalle Valo @ 2009-08-23 9:11 ` Johannes Berg 1 sibling, 0 replies; 15+ messages in thread From: Johannes Berg @ 2009-08-23 9:11 UTC (permalink / raw) To: Rafael Laufer; +Cc: Gábor Stefanik, linux-wireless [-- 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 --] ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2009-08-23 9:11 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-08-21 0:40 [PATCH] Implementation of the IEEE80211_RADIOTAP_RATE option Rafael Laufer 2009-08-21 8:18 ` Johannes Berg 2009-08-21 13:30 ` Gábor Stefanik 2009-08-21 18:03 ` Rafael Laufer 2009-08-21 18:52 ` Gábor Stefanik 2009-08-21 19:06 ` Rafael Laufer 2009-08-21 19:57 ` Gábor Stefanik 2009-08-21 20:21 ` Rafael Laufer 2009-08-21 20:24 ` Rafael Laufer 2009-08-22 7:50 ` Johannes Berg 2009-08-22 22:05 ` Rafael Laufer 2009-08-22 7:48 ` Johannes Berg 2009-08-22 22:03 ` Rafael Laufer 2009-08-23 8:06 ` Kalle Valo 2009-08-23 9:11 ` Johannes Berg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).