Hi all,
The beacon processing in ath9k is done in a tasklet. This tasklet may race
against beacon allocation/deallocation in process context. The patch below
is an attempt to point out / avoid these race conditions. My hope is that
this will stabilize ath9k in a use-case I'm interested in where ap vif
interfaces are added and removed quite often.
This is purely an RFC, and it's probably synchronizing a bit too much. I'm
currently testing this patch with no obvious problems so far (except for
the part in xmit.c which I've commented out). More testing is necessary
but so is a rewrite of the patch anyway.
Any thoughts?
Best regards,
Bj?rn
---
diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
index 19891e7..f91e0b5 100644
--- a/drivers/net/wireless/ath/ath9k/beacon.c
+++ b/drivers/net/wireless/ath/ath9k/beacon.c
@@ -192,6 +192,7 @@ static struct ath_buf *ath_beacon_generate(struct ieee80211_hw *hw,
if (sc->nvifs > 1) {
ath_print(common, ATH_DBG_BEACON,
"Flushing previous cabq traffic\n");
+ ath9k_hw_stoptxdma(sc->sc_ah, cabq->axq_qnum);
ath_draintxq(sc, cabq, false);
}
}
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index b52f1cf..c3d2a36 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -290,9 +290,11 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
ath9k_hw_set_interrupts(ah, ah->imask);
if (!(sc->sc_flags & (SC_OP_OFFCHANNEL))) {
+ tasklet_disable(&sc->bcon_tasklet);
ath_beacon_config(sc, NULL);
ieee80211_queue_delayed_work(sc->hw, &sc->tx_complete_work, 0);
ath_start_ani(common);
+ tasklet_enable(&sc->bcon_tasklet);
}
ps_restore:
@@ -838,6 +840,8 @@ static void ath9k_bss_assoc_info(struct ath_softc *sc,
struct ath_common *common = ath9k_hw_common(ah);
if (bss_conf->assoc) {
+ tasklet_disable(&sc->bcon_tasklet);
+
ath_print(common, ATH_DBG_CONFIG,
"Bss Info ASSOC %d, bssid: %pM\n",
bss_conf->aid, common->curbssid);
@@ -861,6 +865,8 @@ static void ath9k_bss_assoc_info(struct ath_softc *sc,
sc->sc_flags |= SC_OP_ANI_RUN;
ath_start_ani(common);
+
+ tasklet_enable(&sc->bcon_tasklet);
} else {
ath_print(common, ATH_DBG_CONFIG, "Bss Info DISASSOC\n");
common->curaid = 0;
@@ -903,8 +909,11 @@ void ath_radio_enable(struct ath_softc *sc, struct ieee80211_hw *hw)
}
spin_unlock_bh(&sc->rx.pcu_lock);
- if (sc->sc_flags & SC_OP_BEACONS)
+ if (sc->sc_flags & SC_OP_BEACONS) {
+ tasklet_disable(&sc->bcon_tasklet);
ath_beacon_config(sc, NULL); /* restart beacons */
+ tasklet_enable(&sc->bcon_tasklet);
+ }
/* Re-Enable interrupts */
ath9k_hw_set_interrupts(ah, ah->imask);
@@ -1008,8 +1017,11 @@ int ath_reset(struct ath_softc *sc, bool retry_tx)
*/
ath_update_txpow(sc);
- if ((sc->sc_flags & SC_OP_BEACONS) || !(sc->sc_flags & (SC_OP_OFFCHANNEL)))
+ if ((sc->sc_flags & SC_OP_BEACONS) || !(sc->sc_flags & (SC_OP_OFFCHANNEL))) {
+ tasklet_disable(&sc->bcon_tasklet);
ath_beacon_config(sc, NULL); /* restart beacons */
+ tasklet_enable(&sc->bcon_tasklet);
+ }
ath9k_hw_set_interrupts(ah, ah->imask);
@@ -1517,6 +1529,8 @@ static void ath9k_remove_interface(struct ieee80211_hw *hw,
mutex_lock(&sc->mutex);
+ tasklet_disable(&sc->bcon_tasklet);
+
/* Stop ANI */
sc->sc_flags &= ~SC_OP_ANI_RUN;
del_timer_sync(&common->ani.timer);
@@ -1544,6 +1558,8 @@ static void ath9k_remove_interface(struct ieee80211_hw *hw,
sc->nvifs--;
+ tasklet_enable(&sc->bcon_tasklet);
+
mutex_unlock(&sc->mutex);
}
@@ -1817,6 +1833,8 @@ static int ath9k_conf_tx(struct ieee80211_hw *hw, u16 queue,
mutex_lock(&sc->mutex);
+ tasklet_disable(&sc->bcon_tasklet);
+
memset(&qi, 0, sizeof(struct ath9k_tx_queue_info));
qi.tqi_aifs = params->aifs;
@@ -1839,6 +1857,8 @@ static int ath9k_conf_tx(struct ieee80211_hw *hw, u16 queue,
if ((qnum == sc->tx.hwq_map[WME_AC_BE]) && !ret)
ath_beaconq_config(sc);
+ tasklet_enable(&sc->bcon_tasklet);
+
mutex_unlock(&sc->mutex);
return ret;
@@ -1930,13 +1950,16 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
/* Enable transmission of beacons (AP, IBSS, MESH) */
if ((changed & BSS_CHANGED_BEACON) ||
((changed & BSS_CHANGED_BEACON_ENABLED) && bss_conf->enable_beacon)) {
+ tasklet_disable(&sc->bcon_tasklet);
ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq);
error = ath_beacon_alloc(aphy, vif);
if (!error)
ath_beacon_config(sc, vif);
+ tasklet_enable(&sc->bcon_tasklet);
}
if (changed & BSS_CHANGED_ERP_SLOT) {
+ tasklet_disable(&sc->bcon_tasklet);
if (bss_conf->use_short_slot)
slottime = 9;
else
@@ -1953,6 +1976,7 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
ah->slottime = slottime;
ath9k_hw_init_global_settings(ah);
}
+ tasklet_enable(&sc->bcon_tasklet);
}
/* Disable transmission of beacons */
@@ -1960,6 +1984,7 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq);
if (changed & BSS_CHANGED_BEACON_INT) {
+ tasklet_disable(&sc->bcon_tasklet);
sc->beacon_interval = bss_conf->beacon_int;
/*
* In case of AP mode, the HW TSF has to be reset
@@ -1974,6 +1999,7 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
} else {
ath_beacon_config(sc, vif);
}
+ tasklet_enable(&sc->bcon_tasklet);
}
if (changed & BSS_CHANGED_ERP_PREAMBLE) {
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index f2ade24..174a8ae 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1133,6 +1133,10 @@ void ath_drain_all_txq(struct ath_softc *sc, bool retry_tx)
/* Stop beacon queue */
ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq);
+ /* Stop cab queue */
+ if (sc->beacon.cabq != NULL)
+ ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.cabq->axq_qnum);
+
/* Stop data queues */
for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) {
if (ATH_TXQ_SETUP(sc, i)) {
@@ -1157,6 +1161,11 @@ void ath_drain_all_txq(struct ath_softc *sc, bool retry_tx)
spin_unlock_bh(&sc->sc_resetlock);
}
+ /* Drain cab queue
+ * BUG: for some reason this causes a dump in ath_draintxq(). Why?
+ if (sc->beacon.cabq != NULL)
+ ath_draintxq(sc->sc_ah, sc->beacon.cabq, false); */
+
for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) {
if (ATH_TXQ_SETUP(sc, i))
ath_draintxq(sc, &sc->tx.txq[i], retry_tx);
2010/11/3 Bj?rn Smedman <[email protected]>
> Hi all,
>
> The beacon processing in ath9k is done in a tasklet. This tasklet may race
> against beacon allocation/deallocation in process context. The patch below
> is an attempt to point out / avoid these race conditions. My hope is that
> this will stabilize ath9k in a use-case I'm interested in where ap vif
> interfaces are added and removed quite often.
>
> This is purely an RFC, and it's probably synchronizing a bit too much. I'm
> currently testing this patch with no obvious problems so far (except for
> the part in xmit.c which I've commented out). More testing is necessary
> but so is a rewrite of the patch anyway.
>
> Any thoughts?
Anybody?
In principle at least, don't you agree it's a bad idea letting the
beacon tasklet race against the beacon allocation/deallocation?
Do you think it's a good idea to disable the tasklet, or should we add
some spin_lock_bh's instead?
Best regards,
Bj?rn
2010/11/8 Felix Fietkau <[email protected]>:
> On 2010-11-03 6:36 PM, Bj?rn Smedman wrote:
>> Any thoughts?
Thanx Felix! So if I understand your comments below you think it's a
reasonable approach to disable the beacon tasklet in some situations.
It shouldn't have any adverse effects, like missing beacons, right? If
I understand correctly the tasklet will run as soon as it is enabled
(if it has been scheduled through an interrupt).
>> ---
>> diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
>> index 19891e7..f91e0b5 100644
>> --- a/drivers/net/wireless/ath/ath9k/beacon.c
>> +++ b/drivers/net/wireless/ath/ath9k/beacon.c
>> @@ -192,6 +192,7 @@ static struct ath_buf *ath_beacon_generate(struct ieee80211_hw *hw,
>> ? ? ? ? ? ? ? if (sc->nvifs > 1) {
>> ? ? ? ? ? ? ? ? ? ? ? ath_print(common, ATH_DBG_BEACON,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "Flushing previous cabq traffic\n");
>> + ? ? ? ? ? ? ? ? ? ? ath9k_hw_stoptxdma(sc->sc_ah, cabq->axq_qnum);
>> ? ? ? ? ? ? ? ? ? ? ? ath_draintxq(sc, cabq, false);
>> ? ? ? ? ? ? ? }
>> ? ? ? }
> Makes sense
>
>> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
>> index b52f1cf..c3d2a36 100644
>> --- a/drivers/net/wireless/ath/ath9k/main.c
>> +++ b/drivers/net/wireless/ath/ath9k/main.c
>> @@ -290,9 +290,11 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
>> ? ? ? ath9k_hw_set_interrupts(ah, ah->imask);
>>
>> ? ? ? if (!(sc->sc_flags & (SC_OP_OFFCHANNEL))) {
>> + ? ? ? ? ? ? tasklet_disable(&sc->bcon_tasklet);
>> ? ? ? ? ? ? ? ath_beacon_config(sc, NULL);
>> ? ? ? ? ? ? ? ieee80211_queue_delayed_work(sc->hw, &sc->tx_complete_work, 0);
>> ? ? ? ? ? ? ? ath_start_ani(common);
>> + ? ? ? ? ? ? tasklet_enable(&sc->bcon_tasklet);
>> ? ? ? }
>>
>> ? ps_restore:
> Why?
I guess I was scared of what would happen in ath_beacon_config() and
how it might race with the beacon tasklet.
>
>> @@ -838,6 +840,8 @@ static void ath9k_bss_assoc_info(struct ath_softc *sc,
>> ? ? ? struct ath_common *common = ath9k_hw_common(ah);
>>
>> ? ? ? if (bss_conf->assoc) {
>> + ? ? ? ? ? ? tasklet_disable(&sc->bcon_tasklet);
>> +
>> ? ? ? ? ? ? ? ath_print(common, ATH_DBG_CONFIG,
>> ? ? ? ? ? ? ? ? ? ? ? ? "Bss Info ASSOC %d, bssid: %pM\n",
>> ? ? ? ? ? ? ? ? ? ? ? ? ?bss_conf->aid, common->curbssid);
>> @@ -861,6 +865,8 @@ static void ath9k_bss_assoc_info(struct ath_softc *sc,
>>
>> ? ? ? ? ? ? ? sc->sc_flags |= SC_OP_ANI_RUN;
>> ? ? ? ? ? ? ? ath_start_ani(common);
>> +
>> + ? ? ? ? ? ? tasklet_enable(&sc->bcon_tasklet);
>> ? ? ? } else {
>> ? ? ? ? ? ? ? ath_print(common, ATH_DBG_CONFIG, "Bss Info DISASSOC\n");
>> ? ? ? ? ? ? ? common->curaid = 0;
> Unnecessary, does not have anything to do with *sending* beacons.
Sounds reasonable.
>
>> @@ -903,8 +909,11 @@ void ath_radio_enable(struct ath_softc *sc, struct ieee80211_hw *hw)
>> ? ? ? }
>> ? ? ? spin_unlock_bh(&sc->rx.pcu_lock);
>>
>> - ? ? if (sc->sc_flags & SC_OP_BEACONS)
>> + ? ? if (sc->sc_flags & SC_OP_BEACONS) {
>> + ? ? ? ? ? ? tasklet_disable(&sc->bcon_tasklet);
>> ? ? ? ? ? ? ? ath_beacon_config(sc, NULL); ? ?/* restart beacons */
>> + ? ? ? ? ? ? tasklet_enable(&sc->bcon_tasklet);
>> + ? ? }
>>
>> ? ? ? /* Re-Enable ?interrupts */
>> ? ? ? ath9k_hw_set_interrupts(ah, ah->imask);
>> @@ -1008,8 +1017,11 @@ int ath_reset(struct ath_softc *sc, bool retry_tx)
>> ? ? ? ?*/
>> ? ? ? ath_update_txpow(sc);
>>
>> - ? ? if ((sc->sc_flags & SC_OP_BEACONS) || !(sc->sc_flags & (SC_OP_OFFCHANNEL)))
>> + ? ? if ((sc->sc_flags & SC_OP_BEACONS) || !(sc->sc_flags & (SC_OP_OFFCHANNEL))) {
>> + ? ? ? ? ? ? tasklet_disable(&sc->bcon_tasklet);
>> ? ? ? ? ? ? ? ath_beacon_config(sc, NULL); ? ?/* restart beacons */
>> + ? ? ? ? ? ? tasklet_enable(&sc->bcon_tasklet);
>> + ? ? }
>>
>> ? ? ? ath9k_hw_set_interrupts(ah, ah->imask);
>>
> Why?
Again, I guess I was scared of whatever may happen in
ath_beacon_config(). Is that safe to call "in parallel with" beacon
tasklet?
>
>> @@ -1517,6 +1529,8 @@ static void ath9k_remove_interface(struct ieee80211_hw *hw,
>>
>> ? ? ? mutex_lock(&sc->mutex);
>>
>> + ? ? tasklet_disable(&sc->bcon_tasklet);
>> +
>> ? ? ? /* Stop ANI */
>> ? ? ? sc->sc_flags &= ~SC_OP_ANI_RUN;
>> ? ? ? del_timer_sync(&common->ani.timer);
>> @@ -1544,6 +1558,8 @@ static void ath9k_remove_interface(struct ieee80211_hw *hw,
>>
>> ? ? ? sc->nvifs--;
>>
>> + ? ? tasklet_enable(&sc->bcon_tasklet);
>> +
>> ? ? ? mutex_unlock(&sc->mutex);
>> ?}
>>
> Makes sense.
>
>> @@ -1817,6 +1833,8 @@ static int ath9k_conf_tx(struct ieee80211_hw *hw, u16 queue,
>>
>> ? ? ? mutex_lock(&sc->mutex);
>>
>> + ? ? tasklet_disable(&sc->bcon_tasklet);
>> +
>> ? ? ? memset(&qi, 0, sizeof(struct ath9k_tx_queue_info));
>>
>> ? ? ? qi.tqi_aifs = params->aifs;
>> @@ -1839,6 +1857,8 @@ static int ath9k_conf_tx(struct ieee80211_hw *hw, u16 queue,
>> ? ? ? ? ? ? ? if ((qnum == sc->tx.hwq_map[WME_AC_BE]) && !ret)
>> ? ? ? ? ? ? ? ? ? ? ? ath_beaconq_config(sc);
>>
>> + ? ? tasklet_enable(&sc->bcon_tasklet);
>> +
>> ? ? ? mutex_unlock(&sc->mutex);
>>
>> ? ? ? return ret;
> I don't think that one's necessary.
>
>> @@ -1930,13 +1950,16 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
>> ? ? ? /* Enable transmission of beacons (AP, IBSS, MESH) */
>> ? ? ? if ((changed & BSS_CHANGED_BEACON) ||
>> ? ? ? ? ? ((changed & BSS_CHANGED_BEACON_ENABLED) && bss_conf->enable_beacon)) {
>> + ? ? ? ? ? ? tasklet_disable(&sc->bcon_tasklet);
>> ? ? ? ? ? ? ? ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq);
>> ? ? ? ? ? ? ? error = ath_beacon_alloc(aphy, vif);
>> ? ? ? ? ? ? ? if (!error)
>> ? ? ? ? ? ? ? ? ? ? ? ath_beacon_config(sc, vif);
>> + ? ? ? ? ? ? tasklet_enable(&sc->bcon_tasklet);
>> ? ? ? }
> Makes sense.
>
>> ? ? ? if (changed & BSS_CHANGED_ERP_SLOT) {
>> + ? ? ? ? ? ? tasklet_disable(&sc->bcon_tasklet);
>> ? ? ? ? ? ? ? if (bss_conf->use_short_slot)
>> ? ? ? ? ? ? ? ? ? ? ? slottime = 9;
>> ? ? ? ? ? ? ? else
>> @@ -1953,6 +1976,7 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
>> ? ? ? ? ? ? ? ? ? ? ? ah->slottime = slottime;
>> ? ? ? ? ? ? ? ? ? ? ? ath9k_hw_init_global_settings(ah);
>> ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? tasklet_enable(&sc->bcon_tasklet);
>> ? ? ? }
>>
>> ? ? ? /* Disable transmission of beacons */
> A memory barrier between setting beacon.slottime and beacon.updateslot
> should be enough here.
Ok, sounds reasonable.
>
>> @@ -1960,6 +1984,7 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
>> ? ? ? ? ? ? ? ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq);
>>
>> ? ? ? if (changed & BSS_CHANGED_BEACON_INT) {
>> + ? ? ? ? ? ? tasklet_disable(&sc->bcon_tasklet);
>> ? ? ? ? ? ? ? sc->beacon_interval = bss_conf->beacon_int;
>> ? ? ? ? ? ? ? /*
>> ? ? ? ? ? ? ? ?* In case of AP mode, the HW TSF has to be reset
>> @@ -1974,6 +1999,7 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
>> ? ? ? ? ? ? ? } else {
>> ? ? ? ? ? ? ? ? ? ? ? ath_beacon_config(sc, vif);
>> ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? tasklet_enable(&sc->bcon_tasklet);
>> ? ? ? }
>>
>> ? ? ? if (changed & BSS_CHANGED_ERP_PREAMBLE) {
> Makes sense.
>
>> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
>> index f2ade24..174a8ae 100644
>> --- a/drivers/net/wireless/ath/ath9k/xmit.c
>> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
>> @@ -1133,6 +1133,10 @@ void ath_drain_all_txq(struct ath_softc *sc, bool retry_tx)
>> ? ? ? /* Stop beacon queue */
>> ? ? ? ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq);
>>
>> + ? ? /* Stop cab queue */
>> + ? ? if (sc->beacon.cabq != NULL)
>> + ? ? ? ? ? ? ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.cabq->axq_qnum);
>> +
>> ? ? ? /* Stop data queues */
>> ? ? ? for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) {
>> ? ? ? ? ? ? ? if (ATH_TXQ_SETUP(sc, i)) {
> Makes sense.
>
>> @@ -1157,6 +1161,11 @@ void ath_drain_all_txq(struct ath_softc *sc, bool retry_tx)
>> ? ? ? ? ? ? ? spin_unlock_bh(&sc->sc_resetlock);
>> ? ? ? }
>>
>> + ? ? /* Drain cab queue
>> + ? ? ?* BUG: for some reason this causes a dump in ath_draintxq(). Why?
>> + ? ? if (sc->beacon.cabq != NULL)
>> + ? ? ? ? ? ? ath_draintxq(sc->sc_ah, sc->beacon.cabq, false); */
>> +
>> ? ? ? for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) {
>> ? ? ? ? ? ? ? if (ATH_TXQ_SETUP(sc, i))
>> ? ? ? ? ? ? ? ? ? ? ? ath_draintxq(sc, &sc->tx.txq[i], retry_tx);
> What kind of dump?
I thought I saved it somewhere but can't find it. Unable to handle a
paging request at too low an address (but not zero) if I remember
correctly.
/Bj?rn
On 2010-11-03 6:36 PM, Bj?rn Smedman wrote:
> Hi all,
>
> The beacon processing in ath9k is done in a tasklet. This tasklet may race
> against beacon allocation/deallocation in process context. The patch below
> is an attempt to point out / avoid these race conditions. My hope is that
> this will stabilize ath9k in a use-case I'm interested in where ap vif
> interfaces are added and removed quite often.
>
> This is purely an RFC, and it's probably synchronizing a bit too much. I'm
> currently testing this patch with no obvious problems so far (except for
> the part in xmit.c which I've commented out). More testing is necessary
> but so is a rewrite of the patch anyway.
>
> Any thoughts?
>
> Best regards,
>
> Bj?rn
> ---
> diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c
> index 19891e7..f91e0b5 100644
> --- a/drivers/net/wireless/ath/ath9k/beacon.c
> +++ b/drivers/net/wireless/ath/ath9k/beacon.c
> @@ -192,6 +192,7 @@ static struct ath_buf *ath_beacon_generate(struct ieee80211_hw *hw,
> if (sc->nvifs > 1) {
> ath_print(common, ATH_DBG_BEACON,
> "Flushing previous cabq traffic\n");
> + ath9k_hw_stoptxdma(sc->sc_ah, cabq->axq_qnum);
> ath_draintxq(sc, cabq, false);
> }
> }
Makes sense
> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
> index b52f1cf..c3d2a36 100644
> --- a/drivers/net/wireless/ath/ath9k/main.c
> +++ b/drivers/net/wireless/ath/ath9k/main.c
> @@ -290,9 +290,11 @@ int ath_set_channel(struct ath_softc *sc, struct ieee80211_hw *hw,
> ath9k_hw_set_interrupts(ah, ah->imask);
>
> if (!(sc->sc_flags & (SC_OP_OFFCHANNEL))) {
> + tasklet_disable(&sc->bcon_tasklet);
> ath_beacon_config(sc, NULL);
> ieee80211_queue_delayed_work(sc->hw, &sc->tx_complete_work, 0);
> ath_start_ani(common);
> + tasklet_enable(&sc->bcon_tasklet);
> }
>
> ps_restore:
Why?
> @@ -838,6 +840,8 @@ static void ath9k_bss_assoc_info(struct ath_softc *sc,
> struct ath_common *common = ath9k_hw_common(ah);
>
> if (bss_conf->assoc) {
> + tasklet_disable(&sc->bcon_tasklet);
> +
> ath_print(common, ATH_DBG_CONFIG,
> "Bss Info ASSOC %d, bssid: %pM\n",
> bss_conf->aid, common->curbssid);
> @@ -861,6 +865,8 @@ static void ath9k_bss_assoc_info(struct ath_softc *sc,
>
> sc->sc_flags |= SC_OP_ANI_RUN;
> ath_start_ani(common);
> +
> + tasklet_enable(&sc->bcon_tasklet);
> } else {
> ath_print(common, ATH_DBG_CONFIG, "Bss Info DISASSOC\n");
> common->curaid = 0;
Unnecessary, does not have anything to do with *sending* beacons.
> @@ -903,8 +909,11 @@ void ath_radio_enable(struct ath_softc *sc, struct ieee80211_hw *hw)
> }
> spin_unlock_bh(&sc->rx.pcu_lock);
>
> - if (sc->sc_flags & SC_OP_BEACONS)
> + if (sc->sc_flags & SC_OP_BEACONS) {
> + tasklet_disable(&sc->bcon_tasklet);
> ath_beacon_config(sc, NULL); /* restart beacons */
> + tasklet_enable(&sc->bcon_tasklet);
> + }
>
> /* Re-Enable interrupts */
> ath9k_hw_set_interrupts(ah, ah->imask);
> @@ -1008,8 +1017,11 @@ int ath_reset(struct ath_softc *sc, bool retry_tx)
> */
> ath_update_txpow(sc);
>
> - if ((sc->sc_flags & SC_OP_BEACONS) || !(sc->sc_flags & (SC_OP_OFFCHANNEL)))
> + if ((sc->sc_flags & SC_OP_BEACONS) || !(sc->sc_flags & (SC_OP_OFFCHANNEL))) {
> + tasklet_disable(&sc->bcon_tasklet);
> ath_beacon_config(sc, NULL); /* restart beacons */
> + tasklet_enable(&sc->bcon_tasklet);
> + }
>
> ath9k_hw_set_interrupts(ah, ah->imask);
>
Why?
> @@ -1517,6 +1529,8 @@ static void ath9k_remove_interface(struct ieee80211_hw *hw,
>
> mutex_lock(&sc->mutex);
>
> + tasklet_disable(&sc->bcon_tasklet);
> +
> /* Stop ANI */
> sc->sc_flags &= ~SC_OP_ANI_RUN;
> del_timer_sync(&common->ani.timer);
> @@ -1544,6 +1558,8 @@ static void ath9k_remove_interface(struct ieee80211_hw *hw,
>
> sc->nvifs--;
>
> + tasklet_enable(&sc->bcon_tasklet);
> +
> mutex_unlock(&sc->mutex);
> }
>
Makes sense.
> @@ -1817,6 +1833,8 @@ static int ath9k_conf_tx(struct ieee80211_hw *hw, u16 queue,
>
> mutex_lock(&sc->mutex);
>
> + tasklet_disable(&sc->bcon_tasklet);
> +
> memset(&qi, 0, sizeof(struct ath9k_tx_queue_info));
>
> qi.tqi_aifs = params->aifs;
> @@ -1839,6 +1857,8 @@ static int ath9k_conf_tx(struct ieee80211_hw *hw, u16 queue,
> if ((qnum == sc->tx.hwq_map[WME_AC_BE]) && !ret)
> ath_beaconq_config(sc);
>
> + tasklet_enable(&sc->bcon_tasklet);
> +
> mutex_unlock(&sc->mutex);
>
> return ret;
I don't think that one's necessary.
> @@ -1930,13 +1950,16 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
> /* Enable transmission of beacons (AP, IBSS, MESH) */
> if ((changed & BSS_CHANGED_BEACON) ||
> ((changed & BSS_CHANGED_BEACON_ENABLED) && bss_conf->enable_beacon)) {
> + tasklet_disable(&sc->bcon_tasklet);
> ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq);
> error = ath_beacon_alloc(aphy, vif);
> if (!error)
> ath_beacon_config(sc, vif);
> + tasklet_enable(&sc->bcon_tasklet);
> }
Makes sense.
> if (changed & BSS_CHANGED_ERP_SLOT) {
> + tasklet_disable(&sc->bcon_tasklet);
> if (bss_conf->use_short_slot)
> slottime = 9;
> else
> @@ -1953,6 +1976,7 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
> ah->slottime = slottime;
> ath9k_hw_init_global_settings(ah);
> }
> + tasklet_enable(&sc->bcon_tasklet);
> }
>
> /* Disable transmission of beacons */
A memory barrier between setting beacon.slottime and beacon.updateslot
should be enough here.
> @@ -1960,6 +1984,7 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
> ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq);
>
> if (changed & BSS_CHANGED_BEACON_INT) {
> + tasklet_disable(&sc->bcon_tasklet);
> sc->beacon_interval = bss_conf->beacon_int;
> /*
> * In case of AP mode, the HW TSF has to be reset
> @@ -1974,6 +1999,7 @@ static void ath9k_bss_info_changed(struct ieee80211_hw *hw,
> } else {
> ath_beacon_config(sc, vif);
> }
> + tasklet_enable(&sc->bcon_tasklet);
> }
>
> if (changed & BSS_CHANGED_ERP_PREAMBLE) {
Makes sense.
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index f2ade24..174a8ae 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -1133,6 +1133,10 @@ void ath_drain_all_txq(struct ath_softc *sc, bool retry_tx)
> /* Stop beacon queue */
> ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq);
>
> + /* Stop cab queue */
> + if (sc->beacon.cabq != NULL)
> + ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.cabq->axq_qnum);
> +
> /* Stop data queues */
> for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) {
> if (ATH_TXQ_SETUP(sc, i)) {
Makes sense.
> @@ -1157,6 +1161,11 @@ void ath_drain_all_txq(struct ath_softc *sc, bool retry_tx)
> spin_unlock_bh(&sc->sc_resetlock);
> }
>
> + /* Drain cab queue
> + * BUG: for some reason this causes a dump in ath_draintxq(). Why?
> + if (sc->beacon.cabq != NULL)
> + ath_draintxq(sc->sc_ah, sc->beacon.cabq, false); */
> +
> for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) {
> if (ATH_TXQ_SETUP(sc, i))
> ath_draintxq(sc, &sc->tx.txq[i], retry_tx);
What kind of dump?
- Felix