Currently we didn't check if there are still pending frames
Signed-off-by: Christian Lamparter <[email protected]>
---
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;
}
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
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
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
On Tue, May 12, 2009 at 2:39 AM, Johannes Berg
<[email protected]> 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 %% http://www.bobcopeland.com
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 %% http://www.bobcopeland.com
On Tue, 2009-05-12 at 11:29 -0400, Bob Copeland wrote:
> On Tue, May 12, 2009 at 2:39 AM, Johannes Berg
> <[email protected]> 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
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
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