[RFC v2] mac80211: add general rate information to Tx status

[RFC v2] mac80211: add general rate information to Tx status

[Tomas Winkler]

From: Ron Rindjunsky <ron.rindjunsky@intel.com>

This patch introduces data structures meant to contain actual
configuration in which Tx was made in HW, for general purposes
and for rate scaling in specific.
it includes the rate, general flags for the rate (e.g. Tx antenna)
and the number of retries for the rate.

Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 include/net/mac80211.h |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 460da54..ec966c8 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -318,6 +318,22 @@ struct ieee80211_rx_status {
 };
 
 /**
+ * struct ieee80211_tx_rate_ctrl - rate ctrl data
+ *
+ * The low-level driver should provide this struct as part
+ * of the ieee80211_tx_status. this struct should give information
+ * about the tx that is not part of the plcp rates table.
+ * @bitrate: rate used.
+ * @flags: Tx information (e.g. antenna selectd, guard interval, etc.)
+ * @retry_count: number of retries for this specific rate
+ */
+struct ieee80211_tx_rate_ctrl {
+	u32 bitrate;
+	u32 flags;
+	u8 retry_count;
+};
+
+/**
  * enum ieee80211_tx_status_flags - transmit status flags
  *
  * Status flags to indicate various transmit conditions.
@@ -343,7 +359,7 @@ enum ieee80211_tx_status_flags {
  * @control: a copy of the &struct ieee80211_tx_control passed to the driver
  *	in the tx() callback.
  * @flags: transmit status flags, defined above
- * @retry_count: number of retries
+ * @retry_count: total number of retries
  * @excessive_retries: set to 1 if the frame was retried many times
  *	but not acknowledged
  * @ampdu_ack_len: number of aggregated frames.
@@ -353,6 +369,8 @@ enum ieee80211_tx_status_flags {
  * @ack_signal: signal strength of the ACK frame
  * @queue_length: ?? REMOVE
  * @queue_number: ?? REMOVE
+ * @rate_ctrl_num: number of rate_ctrl allocated
+ * @rate_ctrl: info for rate at which the packet was actually transmitted
  */
 struct ieee80211_tx_status {
 	struct ieee80211_tx_control control;
@@ -364,6 +382,8 @@ struct ieee80211_tx_status {
 	int ack_signal;
 	int queue_length;
 	int queue_number;
+	size_t rate_ctrl_num;
+	struct ieee80211_tx_rate_ctrl rate_ctrl[0];
 };
 
 /**
-- 
1.5.2.2

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [RFC v2] mac80211: add general rate information to Tx status

[Tomas Winkler]

On Feb 5, 2008 5:45 PM, Tomas Winkler <tomas.winkler@intel.com> wrote:
> From: Ron Rindjunsky <ron.rindjunsky@intel.com>
>
> This patch introduces data structures meant to contain actual
> configuration in which Tx was made in HW, for general purposes
> and for rate scaling in specific.
> it includes the rate, general flags for the rate (e.g. Tx antenna)
> and the number of retries for the rate.
>
> Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  include/net/mac80211.h |   22 +++++++++++++++++++++-
>  1 files changed, 21 insertions(+), 1 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 460da54..ec966c8 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -318,6 +318,22 @@ struct ieee80211_rx_status {
>  };
>
>  /**
> + * struct ieee80211_tx_rate_ctrl - rate ctrl data
> + *
> + * The low-level driver should provide this struct as part
> + * of the ieee80211_tx_status. this struct should give information
> + * about the tx that is not part of the plcp rates table.
> + * @bitrate: rate used.
> + * @flags: Tx information (e.g. antenna selectd, guard interval, etc.)
> + * @retry_count: number of retries for this specific rate
> + */
> +struct ieee80211_tx_rate_ctrl {
> +       u32 bitrate;
> +       u32 flags;
> +       u8 retry_count;
> +};
> +
> +/**
>   * enum ieee80211_tx_status_flags - transmit status flags
>   *
>   * Status flags to indicate various transmit conditions.
> @@ -343,7 +359,7 @@ enum ieee80211_tx_status_flags {
>   * @control: a copy of the &struct ieee80211_tx_control passed to the driver
>   *     in the tx() callback.
>   * @flags: transmit status flags, defined above
> - * @retry_count: number of retries
> + * @retry_count: total number of retries
>   * @excessive_retries: set to 1 if the frame was retried many times
>   *     but not acknowledged
>   * @ampdu_ack_len: number of aggregated frames.
> @@ -353,6 +369,8 @@ enum ieee80211_tx_status_flags {
>   * @ack_signal: signal strength of the ACK frame
>   * @queue_length: ?? REMOVE
>   * @queue_number: ?? REMOVE
> + * @rate_ctrl_num: number of rate_ctrl allocated
> + * @rate_ctrl: info for rate at which the packet was actually transmitted
>   */
>  struct ieee80211_tx_status {
>         struct ieee80211_tx_control control;
> @@ -364,6 +382,8 @@ struct ieee80211_tx_status {
>         int ack_signal;
>         int queue_length;
>         int queue_number;
> +       size_t rate_ctrl_num;
> +       struct ieee80211_tx_rate_ctrl rate_ctrl[0];
>  };
>
>  /**
> --
> 1.5.2.2


Felix will this work for you?

Thanks
Tomas
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC v2] mac80211: add general rate information to Tx status

[Felix Fietkau]

Tomas Winkler wrote:
> On Feb 5, 2008 5:45 PM, Tomas Winkler <tomas.winkler@intel.com> wrote:
>> From: Ron Rindjunsky <ron.rindjunsky@intel.com>
>>
>> This patch introduces data structures meant to contain actual
>> configuration in which Tx was made in HW, for general purposes
>> and for rate scaling in specific.
>> it includes the rate, general flags for the rate (e.g. Tx antenna)
>> and the number of retries for the rate.
>>
>> Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>> ---
>>  include/net/mac80211.h |   22 +++++++++++++++++++++-
>>  1 files changed, 21 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
>> index 460da54..ec966c8 100644
>> --- a/include/net/mac80211.h
>> +++ b/include/net/mac80211.h
>> @@ -318,6 +318,22 @@ struct ieee80211_rx_status {
>>  };
>>
>>  /**
>> + * struct ieee80211_tx_rate_ctrl - rate ctrl data
>> + *
>> + * The low-level driver should provide this struct as part
>> + * of the ieee80211_tx_status. this struct should give information
>> + * about the tx that is not part of the plcp rates table.
>> + * @bitrate: rate used.
>> + * @flags: Tx information (e.g. antenna selectd, guard interval, etc.)
>> + * @retry_count: number of retries for this specific rate
>> + */
>> +struct ieee80211_tx_rate_ctrl {
>> +       u32 bitrate;
>> +       u32 flags;
>> +       u8 retry_count;
>> +};
>> +
>> +/**
>>   * enum ieee80211_tx_status_flags - transmit status flags
>>   *
>>   * Status flags to indicate various transmit conditions.
>> @@ -343,7 +359,7 @@ enum ieee80211_tx_status_flags {
>>   * @control: a copy of the &struct ieee80211_tx_control passed to the driver
>>   *     in the tx() callback.
>>   * @flags: transmit status flags, defined above
>> - * @retry_count: number of retries
>> + * @retry_count: total number of retries
>>   * @excessive_retries: set to 1 if the frame was retried many times
>>   *     but not acknowledged
>>   * @ampdu_ack_len: number of aggregated frames.
>> @@ -353,6 +369,8 @@ enum ieee80211_tx_status_flags {
>>   * @ack_signal: signal strength of the ACK frame
>>   * @queue_length: ?? REMOVE
>>   * @queue_number: ?? REMOVE
>> + * @rate_ctrl_num: number of rate_ctrl allocated
>> + * @rate_ctrl: info for rate at which the packet was actually transmitted
>>   */
>>  struct ieee80211_tx_status {
>>         struct ieee80211_tx_control control;
>> @@ -364,6 +382,8 @@ struct ieee80211_tx_status {
>>         int ack_signal;
>>         int queue_length;
>>         int queue_number;
>> +       size_t rate_ctrl_num;
>> +       struct ieee80211_tx_rate_ctrl rate_ctrl[0];
>>  };
>>
>>  /**
>> --
>> 1.5.2.2
> 
> 
> Felix will this work for you?
Yes, looks good.

- Felix

Re: [RFC v2] mac80211: add general rate information to Tx status

[Johannes Berg]

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


> + * @flags: Tx information (e.g. antenna selectd, guard interval,
> etc.)

typo: selected, also where are the flags defined?

>  struct ieee80211_tx_status {
>  	struct ieee80211_tx_control control;
> @@ -364,6 +382,8 @@ struct ieee80211_tx_status {
>  	int ack_signal;
>  	int queue_length;
>  	int queue_number;
> +	size_t rate_ctrl_num;
> +	struct ieee80211_tx_rate_ctrl rate_ctrl[0];

I think this is going to blow up if any driver sets rate_ctrl_num > 0
and uses the irq-safe tx status interface because we there copy the
stuff into another buffer and you haven't changed that buffer to be
resized accordingly.

johannes

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

Re: [RFC v2] mac80211: add general rate information to Tx status

[Tomas Winkler]

On Feb 6, 2008 4:15 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> > + * @flags: Tx information (e.g. antenna selectd, guard interval,
> > etc.)
>
> typo: selected, also where are the flags defined?
>
> >  struct ieee80211_tx_status {
> >       struct ieee80211_tx_control control;
> > @@ -364,6 +382,8 @@ struct ieee80211_tx_status {
> >       int ack_signal;
> >       int queue_length;
> >       int queue_number;
> > +     size_t rate_ctrl_num;
> > +     struct ieee80211_tx_rate_ctrl rate_ctrl[0];
>
> I think this is going to blow up if any driver sets rate_ctrl_num > 0
> and uses the irq-safe tx status interface because we there copy the
> stuff into another buffer and you haven't changed that buffer to be
> resized accordingly.

I have the same concern and wanted to set to some reasonable number
but Felix thinks otherwise.
In general the driver should know how many rates it wants to report so
in TX-response path it's allocates enough memory.
Since on the TX path tx_status resides on stack this has to be copied
by the driver for TX-response path use.
Yes it's still , it is quite error prone  :(, maybe some helper
macros/functions can make it less vulnerable.

sizeof(struct ieee80211_tx_status) +  rates_num * sizeof(struct
ieee80211_tx_rate_ctrl)


void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
				 struct sk_buff *skb,
				 struct ieee80211_tx_status *status)
	

	skb->dev = local->mdev;
	saved = kmalloc(sizeof(struct ieee80211_tx_status), GFP_ATOMIC); ---
this has to be changed as well.
	if (unlikely(!saved)) {



> johannes
>

Re: [RFC v2] mac80211: add general rate information to Tx status

[Johannes Berg]

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

> In general the driver should know how many rates it wants to report so
> in TX-response path it's allocates enough memory.

Yeah, obviously.

> Since on the TX path tx_status resides on stack this has to be copied
> by the driver for TX-response path use.

I don't think I understand? This is tx status, what are you referring to
by "tx response"?

> void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
> 				 struct sk_buff *skb,
> 				 struct ieee80211_tx_status *status)
> 	
> 
> 	skb->dev = local->mdev;
> 	saved = kmalloc(sizeof(struct ieee80211_tx_status), GFP_ATOMIC); ---
> this has to be changed as well.

Yeah that's what I was thinking of. Maybe we should change the
ieee80211_tx_status_irqsafe() interface and require the driver to
allocate it rather than giving a pointer to a structure on the stack?
That way we the driver has to do the calculation, but we don't have to
copy things around? Doing it on the stack isn't easy, you'd have to
define something like this:

struct status {
	struct ieee80211_tx_status stat;
	struct ieee80211_tx_rate_ctrl ratectrl[N];
} status __attribute__((packed));

so requiring the driver to allocate it when using the irqsafe interface
would be better imho.

johannes

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

Re: [RFC v2] mac80211: add general rate information to Tx status

[Tomas Winkler]

> > Since on the TX path tx_status resides on stack this has to be copied
> > by the driver for TX-response path use.
>
> I don't think I understand? This is tx status, what are you referring to
> by "tx response"?

Maybe I'm locked in concepts of iwlwifi but I believe also other
drivers uses similar concept. I think USB has some URB complete
handler etc.  So iwlwifi get interrupt (asynchronous) when packet is
transmitted (TX response flow) and driver retrieves the information of
transmitting. This is NOT done in TX contexts so you have to store
tx_status between TX flows and TX response flow so you can feed it
back to ieee80211_tx_status_irqsafe
>
> > void ieee80211_tx_status_irqsafe(struct ieee80211_hw *hw,
> >                                struct sk_buff *skb,
> >                                struct ieee80211_tx_status *status)
> >
> >
> >       skb->dev = local->mdev;
> >       saved = kmalloc(sizeof(struct ieee80211_tx_status), GFP_ATOMIC); ---
> > this has to be changed as well.
>
> Yeah that's what I was thinking of. Maybe we should change the
> ieee80211_tx_status_irqsafe() interface and require the driver to
> allocate it rather than giving a pointer to a structure on the stack?
> That way we the driver has to do the calculation, but we don't have to
> copy things around? Doing it on the stack isn't easy, you'd have to
> define something like this:

>
> struct status {
>         struct ieee80211_tx_status stat;
>         struct ieee80211_tx_rate_ctrl ratectrl[N];
> } status __attribute__((packed));
>
> so requiring the driver to allocate it when using the irqsafe interface
> would be better imho.
>
Not sure which flow will free this pointer ? Iwlwifi keeps tx_status
attached to TBD so it have to be sure it won't be overriden somewhere
in the stack.  But it might be a case that I'm not understand here
something

Thanks
Tomas

> johannes
>

Re: [RFC v2] mac80211: add general rate information to Tx status

[Johannes Berg]

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


> Maybe I'm locked in concepts of iwlwifi but I believe also other
> drivers uses similar concept. I think USB has some URB complete
> handler etc.  So iwlwifi get interrupt (asynchronous) when packet is
> transmitted (TX response flow) and driver retrieves the information of
> transmitting. This is NOT done in TX contexts so you have to store
> tx_status between TX flows and TX response flow so you can feed it
> back to ieee80211_tx_status_irqsafe

Ah. But where's the difference between "TX response" and "TX status"
then? Anyway, yes, you do have to store the info between ->tx() and
tx_status().

> Not sure which flow will free this pointer ? Iwlwifi keeps tx_status
> attached to TBD so it have to be sure it won't be overriden somewhere
> in the stack.  But it might be a case that I'm not understand here
> something

I'd think that mac80211 would free it. I guess we should then have
something like

static inline struct ieee80211_tx_status *
ieee80211_alloc_tx_status(size_t num_rate_ctrl)
{
	struct ieee80211_tx_status *s;
	s = kzalloc(... the expression you had earlier...);
	if (s)
		s->num_rate_control = num_rate_control;
	return s;
}

so that we can change that w/o changing all drivers. In fact, you'd
probably already allocate that in the TX flow and keep it around until
TX status.

Mind you, I'm just writing whatever comes to my head ;) Better ideas
welcome, especially since this makes tx_status_irqsafe() and tx_status()
behave completely differently. Incidentally, why does iwlwifi use both
depending on in_interrupt(), or rather, why isn't the content statically
known?

johannes

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

Re: [RFC v2] mac80211: add general rate information to Tx status

[Tomas Winkler]

On Feb 6, 2008 6:28 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> > Maybe I'm locked in concepts of iwlwifi but I believe also other
> > drivers uses similar concept. I think USB has some URB complete
> > handler etc.  So iwlwifi get interrupt (asynchronous) when packet is
> > transmitted (TX response flow) and driver retrieves the information of
> > transmitting. This is NOT done in TX contexts so you have to store
> > tx_status between TX flows and TX response flow so you can feed it
> > back to ieee80211_tx_status_irqsafe
>
> Ah. But where's the difference between "TX response" and "TX status"
> then?

There are the same, just terminology juggling.

Anyway, yes, you do have to store the info between ->tx() and
> tx_status().
>
> > Not sure which flow will free this pointer ? Iwlwifi keeps tx_status
> > attached to TBD so it have to be sure it won't be overriden somewhere
> > in the stack.  But it might be a case that I'm not understand here
> > something
>
> I'd think that mac80211 would free it. I guess we should then have
> something like
>
> static inline struct ieee80211_tx_status *
> ieee80211_alloc_tx_status(size_t num_rate_ctrl)
> {
>         struct ieee80211_tx_status *s;
>         s = kzalloc(... the expression you had earlier...);
>         if (s)
>                 s->num_rate_control = num_rate_control;
>         return s;
> }

> so that we can change that w/o changing all drivers. In fact, you'd
> probably already allocate that in the TX flow and keep it around until
> TX status.

It's allocated in <bus>_probe stage. The 'dynamic' allocation is done
only by mac in ieee80211_tx_status_irqsafe.


> Mind you, I'm just writing whatever comes to my head ;) Better ideas
> welcome, especially since this makes tx_status_irqsafe() and tx_status()
> behave completely differently.

I prefer to start to converge to something since iwlwifi driver is
broken for week on the tip of everything.

 Incidentally, why does iwlwifi use both
> depending on in_interrupt(), or rather, why isn't the content statically
> known?

Actually in spite of the IF there I don't think  it  ever happens that
iwlwifi uses tx_status() directly only irqsafe. Does any driver uses
tx_status directly anyway?

Tomas

Tomas
> johannes
>

Re: [RFC v2] mac80211: add general rate information to Tx status

[Johannes Berg]

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


> > Mind you, I'm just writing whatever comes to my head ;) Better ideas
> > welcome, especially since this makes tx_status_irqsafe() and tx_status()
> > behave completely differently.
> 
> I prefer to start to converge to something since iwlwifi driver is
> broken for week on the tip of everything.

Yeah let's fix that and then do the larger redesign later.

>  Incidentally, why does iwlwifi use both
> > depending on in_interrupt(), or rather, why isn't the content statically
> > known?
> 
> Actually in spite of the IF there I don't think  it  ever happens that
> iwlwifi uses tx_status() directly only irqsafe. Does any driver uses
> tx_status directly anyway?

ath5k does, but I have no idea why.

johannes

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

Re: [RFC v2] mac80211: add general rate information to Tx status

[Nick Kossifidis]

> > Actually in spite of the IF there I don't think  it  ever happens that
> > iwlwifi uses tx_status() directly only irqsafe. Does any driver uses
> > tx_status directly anyway?
>
> ath5k does, but I have no idea why.
>

We have separate tasklets for tx/rx (so we can parse descriptrors,
keep buffers/descriptors in sync etc), so we call ieee80211_tx_status
from our tx tasklet.


-- 
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick