[PATCH/RFT] iwlagn: remove pointless TX frame check

[PATCH/RFT] iwlagn: remove pointless TX frame check

[Johannes Berg]

mac80211 will not send data frames on a STA mode interface
that is not associated because the queue for it is stopped,
and all remaining data frames that might be sent, e.g. by
packet injection, are accepted here anyway, so remove this
pointless check. Also hold the spinlock for less time.

This will with great probability improve performance in the
driver more than the proper descriptor layout can possibly
cost.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
Haven't tested this so far, so RFT, but I'm fairly certain it's correct.

 drivers/net/wireless/iwlwifi/iwl-tx.c |   26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

--- everything.orig/drivers/net/wireless/iwlwifi/iwl-tx.c	2008-10-09 11:07:05.000000000 +0200
+++ everything/drivers/net/wireless/iwlwifi/iwl-tx.c	2008-10-09 11:08:45.000000000 +0200
@@ -787,17 +787,18 @@ int iwl_tx_skb(struct iwl_priv *priv, st
 	unsigned long flags;
 	int ret;
 
-	spin_lock_irqsave(&priv->lock, flags);
-	if (iwl_is_rfkill(priv)) {
-		IWL_DEBUG_DROP("Dropping - RF KILL\n");
-		goto drop_unlock;
-	}
-
 	if ((ieee80211_get_tx_rate(priv->hw, info)->hw_value & 0xFF) ==
 	     IWL_INVALID_RATE) {
 		IWL_ERROR("ERROR: No TX rate available.\n");
+		goto drop;
+	}
+
+	spin_lock_irqsave(&priv->lock, flags);
+	if (iwl_is_rfkill(priv)) {
+		IWL_DEBUG_DROP("Dropping - RF KILL\n");
 		goto drop_unlock;
 	}
+	spin_unlock_irqrestore(&priv->lock, flags);
 
 	unicast = !is_multicast_ether_addr(hdr->addr1);
 
@@ -812,19 +813,6 @@ int iwl_tx_skb(struct iwl_priv *priv, st
 		IWL_DEBUG_TX("Sending REASSOC frame\n");
 #endif
 
-	/* drop all data frame if we are not associated */
-	if (ieee80211_is_data(fc) &&
-	    (priv->iw_mode != NL80211_IFTYPE_MONITOR ||
-	    !(info->flags & IEEE80211_TX_CTL_INJECTED)) && /* packet injection */
-	    (!iwl_is_associated(priv) ||
-	     ((priv->iw_mode == NL80211_IFTYPE_STATION) && !priv->assoc_id) ||
-	     !priv->assoc_station_added)) {
-		IWL_DEBUG_DROP("Dropping - !iwl_is_associated\n");
-		goto drop_unlock;
-	}
-
-	spin_unlock_irqrestore(&priv->lock, flags);
-
 	hdr_len = ieee80211_hdrlen(fc);
 
 	/* Find (or create) index into station table for destination station */

Re: [PATCH/RFT] iwlagn: remove pointless TX frame check

[Tomas Winkler]

On Thu, Oct 9, 2008 at 12:22 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> mac80211 will not send data frames on a STA mode interface
> that is not associated because the queue for it is stopped,
> and all remaining data frames that might be sent, e.g. by
> packet injection, are accepted here anyway, so remove this
> pointless check. Also hold the spinlock for less time.
>
> This will with great probability improve performance in the
> driver more than the proper descriptor layout can possibly
> cost.
>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> Haven't tested this so far, so RFT, but I'm fairly certain it's correct.


NACK, the completion of association of mac80211 and iwlagn isn't fully
synchronized so there is still
a race.  The check can be simplified but there are much more places
that need to be touched.
Tomas

>  drivers/net/wireless/iwlwifi/iwl-tx.c |   26 +++++++-------------------
>  1 file changed, 7 insertions(+), 19 deletions(-)
>
> --- everything.orig/drivers/net/wireless/iwlwifi/iwl-tx.c       2008-10-09 11:07:05.000000000 +0200
> +++ everything/drivers/net/wireless/iwlwifi/iwl-tx.c    2008-10-09 11:08:45.000000000 +0200
> @@ -787,17 +787,18 @@ int iwl_tx_skb(struct iwl_priv *priv, st
>        unsigned long flags;
>        int ret;
>
> -       spin_lock_irqsave(&priv->lock, flags);
> -       if (iwl_is_rfkill(priv)) {
> -               IWL_DEBUG_DROP("Dropping - RF KILL\n");
> -               goto drop_unlock;
> -       }
> -
>        if ((ieee80211_get_tx_rate(priv->hw, info)->hw_value & 0xFF) ==
>             IWL_INVALID_RATE) {
>                IWL_ERROR("ERROR: No TX rate available.\n");
> +               goto drop;
> +       }
> +
> +       spin_lock_irqsave(&priv->lock, flags);
> +       if (iwl_is_rfkill(priv)) {
> +               IWL_DEBUG_DROP("Dropping - RF KILL\n");
>                goto drop_unlock;
>        }
> +       spin_unlock_irqrestore(&priv->lock, flags);
>
>        unicast = !is_multicast_ether_addr(hdr->addr1);
>
> @@ -812,19 +813,6 @@ int iwl_tx_skb(struct iwl_priv *priv, st
>                IWL_DEBUG_TX("Sending REASSOC frame\n");
>  #endif
>
> -       /* drop all data frame if we are not associated */
> -       if (ieee80211_is_data(fc) &&
> -           (priv->iw_mode != NL80211_IFTYPE_MONITOR ||
> -           !(info->flags & IEEE80211_TX_CTL_INJECTED)) && /* packet injection */
> -           (!iwl_is_associated(priv) ||
> -            ((priv->iw_mode == NL80211_IFTYPE_STATION) && !priv->assoc_id) ||
> -            !priv->assoc_station_added)) {
> -               IWL_DEBUG_DROP("Dropping - !iwl_is_associated\n");
> -               goto drop_unlock;
> -       }
> -
> -       spin_unlock_irqrestore(&priv->lock, flags);
> -
>        hdr_len = ieee80211_hdrlen(fc);
>
>        /* Find (or create) index into station table for destination station */
>
>
>

Re: [PATCH/RFT] iwlagn: remove pointless TX frame check

[Johannes Berg]

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

On Thu, 2008-10-09 at 22:08 +0200, Tomas Winkler wrote:
> On Thu, Oct 9, 2008 at 12:22 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > mac80211 will not send data frames on a STA mode interface
> > that is not associated because the queue for it is stopped,
> > and all remaining data frames that might be sent, e.g. by
> > packet injection, are accepted here anyway, so remove this
> > pointless check. Also hold the spinlock for less time.
> >
> > This will with great probability improve performance in the
> > driver more than the proper descriptor layout can possibly
> > cost.
> >
> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > ---
> > Haven't tested this so far, so RFT, but I'm fairly certain it's correct.
> 
> 
> NACK, the completion of association of mac80211 and iwlagn isn't fully
> synchronized so there is still
> a race.

Can we fix that?

johannes

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

Re: [PATCH/RFT] iwlagn: remove pointless TX frame check

[Johannes Berg]

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

On Fri, 2008-10-10 at 11:14 +0200, Johannes Berg wrote:

> > NACK, the completion of association of mac80211 and iwlagn isn't fully
> > synchronized so there is still
> > a race.
> 
> Can we fix that?

Ping? Any idea _where_ the two can get desynchronized?

johannes

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

Re: [PATCH/RFT] iwlagn: remove pointless TX frame check

[Johannes Berg]

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

On Thu, 2008-10-23 at 22:42 +0200, Johannes Berg wrote:
> On Fri, 2008-10-10 at 11:14 +0200, Johannes Berg wrote:
> 
> > > NACK, the completion of association of mac80211 and iwlagn isn't fully
> > > synchronized so there is still
> > > a race.
> > 
> > Can we fix that?
> 
> Ping? Any idea _where_ the two can get desynchronized?

Ping again. I like verifiable reasons for a "NACK".

johannes

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

Re: [PATCH/RFT] iwlagn: remove pointless TX frame check

[Tomas Winkler]

On Thu, Nov 6, 2008 at 2:30 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2008-10-23 at 22:42 +0200, Johannes Berg wrote:
>> On Fri, 2008-10-10 at 11:14 +0200, Johannes Berg wrote:
>>
>> > > NACK, the completion of association of mac80211 and iwlagn isn't fully
>> > > synchronized so there is still
>> > > a race.
>> >
>> > Can we fix that?
>>
>> Ping? Any idea _where_ the two can get desynchronized?
>
> Ping again. I like verifiable reasons for a "NACK".

I'm working on it right now. Practically instead of association check
what will  remain is check if everything is station is properly
configured. Meaning stations is configured in the set in the firmware
and rate scale is also set.
This will cover all modes. Need to implement sta_notify handler and
move some code from rate scaling to the driver.
There will be never full synchronization between mac80211 and the
driver about internal station state but the check might be very
simplified.

Thanks
Tomas

Re: [PATCH/RFT] iwlagn: remove pointless TX frame check

[Johannes Berg]

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

On Thu, 2008-11-06 at 16:41 +0200, Tomas Winkler wrote:

> I'm working on it right now. Practically instead of association check
> what will  remain is check if everything is station is properly
> configured. Meaning stations is configured in the set in the firmware
> and rate scale is also set.

That makes sense, though I think for frames we can't find the station we
use the broadcast station anyway (to make injection possible), so
shouldn't the station _always_ exist?

> This will cover all modes. Need to implement sta_notify handler and
> move some code from rate scaling to the driver.

Alright.

> There will be never full synchronization between mac80211 and the
> driver about internal station state but the check might be very
> simplified.

Good, thanks.

johannes

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

Re: [PATCH/RFT] iwlagn: remove pointless TX frame check

[Tomas Winkler]

On Thu, Nov 6, 2008 at 4:45 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2008-11-06 at 16:41 +0200, Tomas Winkler wrote:
>
>> I'm working on it right now. Practically instead of association check
>> what will  remain is check if everything is station is properly
>> configured. Meaning stations is configured in the set in the firmware
>> and rate scale is also set.
>
> That makes sense, though I think for frames we can't find the station we
> use the broadcast station anyway (to make injection possible), so
> shouldn't the station _always_ exist?

That's not the problem I'm trying to solve. I want to replace
association check, which ensure that two conditions are satisfied with
check for particular station including bcast station (bcast station is
just configured always a head)
Tomas