[PATCH 6/6] ar9170: stop data queues on channel switch

[PATCH 6/6] ar9170: stop data queues on channel switch

[Christian Lamparter]

Currently we didn't check if there are still pending frames

Signed-off-by: Christian Lamparter <chunkeey@web.de>
---
diff --git a/drivers/net/wireless/ath/ar9170/main.c b/drivers/net/wireless/ath/ar9170/main.c
index a66aa7d..8c8f45b 100644
--- a/drivers/net/wireless/ath/ar9170/main.c
+++ b/drivers/net/wireless/ath/ar9170/main.c
@@ -1299,6 +1299,37 @@ static void ar9170_op_remove_interface(struct ieee80211_hw *hw,
 	mutex_unlock(&ar->mutex);
 }
 
+static int ar9170_stop_and_flush_queues(struct ar9170 *ar)
+{
+	int err;
+
+	ieee80211_stop_queues(ar->hw);
+
+	err = ar->flush(ar);
+	if (err) {
+		printk(KERN_ERR "%s: device is not responding (%d)!\n",
+		       wiphy_name(ar->hw->wiphy), err);
+
+		/* FIXME: purge tx_status queues and reset */
+	}
+
+	return err;
+}
+
+static void ar9170_wake_queues(struct ar9170 *ar)
+{
+	unsigned long flags;
+	unsigned int i;
+
+	spin_lock_irqsave(&ar->tx_stats_lock, flags);
+	for (i = 0; i < __AR9170_NUM_TXQ; i++) {
+		if ((ar->tx_stats[i].len < ar->tx_stats[i].limit) &&
+		    (ieee80211_queue_stopped(ar->hw, i)))
+			ieee80211_wake_queue(ar->hw, i);
+	}
+	spin_unlock_irqrestore(&ar->tx_stats_lock, flags);
+}
+
 static int ar9170_op_config(struct ieee80211_hw *hw, u32 changed)
 {
 	struct ar9170 *ar = hw->priv;
@@ -1344,6 +1375,9 @@ static int ar9170_op_config(struct ieee80211_hw *hw, u32 changed)
 	}
 
 	if (changed & IEEE80211_CONF_CHANGE_CHANNEL) {
+		err = ar9170_stop_and_flush_queues(ar);
+		if (err)
+			goto out;
 
 		/* adjust slot time for 5 GHz */
 		err = ar9170_set_slot_time(ar);
@@ -1359,10 +1393,12 @@ static int ar9170_op_config(struct ieee80211_hw *hw, u32 changed)
 				nl80211_to_ar9170(hw->conf.channel_type));
 		if (err)
 			goto out;
+
 	}
 
 out:
 	mutex_unlock(&ar->mutex);
+	ar9170_wake_queues(ar);
 	return err;
 }
 

Re: [PATCH 6/6] ar9170: stop data queues on channel switch

[Johannes Berg]

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

On Tue, 2009-05-12 at 01:09 +0200, Christian Lamparter wrote:

> +		if ((ar->tx_stats[i].len < ar->tx_stats[i].limit) &&
> +		    (ieee80211_queue_stopped(ar->hw, i)))
> +			ieee80211_wake_queue(ar->hw, i);

There's no reason to test _stopped first, and doing so is racy.

>  	if (changed & IEEE80211_CONF_CHANGE_CHANNEL) {
> +		err = ar9170_stop_and_flush_queues(ar);
> +		if (err)
> +			goto out;

Why don't you do that in the pre-scan method?

johannes

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

Re: [PATCH 6/6] ar9170: stop data queues on channel switch

[Christian Lamparter]

On Tuesday 12 May 2009 08:39:49 Johannes Berg wrote:
> On Tue, 2009-05-12 at 01:09 +0200, Christian Lamparter wrote:
> 
> > +		if ((ar->tx_stats[i].len < ar->tx_stats[i].limit) &&
> > +		    (ieee80211_queue_stopped(ar->hw, i)))
> > +			ieee80211_wake_queue(ar->hw, i);
> 
> There's no reason to test _stopped first, and doing so is racy.
uhh, agreed _stopped is not necessary, but where's the race condition?

> 
> >  	if (changed & IEEE80211_CONF_CHANGE_CHANNEL) {
> > +		err = ar9170_stop_and_flush_queues(ar);
> > +		if (err)
> > +			goto out;
> 
> Why don't you do that in the pre-scan method?
dunno, will pre_scan cb code end up in 2.6.30 as well?

Regards,
	Chr 

Re: [PATCH 6/6] ar9170: stop data queues on channel switch

[Johannes Berg]

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

On Tue, 2009-05-12 at 17:15 +0200, Christian Lamparter wrote:
> On Tuesday 12 May 2009 08:39:49 Johannes Berg wrote:
> > On Tue, 2009-05-12 at 01:09 +0200, Christian Lamparter wrote:
> > 
> > > +		if ((ar->tx_stats[i].len < ar->tx_stats[i].limit) &&
> > > +		    (ieee80211_queue_stopped(ar->hw, i)))
> > > +			ieee80211_wake_queue(ar->hw, i);
> > 
> > There's no reason to test _stopped first, and doing so is racy.
> uhh, agreed _stopped is not necessary, but where's the race condition?

Well if _stopped returns false but you were just about to stop you're
now racing against whoever is stopping.

johannes

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

Re: [PATCH 6/6] ar9170: stop data queues on channel switch

[Bob Copeland]

On Tue, May 12, 2009 at 2:39 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>> =A0 =A0 =A0 if (changed & IEEE80211_CONF_CHANGE_CHANNEL) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D ar9170_stop_and_flush_queues(ar);
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (err)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
>
> Why don't you do that in the pre-scan method?

Will pre-scan be called once per scan or once for every channel switch?
If the former, I think it's not the right place to flush.

--=20
Bob Copeland %% www.bobcopeland.com
--
To unsubscribe from this list: send the line "unsubscribe linux-wireles=
s" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] ar9170: stop data queues on channel switch

[Johannes Berg]

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

On Tue, 2009-05-12 at 11:29 -0400, Bob Copeland wrote:
> On Tue, May 12, 2009 at 2:39 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> >>       if (changed & IEEE80211_CONF_CHANGE_CHANNEL) {
> >> +             err = ar9170_stop_and_flush_queues(ar);
> >> +             if (err)
> >> +                     goto out;
> >
> > Why don't you do that in the pre-scan method?
> 
> Will pre-scan be called once per scan or once for every channel switch?
> If the former, I think it's not the right place to flush.

The former.

johannes

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

Re: [PATCH 6/6] ar9170: stop data queues on channel switch

[Bob Copeland]

On Tue, 12 May 2009 17:30:27 +0200, Johannes Berg wrote
> On Tue, 2009-05-12 at 11:29 -0400, Bob Copeland wrote:
> > Will pre-scan be called once per scan or once for every channel switch?
> > If the former, I think it's not the right place to flush.
> 
> The former.

Hmm, nevermind, it should work.  I was thinking of queued tx status 
work not being on the right channel, but I guess we don't do that for probe
requests.  And for ar9170 RX is processed synchronously as far as I can
tell.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH 6/6] ar9170: stop data queues on channel switch

[Christian Lamparter]

On Tuesday 12 May 2009 17:56:10 Bob Copeland wrote:
> On Tue, 12 May 2009 17:30:27 +0200, Johannes Berg wrote
> > On Tue, 2009-05-12 at 11:29 -0400, Bob Copeland wrote:
> > > Will pre-scan be called once per scan or once for every channel switch?
> > > If the former, I think it's not the right place to flush.
> > 
> > The former.
> 
> Hmm, nevermind, it should work.  I was thinking of queued tx status 
> work not being on the right channel, but I guess we don't do that for probe
> requests.
Ermm? wait... what do we do when the userspace does channel switch & scans?
I think, some aircrack tools actually do this. Will they also call pre_scan?

> And for ar9170 RX is processed synchronously as far as I can tell.
most of the time yes... unless we have to _reconstruct_ a stream, because
the hw/fw clipped a few byte from a frame.

Regards,
	Chr

Re: [PATCH 6/6] ar9170: stop data queues on channel switch

[Johannes Berg]

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

On Tue, 2009-05-12 at 18:56 +0200, Christian Lamparter wrote:
> On Tuesday 12 May 2009 17:56:10 Bob Copeland wrote:
> > On Tue, 12 May 2009 17:30:27 +0200, Johannes Berg wrote
> > > On Tue, 2009-05-12 at 11:29 -0400, Bob Copeland wrote:
> > > > Will pre-scan be called once per scan or once for every channel switch?
> > > > If the former, I think it's not the right place to flush.
> > > 
> > > The former.
> > 
> > Hmm, nevermind, it should work.  I was thinking of queued tx status 
> > work not being on the right channel, but I guess we don't do that for probe
> > requests.
> Ermm? wait... what do we do when the userspace does channel switch & scans?
> I think, some aircrack tools actually do this. Will they also call pre_scan?

No, of course not, but there's no expectation that TX is somehow
synchronised with channel changes.

johannes

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