With PSM handled in FW, the checks in wl1271_elp_work() are always true.
Thus during active traffic we constantly enter and exit ELP (many times
per second). As each ELP exit takes ~10ms, this can have an adverse
effect on throughput and interactivity.
Set the ELP timeout to the dyn-ps timeout. This period is longer and
avoids the above problem. It also makes sense to stay out of ELP while
we are awake on the network, to minimize delays in Tx/Rx. The same thing
was done by the mac80211 dynamic-ps mechanism before the FW DPS changes.
Signed-off-by: Arik Nemtsov <[email protected]>
---
drivers/net/wireless/wl12xx/ps.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/ps.c b/drivers/net/wireless/wl12xx/ps.c
index 23d6750..78f598b 100644
--- a/drivers/net/wireless/wl12xx/ps.c
+++ b/drivers/net/wireless/wl12xx/ps.c
@@ -69,8 +69,6 @@ out:
mutex_unlock(&wl->mutex);
}
-#define ELP_ENTRY_DELAY 5
-
/* Routines to toggle sleep mode while in ELP */
void wl1271_ps_elp_sleep(struct wl1271 *wl)
{
@@ -90,7 +88,7 @@ void wl1271_ps_elp_sleep(struct wl1271 *wl)
}
ieee80211_queue_delayed_work(wl->hw, &wl->elp_work,
- msecs_to_jiffies(ELP_ENTRY_DELAY));
+ msecs_to_jiffies(wl->conf.conn.dynamic_ps_timeout));
}
int wl1271_ps_elp_wakeup(struct wl1271 *wl)
--
1.7.5.4
On Tue, Feb 28, 2012 at 11:51, Luciano Coelho <[email protected]> wrote:
> On Tue, 2012-02-28 at 00:41 +0200, Arik Nemtsov wrote:
>> Before, the link was first freed (invalidating it in the map), and later
>> on vif removal, all valid wlvif-related links were reset. Since these
>> links were already invalid, we failed to reset them.
>> The bug was made worse by op_stop, which set the tx_queue_count to 0
>> arbitrarily. This resulted in a negative tx_queue_count in some scenarios.
>>
>> Fix this by resetting the Tx-queues of a link when freeing it. Add a
>> WARN_ON and reset all link Tx-queues in op_stop, to avoid a negative
>> tx_queue_count.
>>
>> Signed-off-by: Arik Nemtsov <[email protected]>
>> ---
>
> [...]
>
>> diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
>> index 6446e4d..311b5a2 100644
>> --- a/drivers/net/wireless/wl12xx/tx.c
>> +++ b/drivers/net/wireless/wl12xx/tx.c
>> @@ -527,6 +527,7 @@ static struct sk_buff *wl12xx_lnk_skb_dequeue(struct wl1271 *wl,
>> ? ? ? if (skb) {
>> ? ? ? ? ? ? ? int q = wl1271_tx_get_queue(skb_get_queue_mapping(skb));
>> ? ? ? ? ? ? ? spin_lock_irqsave(&wl->wl_lock, flags);
>> + ? ? ? ? ? ? WARN_ON(wl->tx_queue_count[q] <= 0);
>> ? ? ? ? ? ? ? wl->tx_queue_count[q]--;
>> ? ? ? ? ? ? ? spin_unlock_irqrestore(&wl->wl_lock, flags);
>> ? ? ? }
>> @@ -602,6 +603,7 @@ static struct sk_buff *wl1271_skb_dequeue(struct wl1271 *wl)
>> ? ? ? ? ? ? ? skb = wl->dummy_packet;
>> ? ? ? ? ? ? ? q = wl1271_tx_get_queue(skb_get_queue_mapping(skb));
>> ? ? ? ? ? ? ? spin_lock_irqsave(&wl->wl_lock, flags);
>> + ? ? ? ? ? ? WARN_ON(wl->tx_queue_count[q] <= 0);
>> ? ? ? ? ? ? ? wl->tx_queue_count[q]--;
>> ? ? ? ? ? ? ? spin_unlock_irqrestore(&wl->wl_lock, flags);
>> ? ? ? }
>
> [...]
>
>> @@ -973,8 +974,14 @@ void wl12xx_tx_reset(struct wl1271 *wl, bool reset_tx_queues)
>> ? ? ? struct sk_buff *skb;
>> ? ? ? struct ieee80211_tx_info *info;
>>
>> - ? ? for (i = 0; i < NUM_TX_QUEUES; i++)
>> - ? ? ? ? ? ? wl->tx_queue_count[i] = 0;
>> + ? ? /* only reset the queues if something bad happened */
>> + ? ? if (WARN_ON(wl1271_tx_total_queue_count(wl) != 0)) {
>
> Let's make all these WARN_ONs as WARN_ON_ONCE as it is recommended
> nowadays. ?This will make Kalle Valo happier. :)
>
> I can do a quick sed before I apply this, if you agree.
Sure thing.
Arik
Before, the link was first freed (invalidating it in the map), and later
on vif removal, all valid wlvif-related links were reset. Since these
links were already invalid, we failed to reset them.
The bug was made worse by op_stop, which set the tx_queue_count to 0
arbitrarily. This resulted in a negative tx_queue_count in some scenarios.
Fix this by resetting the Tx-queues of a link when freeing it. Add a
WARN_ON and reset all link Tx-queues in op_stop, to avoid a negative
tx_queue_count.
Signed-off-by: Arik Nemtsov <[email protected]>
---
drivers/net/wireless/wl12xx/cmd.c | 7 +++++++
drivers/net/wireless/wl12xx/main.c | 1 -
drivers/net/wireless/wl12xx/tx.c | 13 ++++++++++---
3 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/cmd.c b/drivers/net/wireless/wl12xx/cmd.c
index ef994da..ae1f3d7 100644
--- a/drivers/net/wireless/wl12xx/cmd.c
+++ b/drivers/net/wireless/wl12xx/cmd.c
@@ -485,6 +485,13 @@ void wl12xx_free_link(struct wl1271 *wl, struct wl12xx_vif *wlvif, u8 *hlid)
__clear_bit(*hlid, wl->links_map);
__clear_bit(*hlid, wlvif->links_map);
spin_unlock_irqrestore(&wl->wl_lock, flags);
+
+ /*
+ * At this point op_tx() will not add more packets to the queues. We
+ * can purge them.
+ */
+ wl1271_tx_reset_link_queues(wl, *hlid);
+
*hlid = WL12XX_INVALID_LINK_ID;
}
diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index adf9bbc..712e385 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -4228,7 +4228,6 @@ void wl1271_free_sta(struct wl1271 *wl, struct wl12xx_vif *wlvif, u8 hlid)
clear_bit(hlid, wlvif->ap.sta_hlid_map);
memset(wl->links[hlid].addr, 0, ETH_ALEN);
wl->links[hlid].ba_bitmap = 0;
- wl1271_tx_reset_link_queues(wl, hlid);
__clear_bit(hlid, &wl->ap_ps_map);
__clear_bit(hlid, (unsigned long *)&wl->ap_fw_ps_map);
wl12xx_free_link(wl, wlvif, &hlid);
diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
index 6446e4d..311b5a2 100644
--- a/drivers/net/wireless/wl12xx/tx.c
+++ b/drivers/net/wireless/wl12xx/tx.c
@@ -527,6 +527,7 @@ static struct sk_buff *wl12xx_lnk_skb_dequeue(struct wl1271 *wl,
if (skb) {
int q = wl1271_tx_get_queue(skb_get_queue_mapping(skb));
spin_lock_irqsave(&wl->wl_lock, flags);
+ WARN_ON(wl->tx_queue_count[q] <= 0);
wl->tx_queue_count[q]--;
spin_unlock_irqrestore(&wl->wl_lock, flags);
}
@@ -602,6 +603,7 @@ static struct sk_buff *wl1271_skb_dequeue(struct wl1271 *wl)
skb = wl->dummy_packet;
q = wl1271_tx_get_queue(skb_get_queue_mapping(skb));
spin_lock_irqsave(&wl->wl_lock, flags);
+ WARN_ON(wl->tx_queue_count[q] <= 0);
wl->tx_queue_count[q]--;
spin_unlock_irqrestore(&wl->wl_lock, flags);
}
@@ -959,7 +961,6 @@ void wl12xx_tx_reset_wlvif(struct wl1271 *wl, struct wl12xx_vif *wlvif)
else
wlvif->sta.ba_rx_bitmap = 0;
- wl1271_tx_reset_link_queues(wl, i);
wl->links[i].allocated_pkts = 0;
wl->links[i].prev_freed_pkts = 0;
}
@@ -973,8 +974,14 @@ void wl12xx_tx_reset(struct wl1271 *wl, bool reset_tx_queues)
struct sk_buff *skb;
struct ieee80211_tx_info *info;
- for (i = 0; i < NUM_TX_QUEUES; i++)
- wl->tx_queue_count[i] = 0;
+ /* only reset the queues if something bad happened */
+ if (WARN_ON(wl1271_tx_total_queue_count(wl) != 0)) {
+ for (i = 0; i < WL12XX_MAX_LINKS; i++)
+ wl1271_tx_reset_link_queues(wl, i);
+
+ for (i = 0; i < NUM_TX_QUEUES; i++)
+ wl->tx_queue_count[i] = 0;
+ }
wl->stopped_queues_map = 0;
--
1.7.5.4
Luciano Coelho <[email protected]> writes:
>> @@ -973,8 +974,14 @@ void wl12xx_tx_reset(struct wl1271 *wl, bool reset_tx_queues)
>> struct sk_buff *skb;
>> struct ieee80211_tx_info *info;
>>
>> - for (i = 0; i < NUM_TX_QUEUES; i++)
>> - wl->tx_queue_count[i] = 0;
>> + /* only reset the queues if something bad happened */
>> + if (WARN_ON(wl1271_tx_total_queue_count(wl) != 0)) {
>
> Let's make all these WARN_ONs as WARN_ON_ONCE as it is recommended
> nowadays. This will make Kalle Valo happier. :)
Hehe. Actually the dislike against WARN_ON() was started by John, I
again started the war to killing BUG_ON() :)
But nevertheless, this changes makes also me happy. I have seen cases
where the whole system reboots because of WARN_ON() spam, so
WARN_ON_ONCE() is much better in in cases where's a possibility that it
will get printed a lot. Of course printk_ratelimit() is another option,
but for some reason it isn't that popular.
--
Kalle Valo
Re-factor the Tx scheduler so that the system_hlid is taken into account
before restarting an iteration over the wlvifs. Previously this
hlid had a lower priority and would starve if some wlvif had many
packets.
In addition avoid iterating over wlvifs past last_wlvif when performing
the a second pass. If we had packets in those wlvifs they would have
been found earlier.
Signed-off-by: Arik Nemtsov <[email protected]>
---
drivers/net/wireless/wl12xx/tx.c | 17 +++++++++++++----
1 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
index 311b5a2..76480a0 100644
--- a/drivers/net/wireless/wl12xx/tx.c
+++ b/drivers/net/wireless/wl12xx/tx.c
@@ -572,6 +572,7 @@ static struct sk_buff *wl1271_skb_dequeue(struct wl1271 *wl)
struct wl12xx_vif *wlvif = wl->last_wlvif;
struct sk_buff *skb = NULL;
+ /* continue from last wlvif (round robin) */
if (wlvif) {
wl12xx_for_each_wlvif_continue(wl, wlvif) {
skb = wl12xx_vif_skb_dequeue(wl, wlvif);
@@ -582,7 +583,11 @@ static struct sk_buff *wl1271_skb_dequeue(struct wl1271 *wl)
}
}
- /* do another pass */
+ /* dequeue from the system HLID before the restarting wlvif list */
+ if (!skb)
+ skb = wl12xx_lnk_skb_dequeue(wl, &wl->links[wl->system_hlid]);
+
+ /* do a new pass over the wlvif list */
if (!skb) {
wl12xx_for_each_wlvif(wl, wlvif) {
skb = wl12xx_vif_skb_dequeue(wl, wlvif);
@@ -590,12 +595,16 @@ static struct sk_buff *wl1271_skb_dequeue(struct wl1271 *wl)
wl->last_wlvif = wlvif;
break;
}
+
+ /*
+ * No need to continue after last_wlvif. The previous
+ * pass should have found it.
+ */
+ if (wlvif == wl->last_wlvif)
+ break;
}
}
- if (!skb)
- skb = wl12xx_lnk_skb_dequeue(wl, &wl->links[wl->system_hlid]);
-
if (!skb &&
test_and_clear_bit(WL1271_FLAG_DUMMY_PACKET_PENDING, &wl->flags)) {
int q;
--
1.7.5.4
Flush our Tx queues before suspending or changing the channel due to a
channel_switch element in the AP beacon.
Signed-off-by: Arik Nemtsov <[email protected]>
---
drivers/net/wireless/wl12xx/main.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index 712e385..ad1a4ad 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -1737,6 +1737,8 @@ static int wl1271_op_suspend(struct ieee80211_hw *hw,
wl1271_debug(DEBUG_MAC80211, "mac80211 suspend wow=%d", !!wow);
WARN_ON(!wow || !wow->any);
+ wl1271_tx_flush(wl);
+
wl->wow_enabled = true;
wl12xx_for_each_wlvif(wl, wlvif) {
ret = wl1271_configure_suspend(wl, wlvif);
@@ -4496,6 +4498,8 @@ static void wl12xx_op_channel_switch(struct ieee80211_hw *hw,
wl1271_debug(DEBUG_MAC80211, "mac80211 channel switch");
+ wl1271_tx_flush(wl);
+
mutex_lock(&wl->mutex);
if (unlikely(wl->state == WL1271_STATE_OFF)) {
--
1.7.5.4
On Tue, 2012-02-28 at 00:41 +0200, Arik Nemtsov wrote:
> Before, the link was first freed (invalidating it in the map), and later
> on vif removal, all valid wlvif-related links were reset. Since these
> links were already invalid, we failed to reset them.
> The bug was made worse by op_stop, which set the tx_queue_count to 0
> arbitrarily. This resulted in a negative tx_queue_count in some scenarios.
>
> Fix this by resetting the Tx-queues of a link when freeing it. Add a
> WARN_ON and reset all link Tx-queues in op_stop, to avoid a negative
> tx_queue_count.
>
> Signed-off-by: Arik Nemtsov <[email protected]>
> ---
[...]
> diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
> index 6446e4d..311b5a2 100644
> --- a/drivers/net/wireless/wl12xx/tx.c
> +++ b/drivers/net/wireless/wl12xx/tx.c
> @@ -527,6 +527,7 @@ static struct sk_buff *wl12xx_lnk_skb_dequeue(struct wl1271 *wl,
> if (skb) {
> int q = wl1271_tx_get_queue(skb_get_queue_mapping(skb));
> spin_lock_irqsave(&wl->wl_lock, flags);
> + WARN_ON(wl->tx_queue_count[q] <= 0);
> wl->tx_queue_count[q]--;
> spin_unlock_irqrestore(&wl->wl_lock, flags);
> }
> @@ -602,6 +603,7 @@ static struct sk_buff *wl1271_skb_dequeue(struct wl1271 *wl)
> skb = wl->dummy_packet;
> q = wl1271_tx_get_queue(skb_get_queue_mapping(skb));
> spin_lock_irqsave(&wl->wl_lock, flags);
> + WARN_ON(wl->tx_queue_count[q] <= 0);
> wl->tx_queue_count[q]--;
> spin_unlock_irqrestore(&wl->wl_lock, flags);
> }
[...]
> @@ -973,8 +974,14 @@ void wl12xx_tx_reset(struct wl1271 *wl, bool reset_tx_queues)
> struct sk_buff *skb;
> struct ieee80211_tx_info *info;
>
> - for (i = 0; i < NUM_TX_QUEUES; i++)
> - wl->tx_queue_count[i] = 0;
> + /* only reset the queues if something bad happened */
> + if (WARN_ON(wl1271_tx_total_queue_count(wl) != 0)) {
Let's make all these WARN_ONs as WARN_ON_ONCE as it is recommended
nowadays. This will make Kalle Valo happier. :)
I can do a quick sed before I apply this, if you agree.
--
Cheers,
Luca.
Ensure our queues are empty at the end of a tx_flush(), in case we
timeout on passively waiting for them. This makes sure no left-overs are
transmitted when we are on the wrong channel.
Signed-off-by: Arik Nemtsov <[email protected]>
---
drivers/net/wireless/wl12xx/tx.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/tx.c b/drivers/net/wireless/wl12xx/tx.c
index 76480a0..3540d4a 100644
--- a/drivers/net/wireless/wl12xx/tx.c
+++ b/drivers/net/wireless/wl12xx/tx.c
@@ -1040,6 +1040,7 @@ void wl12xx_tx_reset(struct wl1271 *wl, bool reset_tx_queues)
void wl1271_tx_flush(struct wl1271 *wl)
{
unsigned long timeout;
+ int i;
timeout = jiffies + usecs_to_jiffies(WL1271_TX_FLUSH_TIMEOUT);
while (!time_after(jiffies, timeout)) {
@@ -1057,6 +1058,12 @@ void wl1271_tx_flush(struct wl1271 *wl)
}
wl1271_warning("Unable to flush all TX buffers, timed out.");
+
+ /* forcibly flush all Tx buffers on our queues */
+ mutex_lock(&wl->mutex);
+ for (i = 0; i < WL12XX_MAX_LINKS; i++)
+ wl1271_tx_reset_link_queues(wl, i);
+ mutex_unlock(&wl->mutex);
}
u32 wl1271_tx_min_rate_get(struct wl1271 *wl, u32 rate_set)
--
1.7.5.4
On Tue, 2012-02-28 at 00:41 +0200, Arik Nemtsov wrote:
> With PSM handled in FW, the checks in wl1271_elp_work() are always true.
> Thus during active traffic we constantly enter and exit ELP (many times
> per second). As each ELP exit takes ~10ms, this can have an adverse
> effect on throughput and interactivity.
> Set the ELP timeout to the dyn-ps timeout. This period is longer and
> avoids the above problem. It also makes sense to stay out of ELP while
> we are awake on the network, to minimize delays in Tx/Rx. The same thing
> was done by the mac80211 dynamic-ps mechanism before the FW DPS changes.
>
> Signed-off-by: Arik Nemtsov <[email protected]>
> ---
Applied this whole series with the change from WARN_ON to WARN_ON_ONCE
on patch 3/7 and pushed.
Thanks, Arik!
--
Cheers,
Luca.
This ensures the user won't encounter lag associated with getting in and
out of PSM when the card is in use.
Signed-off-by: Arik Nemtsov <[email protected]>
---
drivers/net/wireless/wl12xx/main.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/main.c b/drivers/net/wireless/wl12xx/main.c
index ad1a4ad..418b2b8 100644
--- a/drivers/net/wireless/wl12xx/main.c
+++ b/drivers/net/wireless/wl12xx/main.c
@@ -246,7 +246,7 @@ static struct conf_drv_settings default_conf = {
.psm_entry_retries = 8,
.psm_exit_retries = 16,
.psm_entry_nullfunc_retries = 3,
- .dynamic_ps_timeout = 100,
+ .dynamic_ps_timeout = 200,
.forced_ps = false,
.keep_alive_interval = 55000,
.max_listen_interval = 20,
--
1.7.5.4
On Wed, 2012-02-29 at 10:31 +0200, Kalle Valo wrote:
> Luciano Coelho <[email protected]> writes:
>
> >> @@ -973,8 +974,14 @@ void wl12xx_tx_reset(struct wl1271 *wl, bool reset_tx_queues)
> >> struct sk_buff *skb;
> >> struct ieee80211_tx_info *info;
> >>
> >> - for (i = 0; i < NUM_TX_QUEUES; i++)
> >> - wl->tx_queue_count[i] = 0;
> >> + /* only reset the queues if something bad happened */
> >> + if (WARN_ON(wl1271_tx_total_queue_count(wl) != 0)) {
> >
> > Let's make all these WARN_ONs as WARN_ON_ONCE as it is recommended
> > nowadays. This will make Kalle Valo happier. :)
>
> Hehe. Actually the dislike against WARN_ON() was started by John, I
> again started the war to killing BUG_ON() :)
Right, John's email was where I got the "recommended" from. ;)
> But nevertheless, this changes makes also me happy. I have seen cases
> where the whole system reboots because of WARN_ON() spam, so
> WARN_ON_ONCE() is much better in in cases where's a possibility that it
> will get printed a lot. Of course printk_ratelimit() is another option,
> but for some reason it isn't that popular.
Yeah, I totally agree. Especially when we have a loop, WARN_ON_ONCE
should be sufficient, in my opinion.
--
Cheers,
Luca.
These bits are used in op_tx to determine if a packet should be dropped.
As such we should use the spin lock to sync the state.
Signed-off-by: Arik Nemtsov <[email protected]>
---
drivers/net/wireless/wl12xx/cmd.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/wl12xx/cmd.c b/drivers/net/wireless/wl12xx/cmd.c
index b776d9d..ef994da 100644
--- a/drivers/net/wireless/wl12xx/cmd.c
+++ b/drivers/net/wireless/wl12xx/cmd.c
@@ -459,23 +459,32 @@ out:
int wl12xx_allocate_link(struct wl1271 *wl, struct wl12xx_vif *wlvif, u8 *hlid)
{
+ unsigned long flags;
u8 link = find_first_zero_bit(wl->links_map, WL12XX_MAX_LINKS);
if (link >= WL12XX_MAX_LINKS)
return -EBUSY;
+ /* these bits are used by op_tx */
+ spin_lock_irqsave(&wl->wl_lock, flags);
__set_bit(link, wl->links_map);
__set_bit(link, wlvif->links_map);
+ spin_unlock_irqrestore(&wl->wl_lock, flags);
*hlid = link;
return 0;
}
void wl12xx_free_link(struct wl1271 *wl, struct wl12xx_vif *wlvif, u8 *hlid)
{
+ unsigned long flags;
+
if (*hlid == WL12XX_INVALID_LINK_ID)
return;
+ /* these bits are used by op_tx */
+ spin_lock_irqsave(&wl->wl_lock, flags);
__clear_bit(*hlid, wl->links_map);
__clear_bit(*hlid, wlvif->links_map);
+ spin_unlock_irqrestore(&wl->wl_lock, flags);
*hlid = WL12XX_INVALID_LINK_ID;
}
--
1.7.5.4