This patch addresses a long standing issue of the driver with the
mac80211 .flush() callback. Since implementing the .flush() callback
a number of issues have been fixed, but a WARN_ON_ONCE() was still
triggered because the timeout on the flush could still occur.
This patch changes the awkward design using msleep() into one using
a waitqueue. The waiting flush() context will kick the transmit dma
when it is idle and the timeout used waiting for the event is set
to 500 ms. Worst case there can be 64 frames outstanding for transmit
in the driver. At a rate of 1Mbps that would take 1.5 seconds assuming
MTU is 1500 bytes and ignoring retries. The WARN_ON_ONCE() is also
removed as this was put in to indicate the flush timeout as a reason
for the driver to stall. That was not happening since fixing endless
AMPDU retries with following upstream commit:
commit 85091fc0a75653e239dc8379658515e577544927
Author: Arend van Spriel <[email protected]>
Date: Thu Feb 23 18:38:22 2012 +0100
brcm80211: smac: fix endless retry of A-MPDU transmissions
bugzilla: 42840 <https://bugzilla.kernel.org/show_bug.cgi?id=42840>
bugzilla@redhat: <https://bugzilla.redhat.com/show_bug.cgi?id=799168>
bugzilla@redhat: <https://bugzilla.redhat.com/show_bug.cgi?id=787649>
Cc: Jonathan Nieder <[email protected]>
Cc: Stanislaw Gruszka <[email protected]>
Cc: Camaleón <[email protected]>
Cc: Milan Bouchet-Valat <[email protected]>
Cc: Seth Forshee <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Piotr Haber <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
Hi John,
Hope this one can still ride the 3.8 train. Spent a long time in the
last few weeks on the dreaded flush timeout warning. This patch was
submitted before but it has been reworked to fit with Seth's brcmsmac
changes. Also increased the timeout value to a more reasonable value
as theoretical limit is higher as indicated in the commit message.
Gr. AvS
---
.../net/wireless/brcm80211/brcmsmac/mac80211_if.c | 35 ++++++++++++--------
.../net/wireless/brcm80211/brcmsmac/mac80211_if.h | 3 +-
drivers/net/wireless/brcm80211/brcmsmac/main.c | 15 ++-------
drivers/net/wireless/brcm80211/brcmsmac/pub.h | 3 +-
4 files changed, 27 insertions(+), 29 deletions(-)
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
index 0f71d1d..e5fd209 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
@@ -36,6 +36,7 @@
#include "debug.h"
#define N_TX_QUEUES 4 /* #tx queues on mac80211<->driver interface */
+#define BRCMS_FLUSH_TIMEOUT 500 /* msec */
/* Flags we support */
#define MAC_FILTERS (FIF_PROMISC_IN_BSS | \
@@ -708,16 +709,29 @@ static void brcms_ops_rfkill_poll(struct ieee80211_hw *hw)
wiphy_rfkill_set_hw_state(wl->pub->ieee_hw->wiphy, blocked);
}
+static bool brcms_tx_flush_completed(struct brcms_info *wl)
+{
+ bool result;
+
+ spin_lock_bh(&wl->lock);
+ result = brcms_c_tx_flush_completed(wl->wlc);
+ spin_unlock_bh(&wl->lock);
+ return result;
+}
+
static void brcms_ops_flush(struct ieee80211_hw *hw, bool drop)
{
struct brcms_info *wl = hw->priv;
+ int ret;
no_printk("%s: drop = %s\n", __func__, drop ? "true" : "false");
- /* wait for packet queue and dma fifos to run empty */
- spin_lock_bh(&wl->lock);
- brcms_c_wait_for_tx_completion(wl->wlc, drop);
- spin_unlock_bh(&wl->lock);
+ ret = wait_event_timeout(wl->tx_flush_wq,
+ brcms_tx_flush_completed(wl),
+ msecs_to_jiffies(BRCMS_FLUSH_TIMEOUT));
+
+ brcms_dbg_mac80211(wl->wlc->hw->d11core,
+ "ret=%d\n", jiffies_to_msecs(ret));
}
static const struct ieee80211_ops brcms_ops = {
@@ -772,6 +786,7 @@ void brcms_dpc(unsigned long data)
done:
spin_unlock_bh(&wl->lock);
+ wake_up(&wl->tx_flush_wq);
}
/*
@@ -1020,6 +1035,8 @@ static struct brcms_info *brcms_attach(struct bcma_device *pdev)
atomic_set(&wl->callbacks, 0);
+ init_waitqueue_head(&wl->tx_flush_wq);
+
/* setup the bottom half handler */
tasklet_init(&wl->tasklet, brcms_dpc, (unsigned long) wl);
@@ -1609,13 +1626,3 @@ bool brcms_rfkill_set_hw_state(struct brcms_info *wl)
spin_lock_bh(&wl->lock);
return blocked;
}
-
-/*
- * precondition: perimeter lock has been acquired
- */
-void brcms_msleep(struct brcms_info *wl, uint ms)
-{
- spin_unlock_bh(&wl->lock);
- msleep(ms);
- spin_lock_bh(&wl->lock);
-}
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.h b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.h
index 9358bd5..947ccac 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.h
+++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.h
@@ -68,6 +68,8 @@ struct brcms_info {
spinlock_t lock; /* per-device perimeter lock */
spinlock_t isr_lock; /* per-device ISR synchronization lock */
+ /* tx flush */
+ wait_queue_head_t tx_flush_wq;
/* timer related fields */
atomic_t callbacks; /* # outstanding callback functions */
@@ -100,7 +102,6 @@ extern struct brcms_timer *brcms_init_timer(struct brcms_info *wl,
extern void brcms_free_timer(struct brcms_timer *timer);
extern void brcms_add_timer(struct brcms_timer *timer, uint ms, int periodic);
extern bool brcms_del_timer(struct brcms_timer *timer);
-extern void brcms_msleep(struct brcms_info *wl, uint ms);
extern void brcms_dpc(unsigned long data);
extern void brcms_timer(struct brcms_timer *t);
extern void brcms_fatal_error(struct brcms_info *wl);
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
index 9f3d7e9..8b58390 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
@@ -7511,25 +7511,16 @@ int brcms_c_get_curband(struct brcms_c_info *wlc)
return wlc->band->bandunit;
}
-void brcms_c_wait_for_tx_completion(struct brcms_c_info *wlc, bool drop)
+bool brcms_c_tx_flush_completed(struct brcms_c_info *wlc)
{
- int timeout = 20;
int i;
/* Kick DMA to send any pending AMPDU */
for (i = 0; i < ARRAY_SIZE(wlc->hw->di); i++)
if (wlc->hw->di[i])
- dma_txflush(wlc->hw->di[i]);
+ dma_kick_tx(wlc->hw->di[i]);
- /* wait for queue and DMA fifos to run dry */
- while (brcms_txpktpendtot(wlc) > 0) {
- brcms_msleep(wlc->wl, 1);
-
- if (--timeout == 0)
- break;
- }
-
- WARN_ON_ONCE(timeout == 0);
+ return !brcms_txpktpendtot(wlc);
}
void brcms_c_set_beacon_listen_interval(struct brcms_c_info *wlc, u8 interval)
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/pub.h b/drivers/net/wireless/brcm80211/brcmsmac/pub.h
index 4fb2834..b0f14b7 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/pub.h
+++ b/drivers/net/wireless/brcm80211/brcmsmac/pub.h
@@ -314,8 +314,6 @@ extern void brcms_c_associate_upd(struct brcms_c_info *wlc, bool state);
extern void brcms_c_scan_start(struct brcms_c_info *wlc);
extern void brcms_c_scan_stop(struct brcms_c_info *wlc);
extern int brcms_c_get_curband(struct brcms_c_info *wlc);
-extern void brcms_c_wait_for_tx_completion(struct brcms_c_info *wlc,
- bool drop);
extern int brcms_c_set_channel(struct brcms_c_info *wlc, u16 channel);
extern int brcms_c_set_rate_limit(struct brcms_c_info *wlc, u16 srl, u16 lrl);
extern void brcms_c_get_current_rateset(struct brcms_c_info *wlc,
@@ -332,5 +330,6 @@ extern int brcms_c_set_tx_power(struct brcms_c_info *wlc, int txpwr);
extern int brcms_c_get_tx_power(struct brcms_c_info *wlc);
extern bool brcms_c_check_radio_disabled(struct brcms_c_info *wlc);
extern void brcms_c_mute(struct brcms_c_info *wlc, bool on);
+extern bool brcms_c_tx_flush_completed(struct brcms_c_info *wlc);
#endif /* _BRCM_PUB_H_ */
--
1.7.10.4
On Sat, Feb 02, 2013 at 02:36:50PM +0100, Arend van Spriel wrote:
> This patch addresses a long standing issue of the driver with the
> mac80211 .flush() callback. Since implementing the .flush() callback
> a number of issues have been fixed, but a WARN_ON_ONCE() was still
> triggered because the timeout on the flush could still occur.
>
> This patch changes the awkward design using msleep() into one using
> a waitqueue. The waiting flush() context will kick the transmit dma
> when it is idle and the timeout used waiting for the event is set
> to 500 ms. Worst case there can be 64 frames outstanding for transmit
> in the driver. At a rate of 1Mbps that would take 1.5 seconds assuming
> MTU is 1500 bytes and ignoring retries. The WARN_ON_ONCE() is also
> removed as this was put in to indicate the flush timeout as a reason
> for the driver to stall. That was not happening since fixing endless
> AMPDU retries with following upstream commit:
>
> commit 85091fc0a75653e239dc8379658515e577544927
> Author: Arend van Spriel <[email protected]>
> Date: Thu Feb 23 18:38:22 2012 +0100
>
> brcm80211: smac: fix endless retry of A-MPDU transmissions
>
> bugzilla: 42840 <https://bugzilla.kernel.org/show_bug.cgi?id=42840>
> bugzilla@redhat: <https://bugzilla.redhat.com/show_bug.cgi?id=799168>
> bugzilla@redhat: <https://bugzilla.redhat.com/show_bug.cgi?id=787649>
>
> Cc: Jonathan Nieder <[email protected]>
> Cc: Stanislaw Gruszka <[email protected]>
> Cc: Camaleón <[email protected]>
> Cc: Milan Bouchet-Valat <[email protected]>
> Cc: Seth Forshee <[email protected]>
> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
> Reviewed-by: Hante Meuleman <[email protected]>
> Reviewed-by: Piotr Haber <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
I think this makes sense and is an improvement to how the flush works
now. In the past week I've become pretty well convinced that all
instances of the flush timeout warning that I've been seeing are
actually due to brcmsmac continuing to get frames for tx rather than any
kind of problem with actually transmitting the frames, so demoting this
from a WARN_ON to a debug statement makes sense.
Acked-by: Seth Forshee <[email protected]>
I do have one minor comment below.
> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> index 9f3d7e9..8b58390 100644
> --- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
> +++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> @@ -7511,25 +7511,16 @@ int brcms_c_get_curband(struct brcms_c_info *wlc)
> return wlc->band->bandunit;
> }
>
> -void brcms_c_wait_for_tx_completion(struct brcms_c_info *wlc, bool drop)
> +bool brcms_c_tx_flush_completed(struct brcms_c_info *wlc)
> {
> - int timeout = 20;
> int i;
>
> /* Kick DMA to send any pending AMPDU */
> for (i = 0; i < ARRAY_SIZE(wlc->hw->di); i++)
> if (wlc->hw->di[i])
> - dma_txflush(wlc->hw->di[i]);
> + dma_kick_tx(wlc->hw->di[i]);
While it won't hurt to call dma_kick_tx() here, it really shouldn't be
necessary. It already gets called anytime brcmsmac handles a legit
txstatus interrupt, so I suspect calling it here will has effect.
The call to dma_txflush() was intended to move queued A-MPDU frames into
the DMA fifos at the start of the flush. This is no longer happening,
but if the driver continues getting frames for tx while the flush is in
progress then the flush doesn't really help. Removing the call renders
dma_txflush() unused though, so it could be removed entirely.
Cheers,
Seth
On Mon, Feb 04, 2013 at 09:37:12AM -0600, Seth Forshee wrote:
> On Sat, Feb 02, 2013 at 02:36:50PM +0100, Arend van Spriel wrote:
> > This patch addresses a long standing issue of the driver with the
> > mac80211 .flush() callback. Since implementing the .flush() callback
> > a number of issues have been fixed, but a WARN_ON_ONCE() was still
> > triggered because the timeout on the flush could still occur.
> >
> > This patch changes the awkward design using msleep() into one using
> > a waitqueue. The waiting flush() context will kick the transmit dma
> > when it is idle and the timeout used waiting for the event is set
> > to 500 ms. Worst case there can be 64 frames outstanding for transmit
> > in the driver. At a rate of 1Mbps that would take 1.5 seconds assuming
> > MTU is 1500 bytes and ignoring retries. The WARN_ON_ONCE() is also
> > removed as this was put in to indicate the flush timeout as a reason
> > for the driver to stall. That was not happening since fixing endless
> > AMPDU retries with following upstream commit:
> >
> > commit 85091fc0a75653e239dc8379658515e577544927
> > Author: Arend van Spriel <[email protected]>
> > Date: Thu Feb 23 18:38:22 2012 +0100
> >
> > brcm80211: smac: fix endless retry of A-MPDU transmissions
> >
> > bugzilla: 42840 <https://bugzilla.kernel.org/show_bug.cgi?id=42840>
> > bugzilla@redhat: <https://bugzilla.redhat.com/show_bug.cgi?id=799168>
> > bugzilla@redhat: <https://bugzilla.redhat.com/show_bug.cgi?id=787649>
> >
> > Cc: Jonathan Nieder <[email protected]>
> > Cc: Stanislaw Gruszka <[email protected]>
> > Cc: Camaleón <[email protected]>
> > Cc: Milan Bouchet-Valat <[email protected]>
> > Cc: Seth Forshee <[email protected]>
> > Reviewed-by: Pieter-Paul Giesberts <[email protected]>
> > Reviewed-by: Hante Meuleman <[email protected]>
> > Reviewed-by: Piotr Haber <[email protected]>
> > Signed-off-by: Arend van Spriel <[email protected]>
>
> I think this makes sense and is an improvement to how the flush works
> now. In the past week I've become pretty well convinced that all
> instances of the flush timeout warning that I've been seeing are
> actually due to brcmsmac continuing to get frames for tx rather than any
> kind of problem with actually transmitting the frames, so demoting this
> from a WARN_ON to a debug statement makes sense.
>
> Acked-by: Seth Forshee <[email protected]>
>
> I do have one minor comment below.
>
> > diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> > index 9f3d7e9..8b58390 100644
> > --- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
> > +++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
> > @@ -7511,25 +7511,16 @@ int brcms_c_get_curband(struct brcms_c_info *wlc)
> > return wlc->band->bandunit;
> > }
> >
> > -void brcms_c_wait_for_tx_completion(struct brcms_c_info *wlc, bool drop)
> > +bool brcms_c_tx_flush_completed(struct brcms_c_info *wlc)
> > {
> > - int timeout = 20;
> > int i;
> >
> > /* Kick DMA to send any pending AMPDU */
> > for (i = 0; i < ARRAY_SIZE(wlc->hw->di); i++)
> > if (wlc->hw->di[i])
> > - dma_txflush(wlc->hw->di[i]);
> > + dma_kick_tx(wlc->hw->di[i]);
>
> While it won't hurt to call dma_kick_tx() here, it really shouldn't be
> necessary. It already gets called anytime brcmsmac handles a legit
> txstatus interrupt, so I suspect calling it here will has effect.
>
> The call to dma_txflush() was intended to move queued A-MPDU frames into
> the DMA fifos at the start of the flush. This is no longer happening,
> but if the driver continues getting frames for tx while the flush is in
> progress then the flush doesn't really help. Removing the call renders
Sorry, confusing wording here. I mean that calling dma_txflush() doesn't
help if the driver continues receiving frames during a flush.
> dma_txflush() unused though, so it could be removed entirely.
>
> Cheers,
> Seth
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html