"Try to stop Tx aggregation on non active TID" messages

"Try to stop Tx aggregation on non active TID" messages

[Johannes Berg]

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

Hi Ron,

It seems that after commit b4b5e1f49e9982a1ea0b3408e0df6adee49dad54

    mac80211: tear down of block ack sessions
    
    This patch adds a clean tear down for all block ack sessions if interface
    goes down or if a deauthentication is done.
    
    Signed-off-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
    Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
    Signed-off-by: John W. Linville <linville@tuxdriver.com>

we have a slew of messages saying

[18131.640793] Stop a BA session requested for 00:1a:2a:2c:a2:c5 tid 13
[18131.640798] Try to stop Tx aggregation on non active TID

just like we had for the RX case before.

Please decide once and for all decide whether it is valid to call those
functions, now especially ieee80211_stop_tx_ba_session() when no session
was created.

As indicated by this message rate control algorithms shouldn't call this
function when no TX BA session is present. Therefore the stack itself
shouldn't do so either. The same holds for RX BA sessions.

Please make sure the stack itself always uses the functions only as
specified by their API. Feel free to change the API to allow calling a
function without as session in progress, but in that case please don't
print out anything at all.

Thanks,
johannes

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

Re: "Try to stop Tx aggregation on non active TID" messages

[Ron Rindjunsky]

> we have a slew of messages saying
>
> [18131.640793] Stop a BA session requested for 00:1a:2a:2c:a2:c5 tid 13
> [18131.640798] Try to stop Tx aggregation on non active TID
>
> just like we had for the RX case before.
>
> Feel free to change the API to allow calling a
> function without as session in progress, but in that case please don't
> print out anything at all.

yes, i noticed that. the prints are coming of course from the MLME
itself that can be called while no session is in progress and checks
the status when called.
i'll do a quick check to decide which way is better (outside check of
the state machine or just a silent discard of non active TIDs
requests) and fix it.
Ron

Re: "Try to stop Tx aggregation on non active TID" messages

[Ron Rindjunsky]

> > we have a slew of messages saying
> >
> > [18131.640793] Stop a BA session requested for 00:1a:2a:2c:a2:c5 tid 13
> > [18131.640798] Try to stop Tx aggregation on non active TID
> >
> > just like we had for the RX case before.
> >
> > Feel free to change the API to allow calling a
> > function without as session in progress, but in that case please don't
> > print out anything at all.
>
> yes, i noticed that. the prints are coming of course from the MLME
> itself that can be called while no session is in progress and checks
> the status when called.
> i'll do a quick check to decide which way is better (outside check of
> the state machine or just a silent discard of non active TIDs
> requests) and fix it.
> Ron

ok, i checked it. i think it is the same as the Rx issue we had. those
prints can be eliminated by .config file's MAC80211_HT_DEBUG. if we
don't use this flag what's the use of it? if we do use it, i would
like to insert the Rx prints back as well.
Ron

Re: "Try to stop Tx aggregation on non active TID" messages

[John W. Linville]

On Sun, Mar 30, 2008 at 02:56:27PM +0300, Ron Rindjunsky wrote:
> > > we have a slew of messages saying
> > >
> > > [18131.640793] Stop a BA session requested for 00:1a:2a:2c:a2:c5 tid 13
> > > [18131.640798] Try to stop Tx aggregation on non active TID

> ok, i checked it. i think it is the same as the Rx issue we had. those
> prints can be eliminated by .config file's MAC80211_HT_DEBUG. if we
> don't use this flag what's the use of it? if we do use it, i would
> like to insert the Rx prints back as well.

Perhaps use MAC80211_VERBOSE_DEBUG?

-- 
John W. Linville
linville@tuxdriver.com

Re: "Try to stop Tx aggregation on non active TID" messages

[Johannes Berg]

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


> > yes, i noticed that. the prints are coming of course from the MLME
> > itself that can be called while no session is in progress and checks
> > the status when called.
> > i'll do a quick check to decide which way is better (outside check of
> > the state machine or just a silent discard of non active TIDs
> > requests) and fix it.
> > Ron
> 
> ok, i checked it. i think it is the same as the Rx issue we had. those
> prints can be eliminated by .config file's MAC80211_HT_DEBUG. if we
> don't use this flag what's the use of it? if we do use it, i would
> like to insert the Rx prints back as well.

Well, yes, but I think most debugging features in mac80211 should enable
us to see things going wrong, not totally flood us in messages. YMMV,
but if these things drown you in messages then even developers will not
enable them by default which means that they've effectively been made
useless [1]. Hence, I think _DEBUG symbols should be ok to enable by
default, not print too many messages but enable just some important
messages and further debugging checks.

Now, maybe that message is actually one of such debugging checks, in
that it tells you when you did a wrong (i.e. while not enabled) call.
But then mac80211 itself shouldn't do such calls.

johannes

[1] as, incidentally, you can see in the current wireless tree from a
few days ago, iwlwifi with debugging doesn't even compile!

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

RE: "Try to stop Tx aggregation on non active TID" messages

[Chatre, Reinette]

On , Johannes Berg  wrote:

> [1] as, incidentally, you can see in the current wireless tree from a
> few days ago, iwlwifi with debugging doesn't even compile!

Could you please send the errors you see? The debug flags recently
changed, so the following is needed:
CONFIG_IWLWIFI_DEBUG=y
CONFIG_IWL3945_DEBUG=y

Reinette

RE: "Try to stop Tx aggregation on non active TID" messages

[Johannes Berg]

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


On Mon, 2008-03-31 at 14:00 -0700, Chatre, Reinette wrote:
> On , Johannes Berg  wrote:
> 
> > [1] as, incidentally, you can see in the current wireless tree from a
> > few days ago, iwlwifi with debugging doesn't even compile!
> 
> Could you please send the errors you see? The debug flags recently
> changed, so the following is needed:
> CONFIG_IWLWIFI_DEBUG=y
> CONFIG_IWL3945_DEBUG=y

I'll double check and send if anything still occurs, I realised (after
sending that mail of course...) that the error was from the machine that
has the card in it on which I might have forgotten to update the kernel
sources.

johannes

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

RE: "Try to stop Tx aggregation on non active TID" messages

[Johannes Berg]

> > [1] as, incidentally, you can see in the current wireless tree from a
> > few days ago, iwlwifi with debugging doesn't even compile!
> 
> Could you please send the errors you see? The debug flags recently
> changed, so the following is needed:

It seems that this was before I updated to after the debug flags change,
everything seems fine now.

Unrelated, but while I was at it, I get a whole bunch of warnings that
are fixed by the patch below.

johannes

From: Johannes Berg <johannes@sipsolutions.net>
Subject: iwlwifi: fix warnings

This fixes all kinds of warnings in iwlwifi.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 drivers/net/wireless/iwlwifi/iwl-3945.c     |    6 +++---
 drivers/net/wireless/iwlwifi/iwl-3945.h     |    4 ++--
 drivers/net/wireless/iwlwifi/iwl-4965.c     |    8 ++++----
 drivers/net/wireless/iwlwifi/iwl-4965.h     |    4 ++--
 drivers/net/wireless/iwlwifi/iwl-debugfs.c  |    2 +-
 drivers/net/wireless/iwlwifi/iwl4965-base.c |    2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965.c	2008-04-01 17:38:21.311669543 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965.c	2008-04-01 17:43:17.931669482 +0200
@@ -3297,7 +3297,7 @@ static void iwl4965_add_radiotap(struct 
 	s8 noise = 0;
 	int rate = stats->rate_idx;
 	u64 tsf = stats->mactime;
-	__le16 phy_flags_hw = rx_start->phy_flags;
+	__le16 phy_flags_hw = rx_start->phy_flags, antenna;
 	struct iwl4965_rt_rx_hdr {
 		struct ieee80211_radiotap_header rt_hdr;
 		__le64 rt_tsf;		/* TSF */
@@ -3381,8 +3381,8 @@ static void iwl4965_add_radiotap(struct 
 	 * new 802.11n radiotap field "RX chains" that is defined
 	 * as a bitmask.
 	 */
-	iwl4965_rt->rt_antenna =
-		le16_to_cpu(phy_flags_hw & RX_RES_PHY_FLAGS_ANTENNA_MSK) >> 4;
+	antenna = phy_flags_hw & RX_RES_PHY_FLAGS_ANTENNA_MSK;
+	iwl4965_rt->rt_antenna = le16_to_cpu(antenna) >> 4;
 
 	/* set the preamble flag if appropriate */
 	if (phy_flags_hw & RX_RES_PHY_FLAGS_SHORT_PREAMBLE_MSK)
@@ -3972,7 +3972,7 @@ static void iwl4965_rx_reply_rx(struct i
 
 	IWL_DEBUG_STATS_LIMIT("Rssi %d, noise %d, qual %d, TSF %llu\n",
 			      rx_status.ssi, rx_status.noise, rx_status.signal,
-			      rx_status.mactime);
+			      (unsigned long long)rx_status.mactime);
 
 	network_packet = iwl4965_is_network_packet(priv, header);
 	if (network_packet) {
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965.h	2008-04-01 17:36:10.893796252 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965.h	2008-04-01 17:44:02.702794566 +0200
@@ -293,8 +293,8 @@ struct iwl4965_frame {
 
 #define SEQ_TO_QUEUE(x)  ((x >> 8) & 0xbf)
 #define QUEUE_TO_SEQ(x)  ((x & 0xbf) << 8)
-#define SEQ_TO_INDEX(x) (x & 0xff)
-#define INDEX_TO_SEQ(x) (x & 0xff)
+#define SEQ_TO_INDEX(x) ((u8)(x & 0xff))
+#define INDEX_TO_SEQ(x) ((u8)(x & 0xff))
 #define SEQ_HUGE_FRAME  (0x4000)
 #define SEQ_RX_FRAME    __constant_cpu_to_le16(0x8000)
 #define SEQ_TO_SN(seq) (((seq) & IEEE80211_SCTL_SEQ) >> 4)
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-3945.c	2008-04-01 17:42:39.875671121 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-3945.c	2008-04-01 17:42:55.953764661 +0200
@@ -524,7 +524,7 @@ static void iwl3945_add_radiotap(struct 
 	s8 noise = 0;
 	int rate = stats->rate_idx;
 	u64 tsf = stats->mactime;
-	__le16 phy_flags_hw = rx_hdr->phy_flags;
+	__le16 phy_flags_hw = rx_hdr->phy_flags, antenna;
 
 	struct iwl3945_rt_rx_hdr {
 		struct ieee80211_radiotap_header rt_hdr;
@@ -596,8 +596,8 @@ static void iwl3945_add_radiotap(struct 
 		iwl3945_rt->rt_rate = iwl3945_rates[rate].ieee;
 
 	/* antenna number */
-	iwl3945_rt->rt_antenna =
-		le16_to_cpu(phy_flags_hw & RX_RES_PHY_FLAGS_ANTENNA_MSK) >> 4;
+	antenna = phy_flags_hw & RX_RES_PHY_FLAGS_ANTENNA_MSK;
+	iwl3945_rt->rt_antenna = le16_to_cpu(antenna) >> 4;
 
 	/* set the preamble flag if we have it */
 	if (phy_flags_hw & RX_RES_PHY_FLAGS_SHORT_PREAMBLE_MSK)
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-3945.h	2008-04-01 17:42:17.083795908 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-3945.h	2008-04-01 17:44:02.701795986 +0200
@@ -280,8 +280,8 @@ struct iwl3945_frame {
 
 #define SEQ_TO_QUEUE(x)  ((x >> 8) & 0xbf)
 #define QUEUE_TO_SEQ(x)  ((x & 0xbf) << 8)
-#define SEQ_TO_INDEX(x) (x & 0xff)
-#define INDEX_TO_SEQ(x) (x & 0xff)
+#define SEQ_TO_INDEX(x) ((u8)(x & 0xff))
+#define INDEX_TO_SEQ(x) ((u8)(x & 0xff))
 #define SEQ_HUGE_FRAME  (0x4000)
 #define SEQ_RX_FRAME    __constant_cpu_to_le16(0x8000)
 #define SEQ_TO_SN(seq) (((seq) & IEEE80211_SCTL_SEQ) >> 4)
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-debugfs.c	2008-04-01 17:43:44.900670090 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-debugfs.c	2008-04-01 17:43:55.875048052 +0200
@@ -243,7 +243,7 @@ static ssize_t iwl_dbgfs_stations_read(s
 						station->tid[j].agg.wait_for_ba);
 				pos += sprintf(buf+pos, "%u\t%llu\t%u\n",
 						station->tid[j].agg.start_idx,
-						station->tid[j].agg.bitmap,
+						(unsigned long long)station->tid[j].agg.bitmap,
 						station->tid[j].agg.rate_n_flags);
 			}
 			pos += sprintf(buf+pos, "\n");
--- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl4965-base.c	2008-04-01 17:43:25.884921531 +0200
+++ wireless-testing/drivers/net/wireless/iwlwifi/iwl4965-base.c	2008-04-01 17:43:33.955669481 +0200
@@ -3111,7 +3111,7 @@ static int iwl4965_tx_status_reply_tx(st
 		agg->rate_n_flags = le32_to_cpu(tx_resp->rate_n_flags);
 		IWL_DEBUG_TX_REPLY("Frames %d start_idx=%d bitmap=0x%llx\n",
 				   agg->frame_count, agg->start_idx,
-				   agg->bitmap);
+				   (unsigned long long)agg->bitmap);
 
 		if (bitmap)
 			agg->wait_for_ba = 1;

Re: "Try to stop Tx aggregation on non active TID" messages

[Tomas Winkler]

On Tue, Apr 1, 2008 at 6:51 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
>  > > [1] as, incidentally, you can see in the current wireless tree from a
>  > > few days ago, iwlwifi with debugging doesn't even compile!
>  >
>  > Could you please send the errors you see? The debug flags recently
>  > changed, so the following is needed:
>
>  It seems that this was before I updated to after the debug flags change,
>  everything seems fine now.
>
>  Unrelated, but while I was at it, I get a whole bunch of warnings that
>  are fixed by the patch below.
>
>  johannes
>
>  From: Johannes Berg <johannes@sipsolutions.net>
>  Subject: iwlwifi: fix warnings
>
>  This fixes all kinds of warnings in iwlwifi.
>
>  Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
>  ---
>   drivers/net/wireless/iwlwifi/iwl-3945.c     |    6 +++---
>   drivers/net/wireless/iwlwifi/iwl-3945.h     |    4 ++--
>   drivers/net/wireless/iwlwifi/iwl-4965.c     |    8 ++++----
>   drivers/net/wireless/iwlwifi/iwl-4965.h     |    4 ++--
>   drivers/net/wireless/iwlwifi/iwl-debugfs.c  |    2 +-
>   drivers/net/wireless/iwlwifi/iwl4965-base.c |    2 +-
>   6 files changed, 13 insertions(+), 13 deletions(-)
>
>  --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965.c       2008-04-01 17:38:21.311669543 +0200
>  +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965.c    2008-04-01 17:43:17.931669482 +0200
>  @@ -3297,7 +3297,7 @@ static void iwl4965_add_radiotap(struct
>         s8 noise = 0;
>         int rate = stats->rate_idx;
>         u64 tsf = stats->mactime;
>  -       __le16 phy_flags_hw = rx_start->phy_flags;
>  +       __le16 phy_flags_hw = rx_start->phy_flags, antenna;

Can you do this on separate lines

>         struct iwl4965_rt_rx_hdr {
>                 struct ieee80211_radiotap_header rt_hdr;
>                 __le64 rt_tsf;          /* TSF */
>  @@ -3381,8 +3381,8 @@ static void iwl4965_add_radiotap(struct
>          * new 802.11n radiotap field "RX chains" that is defined
>          * as a bitmask.
>          */
>  -       iwl4965_rt->rt_antenna =
>  -               le16_to_cpu(phy_flags_hw & RX_RES_PHY_FLAGS_ANTENNA_MSK) >> 4;
>  +       antenna = phy_flags_hw & RX_RES_PHY_FLAGS_ANTENNA_MSK;
>  +       iwl4965_rt->rt_antenna = le16_to_cpu(antenna) >> 4;
>
>         /* set the preamble flag if appropriate */
>         if (phy_flags_hw & RX_RES_PHY_FLAGS_SHORT_PREAMBLE_MSK)
>  @@ -3972,7 +3972,7 @@ static void iwl4965_rx_reply_rx(struct i
>
>         IWL_DEBUG_STATS_LIMIT("Rssi %d, noise %d, qual %d, TSF %llu\n",
>                               rx_status.ssi, rx_status.noise, rx_status.signal,
>  -                             rx_status.mactime);
>  +                             (unsigned long long)rx_status.mactime);
>
>         network_packet = iwl4965_is_network_packet(priv, header);
>         if (network_packet) {
>  --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-4965.h       2008-04-01 17:36:10.893796252 +0200
>  +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-4965.h    2008-04-01 17:44:02.702794566 +0200
>  @@ -293,8 +293,8 @@ struct iwl4965_frame {
>
>   #define SEQ_TO_QUEUE(x)  ((x >> 8) & 0xbf)
>   #define QUEUE_TO_SEQ(x)  ((x & 0xbf) << 8)
>  -#define SEQ_TO_INDEX(x) (x & 0xff)
>  -#define INDEX_TO_SEQ(x) (x & 0xff)
>  +#define SEQ_TO_INDEX(x) ((u8)(x & 0xff))
>  +#define INDEX_TO_SEQ(x) ((u8)(x & 0xff))
>   #define SEQ_HUGE_FRAME  (0x4000)
>   #define SEQ_RX_FRAME    __constant_cpu_to_le16(0x8000)
>   #define SEQ_TO_SN(seq) (((seq) & IEEE80211_SCTL_SEQ) >> 4)
>  --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-3945.c       2008-04-01 17:42:39.875671121 +0200
>  +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-3945.c    2008-04-01 17:42:55.953764661 +0200
>  @@ -524,7 +524,7 @@ static void iwl3945_add_radiotap(struct
>         s8 noise = 0;
>         int rate = stats->rate_idx;
>         u64 tsf = stats->mactime;
>  -       __le16 phy_flags_hw = rx_hdr->phy_flags;
>  +       __le16 phy_flags_hw = rx_hdr->phy_flags, antenna;
>
>         struct iwl3945_rt_rx_hdr {
>                 struct ieee80211_radiotap_header rt_hdr;
>  @@ -596,8 +596,8 @@ static void iwl3945_add_radiotap(struct
>                 iwl3945_rt->rt_rate = iwl3945_rates[rate].ieee;
>
>         /* antenna number */
>  -       iwl3945_rt->rt_antenna =
>  -               le16_to_cpu(phy_flags_hw & RX_RES_PHY_FLAGS_ANTENNA_MSK) >> 4;
>  +       antenna = phy_flags_hw & RX_RES_PHY_FLAGS_ANTENNA_MSK;
>  +       iwl3945_rt->rt_antenna = le16_to_cpu(antenna) >> 4;
>
>         /* set the preamble flag if we have it */
>         if (phy_flags_hw & RX_RES_PHY_FLAGS_SHORT_PREAMBLE_MSK)
>  --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-3945.h       2008-04-01 17:42:17.083795908 +0200
>  +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-3945.h    2008-04-01 17:44:02.701795986 +0200
>  @@ -280,8 +280,8 @@ struct iwl3945_frame {
>
>   #define SEQ_TO_QUEUE(x)  ((x >> 8) & 0xbf)
>   #define QUEUE_TO_SEQ(x)  ((x & 0xbf) << 8)
>  -#define SEQ_TO_INDEX(x) (x & 0xff)
>  -#define INDEX_TO_SEQ(x) (x & 0xff)
>  +#define SEQ_TO_INDEX(x) ((u8)(x & 0xff))
>  +#define INDEX_TO_SEQ(x) ((u8)(x & 0xff))
>   #define SEQ_HUGE_FRAME  (0x4000)
>   #define SEQ_RX_FRAME    __constant_cpu_to_le16(0x8000)
>   #define SEQ_TO_SN(seq) (((seq) & IEEE80211_SCTL_SEQ) >> 4)
>  --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl-debugfs.c    2008-04-01 17:43:44.900670090 +0200
>  +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl-debugfs.c 2008-04-01 17:43:55.875048052 +0200
>  @@ -243,7 +243,7 @@ static ssize_t iwl_dbgfs_stations_read(s
>                                                 station->tid[j].agg.wait_for_ba);
>                                 pos += sprintf(buf+pos, "%u\t%llu\t%u\n",
>                                                 station->tid[j].agg.start_idx,
>  -                                               station->tid[j].agg.bitmap,
>  +                                               (unsigned long long)station->tid[j].agg.bitmap,
>                                                 station->tid[j].agg.rate_n_flags);



>                         }
>                         pos += sprintf(buf+pos, "\n");
>  --- wireless-testing.orig/drivers/net/wireless/iwlwifi/iwl4965-base.c   2008-04-01 17:43:25.884921531 +0200
>  +++ wireless-testing/drivers/net/wireless/iwlwifi/iwl4965-base.c        2008-04-01 17:43:33.955669481 +0200
>  @@ -3111,7 +3111,7 @@ static int iwl4965_tx_status_reply_tx(st
>                 agg->rate_n_flags = le32_to_cpu(tx_resp->rate_n_flags);
>                 IWL_DEBUG_TX_REPLY("Frames %d start_idx=%d bitmap=0x%llx\n",
>                                    agg->frame_count, agg->start_idx,
>  -                                  agg->bitmap);
>  +                                  (unsigned long long)agg->bitmap);
>
This was already submitted by John IIRC.


>                 if (bitmap)
>                         agg->wait_for_ba = 1;
>
>
>