[RFC] mac80211: add rate to ieee80211_tx_status

[RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

This patch adds rate to ieee80211_tx_status.
Downscaling in iwl4965 is done in FW. Rate scaling algorithm
needs to know at which rate the packet was actually transmitted
Patch 'cfg80211 API for channels/bitrates' removed the placeholder
for this information that was present in ieee80211_tx_control.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---

This fix or something similar is required for bringing iwlwifi back to life.
Please comments, suggestions 
Thanks

 include/net/mac80211.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 460da54..a700ea6 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -342,6 +342,7 @@ enum ieee80211_tx_status_flags {
  *
  * @control: a copy of the &struct ieee80211_tx_control passed to the driver
  *	in the tx() callback.
+ * @rate: rate at which the packet was actually transmitted
  * @flags: transmit status flags, defined above
  * @retry_count: number of retries
  * @excessive_retries: set to 1 if the frame was retried many times
@@ -356,6 +357,7 @@ enum ieee80211_tx_status_flags {
  */
 struct ieee80211_tx_status {
 	struct ieee80211_tx_control control;
+	struct ieee80211_rate rate;
 	u8 flags;
 	u8 retry_count;
 	bool excessive_retries;
-- 
1.5.2.2

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Johannes Berg]

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


> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 460da54..a700ea6 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -342,6 +342,7 @@ enum ieee80211_tx_status_flags {
>   *
>   * @control: a copy of the &struct ieee80211_tx_control passed to the driver
>   *	in the tx() callback.
> + * @rate: rate at which the packet was actually transmitted
>   * @flags: transmit status flags, defined above
>   * @retry_count: number of retries
>   * @excessive_retries: set to 1 if the frame was retried many times
> @@ -356,6 +357,7 @@ enum ieee80211_tx_status_flags {
>   */
>  struct ieee80211_tx_status {
>  	struct ieee80211_tx_control control;
> +	struct ieee80211_rate rate;

Shouldn't that rather be a rate pointer? Seems a bit odd to me to copy
that information. Felix, could this help you as well?

johannes

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

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 10:46 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> > diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> > index 460da54..a700ea6 100644
> > --- a/include/net/mac80211.h
> > +++ b/include/net/mac80211.h
> > @@ -342,6 +342,7 @@ enum ieee80211_tx_status_flags {
> >   *
> >   * @control: a copy of the &struct ieee80211_tx_control passed to the driver
> >   *   in the tx() callback.
> > + * @rate: rate at which the packet was actually transmitted
> >   * @flags: transmit status flags, defined above
> >   * @retry_count: number of retries
> >   * @excessive_retries: set to 1 if the frame was retried many times
> > @@ -356,6 +357,7 @@ enum ieee80211_tx_status_flags {
> >   */
> >  struct ieee80211_tx_status {
> >       struct ieee80211_tx_control control;
> > +     struct ieee80211_rate rate;
>
> Shouldn't that rather be a rate pointer? Seems a bit odd to me to copy
> that information. Felix, could this help you as well?

Where would be this pointer allocated? TX response is not the same
flow as TX where we keep it on the stack. It's will be very error
prone to be outside tx_status. From concept perspective you already
have all transimission information in that structure.
For iwlwifi I actually need only u32 value but I was looking for
something more general even it's a little pain.
I'm not sure but I guess we may as well separate ieee80211_tx_control
form ieee80211_tx_status as they carry different information

Thanks
Tomas

> johannes
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 11:56 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > Where would be this pointer allocated? TX response is not the same
> > flow as TX where we keep it on the stack. It's will be very error
> > prone to be outside tx_status. From concept perspective you already
> > have all transimission information in that structure.
> > For iwlwifi I actually need only u32 value but I was looking for
> > something more general even it's a little pain.
>
> You don't need to allocate it, it can just point to the rate array you
> have registered, no? Rates information is static after all. Or do it like
> rx status where we pass the index into the rate array.

I prefer not. This is not just plcp rate there is also MCS flags that
have to passed. With all combinations the array will be huge.

Tomas

> johannes
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 12:32 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> Tomas Winkler wrote:
>
> > I prefer not. This is not just plcp rate there is also MCS flags that
> > have to passed. With all combinations the array will be huge.
>
> So you'd actually abuse the struct bitrate contents if you add the
> bitrate? I prefer that wouldn't be done, it makes the code impossible to
> follow.
>
> I guess we need to rework the rate control algorithm API completely.
>

No I won't we add rate flags which are part of spec anyway  and then
plcp rate it self will be in bitrate.

num ieee80211_rate_flags {
	IEEE80211_RATE_SHORT_PREAMBLE	= 1<<0,
	IEEE80211_RATE_MANDATORY_A	= 1<<1,
	IEEE80211_RATE_MANDATORY_B	= 1<<2,
	IEEE80211_RATE_MANDATORY_G	= 1<<3,
	IEEE80211_RATE_ERP_G		= 1<<4,
        IEEE80211_RATE_HT_GF	=  1<<5,  /* green field */
        IEEE80211_RATE_HT_SGI	=  1<<5,  /* short guarding interval */
        IEEE80211_RATE_HT_FAT	=  1<<6, /* 40Mhz Channel */
        IEEE80211_RATE_HT_SISO	=  1<<7,
        IEEE80211_RATE_HT_MIMO	=  1<<8,
        IEEE80211_RATE_ANT_A   =  1<<8,    /* Number of antenna - no
radio goes over 3 so 4 will be enough */
        IEEE80211_RATE_ANT_B   =  1<<8,
        IEEE80211_RATE_ANT_C   =  1<<8,
        IEEE80211_RATE_ANT_D   =  1<<8,
};

/**
 * struct ieee80211_rate - bitrate definition
 *
 * This structure describes a bitrate that an 802.11 PHY can
 * operate with. The two values @hw_value and @hw_value_short
 * are only for driver use when pointers to this structure are
 * passed around.
 *
 * @flags: rate-specific flags
 * @bitrate: bitrate in units of 100 Kbps
 * @hw_value: driver/hardware value for this rate
 * @hw_value_short: driver/hardware value for this rate when
 *	short preamble is used
 */
struct ieee80211_rate {
	u32 flags;
	u16 bitrate;
	u16 hw_value, hw_value_short;
};
> johannes
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 1:14 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> Tomas Winkler wrote:
>
> > No I won't we add rate flags which are part of spec anyway  and then
> > plcp rate it self will be in bitrate.
>
> >         IEEE80211_RATE_HT_GF  =  1<<5,  /* green field */
> >         IEEE80211_RATE_HT_SGI =  1<<5,  /* short guarding interval */
> >         IEEE80211_RATE_HT_FAT =  1<<6, /* 40Mhz Channel */
> >         IEEE80211_RATE_HT_SISO        =  1<<7,
> >         IEEE80211_RATE_HT_MIMO        =  1<<8,
> >         IEEE80211_RATE_ANT_A   =  1<<8,    /* Number of antenna - no
> > radio goes over 3 so 4 will be enough */
> >         IEEE80211_RATE_ANT_B   =  1<<8,
> >         IEEE80211_RATE_ANT_C   =  1<<8,
> >         IEEE80211_RATE_ANT_D   =  1<<8,
>
> Ah, I see. Since these flags aren't really rate specific, so shouldn't
> they rather be part of the TX control, TX status and RX status directly
> rather than inside the rate? The flags within the bitrate are meant to
> indicate hardware capabilities and/or spec flags (like ERP) not control
> flags like these are.

No, they are rate specific the are not modulation specific, each of
them can change from packet to packet.
Thanks
Tomas

> johannes
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 1:23 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> Tomas Winkler wrote:
> > No, they are rate specific the are not modulation specific, each of
> > them can change from packet to packet.
>
> Exactly. flags in the bitrate struct aren't meant to change from packet to
> packet but be specific to a bitrate. The way I see it these are TX control
> or rate control flags, not rate flags.

I see, I got it.
Yes you are right. they should be part of the statuses.
Tomas

> johannes
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 1:26 PM, Tomas Winkler <tomasw@gmail.com> wrote:
>
> On Feb 4, 2008 1:23 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> > Tomas Winkler wrote:
> > > No, they are rate specific the are not modulation specific, each of
> > > them can change from packet to packet.
> >
> > Exactly. flags in the bitrate struct aren't meant to change from packet to
> > packet but be specific to a bitrate. The way I see it these are TX control
> > or rate control flags, not rate flags.
>
> I see, I got it.
> Yes you are right. they should be part of the statuses.
> Tomas
Last question before I got another patch out?
So what will  be use of ieee80211_rate in tx_control then?

> > johannes
> >
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 1:53 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> Tomas Winkler wrote:
>
> > Last question before I got another patch out?
> > So what will  be use of ieee80211_rate in tx_control then?
>
> Hmm? Not quite sure what you're asking. The tx_control has a pointer to
> the bitrate right now so that contains what bitrate the rate control
> algorithm selected (whether or not the hw downscales by itself).

So we add rate flags and pointer to bit rate structure?
Correct?

Thanks

> johannes
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 2:08 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> Tomas Winkler wrote:
>
> >> Hmm? Not quite sure what you're asking. The tx_control has a pointer to
> >> the bitrate right now so that contains what bitrate the rate control
> >> algorithm selected (whether or not the hw downscales by itself).
> >
> > So we add rate flags and pointer to bit rate structure?
> > Correct?
>
> I thought we just agreed not to because they're not bit rate but rather
> rate control specific. Think of it this way: the rate control algorithm
> selects a bitrate and those control flags you mentioned (SISO/MIMO/...),
> right?
>
> So the rate control algorithm has to communicate the bitrate and the flags
> to the hardware. It currently communicates the bitrate selection by giving
> a bitrate structure pointer (it could simply give the bitrate in kbps or
> whatever but that would mean the driver needs to look up its hw_value so
> this is the reason for passing the pointer) and cannot currently
> communicate the control flags selection.
>
> So the way I see it we'd add something like "u32 rate_control_flags" to
> struct tx_control for the flags you wanted, and also add a similar field
> (to be able to reuse the flag definitions) to the tx status & rx status
> structs. It seems to me that makes most sense, no?

I think we're talking about the same

> johannes
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Felix Fietkau]

Johannes Berg wrote:
> I thought we just agreed not to because they're not bit rate but rather
> rate control specific. Think of it this way: the rate control algorithm
> selects a bitrate and those control flags you mentioned (SISO/MIMO/...),
> right?
> 
> So the rate control algorithm has to communicate the bitrate and the flags
> to the hardware. It currently communicates the bitrate selection by giving
> a bitrate structure pointer (it could simply give the bitrate in kbps or
> whatever but that would mean the driver needs to look up its hw_value so
> this is the reason for passing the pointer) and cannot currently
> communicate the control flags selection.
> 
> So the way I see it we'd add something like "u32 rate_control_flags" to
> struct tx_control for the flags you wanted, and also add a similar field
> (to be able to reuse the flag definitions) to the tx status & rx status
> structs. It seems to me that makes most sense, no?
I'd like to have multiple rates in the tx status, including the number of
attempts for each rate. The Minstrel rate control needs these to build its
statistics properly.

- Felix

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 4:04 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> Felix Fietkau wrote:
>
> > I'd like to have multiple rates in the tx status, including the number of
> > attempts for each rate. The Minstrel rate control needs these to build its
> > statistics properly.

> Maybe we should add two new structures:
>  (a) struct ieee80211_ratectrl_tx_status
> and
>  (b) struct ieee80211_tx_ratectrl
>
> both containing a struct ieee80211_bitrate * and u32 rate_ctrl_flags, then
> we can build arrays of these where necessary and/or embed them in
> tx_status and tx_control.

I think we should also remove ieee80211_tx_control from  ieee80211_tx_status.
Maybe we should add blob to tx_status let say of skb->cb[] size and
field for rate scale algorithm version/name.  Otherwise we will be
inventing API for each rate scale algorithm... or maybe I'm giving up
to reach something general...

Tomas



> johannes
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 4:25 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> Tomas Winkler wrote:
>
> > I think we should also remove ieee80211_tx_control from
> > ieee80211_tx_status.
>
> Not sure. Where is it used?

Let's try, if it's break we won't be in worst situation then now :)

>
> > Maybe we should add blob to tx_status let say of skb->cb[] size and
> > field for rate scale algorithm version/name.  Otherwise we will be
> > inventing API for each rate scale algorithm...
>
> I disagree, we should have something generic enough to allow some sort of
> "capability negotiation" between the driver and the rate control
> algorithm, otherwise we could just remove rate control algorithms
> completely and internalize them into drivers (with some "helper" functions
> for those who don't want to write their own)... Which I'm sure you might
> actually like but I don't ;)

So Felix how many rates do you need for each packet?

Thanks
Tomas



> johannes
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Felix Fietkau]

Tomas Winkler wrote:
>> > Maybe we should add blob to tx_status let say of skb->cb[] size and
>> > field for rate scale algorithm version/name.  Otherwise we will be
>> > inventing API for each rate scale algorithm...
>>
>> I disagree, we should have something generic enough to allow some sort of
>> "capability negotiation" between the driver and the rate control
>> algorithm, otherwise we could just remove rate control algorithms
>> completely and internalize them into drivers (with some "helper" functions
>> for those who don't want to write their own)... Which I'm sure you might
>> actually like but I don't ;)
> 
> So Felix how many rates do you need for each packet?
The algorithm is designed to work with 4 different rates with adjustable
retry counts. A good description can be found here:
http://madwifi.org/browser/madwifi/trunk/ath_rate/minstrel/minstrel.txt

- Felix

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 5:24 PM, Felix Fietkau <nbd@openwrt.org> wrote:
> Tomas Winkler wrote:
> >> > Maybe we should add blob to tx_status let say of skb->cb[] size and
> >> > field for rate scale algorithm version/name.  Otherwise we will be
> >> > inventing API for each rate scale algorithm...
> >>
> >> I disagree, we should have something generic enough to allow some sort of
> >> "capability negotiation" between the driver and the rate control
> >> algorithm, otherwise we could just remove rate control algorithms
> >> completely and internalize them into drivers (with some "helper" functions
> >> for those who don't want to write their own)... Which I'm sure you might
> >> actually like but I don't ;)
> >
> > So Felix how many rates do you need for each packet?
> The algorithm is designed to work with 4 different rates with adjustable
> retry counts. A good description can be found here:
> http://madwifi.org/browser/madwifi/trunk/ath_rate/minstrel/minstrel.txt
>

Will this work work you?

ieee80211_tx_ratectrl {
  u32 bitrate ( or pointer to ieee80211_rate I prefer no)
  u32 flags
  u32 retry_count
}

ieee80211_tx_status {
    ieee80211_tx_ratectrl  retry_count[ X]  / * X >= 4 */
}


Thanks
Tomas

> - Felix
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Felix Fietkau]

Tomas Winkler wrote:
> Will this work work you?
> 
> ieee80211_tx_ratectrl {
>   u32 bitrate ( or pointer to ieee80211_rate I prefer no)
>   u32 flags
>   u32 retry_count
> }
> 
> ieee80211_tx_status {
>     ieee80211_tx_ratectrl  retry_count[ X]  / * X >= 4 */
> }
I guess that would work, but shouln't the array below be dynamic based on
what the driver can do? Many drivers can only work with one or two rates, IMHO.

- Felix

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 8:39 PM, Felix Fietkau <nbd@openwrt.org> wrote:
> Tomas Winkler wrote:
> > Will this work work you?
> >
> > ieee80211_tx_ratectrl {
> >   u32 bitrate ( or pointer to ieee80211_rate I prefer no)
> >   u32 flags
> >   u32 retry_count
> > }
> >
> > ieee80211_tx_status {
> >     ieee80211_tx_ratectrl  retry_count[ X]  / * X >= 4 */
> > }
> I guess that would work, but shouln't the array below be dynamic based on
> what the driver can do? Many drivers can only work with one or two rates, IMHO.

The problem that this structure is passed with each packett I don't
like the idea that anybody would even try to allocate and free this
structure as packets goes. However what about the following?

ieee80211_tx_status {
         size_t nrates;
         ieee80211_tx_ratectrl  retry_count[0] ;
}

> - Felix
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 4, 2008 9:03 PM, Tomas Winkler <tomasw@gmail.com> wrote:
>
> On Feb 4, 2008 8:39 PM, Felix Fietkau <nbd@openwrt.org> wrote:
> > Tomas Winkler wrote:
> > > Will this work work you?
> > >
> > > ieee80211_tx_ratectrl {
> > >   u32 bitrate ( or pointer to ieee80211_rate I prefer no)
> > >   u32 flags
> > >   u32 retry_count
> > > }
> > >
> > > ieee80211_tx_status {
> > >     ieee80211_tx_ratectrl  retry_count[ X]  / * X >= 4 */
> > > }
> > I guess that would work, but shouln't the array below be dynamic based on
> > what the driver can do? Many drivers can only work with one or two rates, IMHO.
>
> The problem that this structure is passed with each packett I don't
> like the idea that anybody would even try to allocate and free this
> structure as packets goes. However what about the following?
>
> ieee80211_tx_status {
>          size_t nrates;
>          ieee80211_tx_ratectrl  retry_count[0] ;
> }
>
Sorry I meant
ieee80211_tx_status {
>          size_t nrates;
>          ieee80211_tx_ratectrl  rates[0] ;
> }
> > - Felix
> >
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Stefano Brivio]

On Mon, 4 Feb 2008 21:05:25 +0200
"Tomas Winkler" <tomasw@gmail.com> wrote:

> Sorry I meant
> ieee80211_tx_status {
> >          size_t nrates;
> >          ieee80211_tx_ratectrl  rates[0] ;
> > }

FWIW, I'm OK with this. BTW, further changes to this may be required in
the future as I plan to rework a bit the whole tx_status thing so that
drivers won't have to report ACKed frames but failed and retransmitted
frames only - this would reduce the load on slow buses such as USB.


--
Ciao
Stefano

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Tomas Winkler]

On Feb 5, 2008 11:23 PM, Stefano Brivio <stefano.brivio@polimi.it> wrote:
> On Mon, 4 Feb 2008 21:05:25 +0200
> "Tomas Winkler" <tomasw@gmail.com> wrote:
>
> > Sorry I meant
> > ieee80211_tx_status {
> > >          size_t nrates;
> > >          ieee80211_tx_ratectrl  rates[0] ;
> > > }
>
> FWIW, I'm OK with this. BTW, further changes to this may be required in
> the future as I plan to rework a bit the whole tx_status thing so that
> drivers won't have to report ACKed frames but failed and retransmitted
> frames only - this would reduce the load on slow buses such as USB.
>
Just keep in mind that some driver requires to report ACKed frames to
make rate scaling algorithm to work properly, this should be optional
only.
Second I believe for some management frames it's good to notify mac
about successful transmission regardless of rate scaling algorithm.

Thanks
Tomas
> --
> Ciao
> Stefano
>

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Johannes Berg]

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


> Just keep in mind that some driver requires to report ACKed frames to
> make rate scaling algorithm to work properly, this should be optional
> only.
> Second I believe for some management frames it's good to notify mac
> about successful transmission regardless of rate scaling algorithm.

Hostapd wants that for EAPOL frames as well, so it has to be somehow
triggerable per-frame.

johannes

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

Re: [RFC] mac80211: add rate to ieee80211_tx_status

[Felix Fietkau]

Tomas Winkler wrote:
> The problem that this structure is passed with each packett I don't
> like the idea that anybody would even try to allocate and free this
> structure as packets goes. However what about the following?
> 
> ieee80211_tx_status {
>          size_t nrates;
>          ieee80211_tx_ratectrl  rates[0] ;
> }
Yes, I think that would be ok.

- Felix