I did these patches a while ago but never got around sending them.
I know it's a bit late for .39 and don't mind if they go for .40
(or not at all if they are bad ;) ).
Grazvydas Ignotas (2):
wl1251: remove wl1251_ps_set_elp function
wl1251: fix elp_work race condition
drivers/net/wireless/wl1251/ps.c | 41 +++++--------------------------------
1 files changed, 6 insertions(+), 35 deletions(-)
Grazvydas Ignotas <[email protected]> writes:
> While working on PS I've noticed elp_work is kicking rather often, and
> sometimes the chip is put to sleep before 5ms delay expires. This
> seems to happen because by the time wl1251_ps_elp_wakeup is called
> elp_work might still be pending. After wakeup is done, the processing
> may take some time, during which 5ms might expire and elp_work might
> get scheduled. In this case, ss soon as 1st thread finishes work and
> releases the mutex, elp_work will then put the device to sleep without
> 5ms delay. In addition 1st thread will queue additional elp_work
> needlessly.
>
> Fix this by cancelling work in wl1251_ps_elp_wakeup instead.
Thanks for debugging this.
> Signed-off-by: Grazvydas Ignotas <[email protected]>
Acked-by: Kalle Valo <[email protected]>
--
Kalle Valo
While working on PS I've noticed elp_work is kicking rather often, and
sometimes the chip is put to sleep before 5ms delay expires. This
seems to happen because by the time wl1251_ps_elp_wakeup is called
elp_work might still be pending. After wakeup is done, the processing
may take some time, during which 5ms might expire and elp_work might
get scheduled. In this case, ss soon as 1st thread finishes work and
releases the mutex, elp_work will then put the device to sleep without
5ms delay. In addition 1st thread will queue additional elp_work
needlessly.
Fix this by cancelling work in wl1251_ps_elp_wakeup instead.
Signed-off-by: Grazvydas Ignotas <[email protected]>
---
drivers/net/wireless/wl1251/ps.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/wl1251/ps.c b/drivers/net/wireless/wl1251/ps.c
index 842155e..9cc5147 100644
--- a/drivers/net/wireless/wl1251/ps.c
+++ b/drivers/net/wireless/wl1251/ps.c
@@ -58,7 +58,6 @@ void wl1251_ps_elp_sleep(struct wl1251 *wl)
unsigned long delay;
if (wl->psm) {
- cancel_delayed_work(&wl->elp_work);
delay = msecs_to_jiffies(ELP_ENTRY_DELAY);
ieee80211_queue_delayed_work(wl->hw, &wl->elp_work, delay);
}
@@ -69,6 +68,9 @@ int wl1251_ps_elp_wakeup(struct wl1251 *wl)
unsigned long timeout, start;
u32 elp_reg;
+ if (delayed_work_pending(&wl->elp_work))
+ cancel_delayed_work(&wl->elp_work);
+
if (!wl->elp)
return 0;
--
1.6.3.3
wl1251_ps_set_elp() only does acx_sleep_auth call and takes the chip
from/to ELP, however all callers of wl1251_ps_set_mode() have already
taken the chip out of ELP and puts it back to ELP when they finish.
This makes ELP calls (and register writes they result in) superfluous.
So remove wl1251_ps_set_elp function and call acx_sleep_auth directly.
Signed-off-by: Grazvydas Ignotas <[email protected]>
---
drivers/net/wireless/wl1251/ps.c | 37 +++----------------------------------
1 files changed, 3 insertions(+), 34 deletions(-)
diff --git a/drivers/net/wireless/wl1251/ps.c b/drivers/net/wireless/wl1251/ps.c
index 9ba23ed..842155e 100644
--- a/drivers/net/wireless/wl1251/ps.c
+++ b/drivers/net/wireless/wl1251/ps.c
@@ -102,38 +102,6 @@ int wl1251_ps_elp_wakeup(struct wl1251 *wl)
return 0;
}
-static int wl1251_ps_set_elp(struct wl1251 *wl, bool enable)
-{
- int ret;
-
- if (enable) {
- wl1251_debug(DEBUG_PSM, "sleep auth psm/elp");
-
- ret = wl1251_acx_sleep_auth(wl, WL1251_PSM_ELP);
- if (ret < 0)
- return ret;
-
- wl1251_ps_elp_sleep(wl);
- } else {
- wl1251_debug(DEBUG_PSM, "sleep auth cam");
-
- /*
- * When the target is in ELP, we can only
- * access the ELP control register. Thus,
- * we have to wake the target up before
- * changing the power authorization.
- */
-
- wl1251_ps_elp_wakeup(wl);
-
- ret = wl1251_acx_sleep_auth(wl, WL1251_PSM_CAM);
- if (ret < 0)
- return ret;
- }
-
- return 0;
-}
-
int wl1251_ps_set_mode(struct wl1251 *wl, enum wl1251_cmd_ps_mode mode)
{
int ret;
@@ -162,7 +130,7 @@ int wl1251_ps_set_mode(struct wl1251 *wl, enum wl1251_cmd_ps_mode mode)
if (ret < 0)
return ret;
- ret = wl1251_ps_set_elp(wl, true);
+ ret = wl1251_acx_sleep_auth(wl, WL1251_PSM_ELP);
if (ret < 0)
return ret;
@@ -171,7 +139,8 @@ int wl1251_ps_set_mode(struct wl1251 *wl, enum wl1251_cmd_ps_mode mode)
case STATION_ACTIVE_MODE:
default:
wl1251_debug(DEBUG_PSM, "leaving psm");
- ret = wl1251_ps_set_elp(wl, false);
+
+ ret = wl1251_acx_sleep_auth(wl, WL1251_PSM_CAM);
if (ret < 0)
return ret;
--
1.6.3.3
Grazvydas Ignotas <[email protected]> writes:
> wl1251_ps_set_elp() only does acx_sleep_auth call and takes the chip
> from/to ELP, however all callers of wl1251_ps_set_mode() have already
> taken the chip out of ELP and puts it back to ELP when they finish.
> This makes ELP calls (and register writes they result in) superfluous.
>
> So remove wl1251_ps_set_elp function and call acx_sleep_auth directly.
Thanks, makes sense to me.
> Signed-off-by: Grazvydas Ignotas <[email protected]>
Acked-by: Kalle Valo <[email protected]>
--
Kalle Valo