@@ -764,6 +764,7 @@ struct ath_softc {atomic_twow_got_bmiss_intr;atomic_twow_sleep_proc_intr;/* in the middle of WoW sleep ? */u32wow_intr_before_sleep;+boolsuspending;#endif};
@@ -764,6 +764,7 @@ struct ath_softc {atomic_twow_got_bmiss_intr;atomic_twow_sleep_proc_intr;/* in the middle of WoW sleep ? */u32wow_intr_before_sleep;+boolsuspending;#endif};
@@ -764,6 +764,7 @@ struct ath_softc {atomic_twow_got_bmiss_intr;atomic_twow_sleep_proc_intr;/* in the middle of WoW sleep ? */u32wow_intr_before_sleep;+boolsuspending;#endif};
Thanks for the patch! Please note the style issue here, you should
use a tab, but other than that lets review what happened.
quoted
if (!test_bit(SC_OP_PRIM_STA_VIF, &sc->sc_flags))
return;
Note that what this will do is call later mod_timer() for
rx_poll_timer, the right thing to do then, which would
be equivalent to your patch is to modify the ath_start_rx_poll()
to instead use the new API mod_timer_pending() added on v2.6.30
via commit 74019224. This would not re-arm the timer if it was
previously removed.
commit 74019224ac34b044b44a31dd89a54e3477db4896
Author: Ingo Molnar [off-list ref]
Date: Wed Feb 18 12:23:29 2009 +0100
timers: add mod_timer_pending()
Impact: new timer API
Based on an idea from Martin Josefsson with the help of
Patrick McHardy and Stephen Hemminger:
introduce the mod_timer_pending() API which is a mod_timer()
offspring that is an invariant on already removed timers.
(regular mod_timer() re-activates non-pending timers.)
This is useful for the networking code in that it can
allow unserialized mod_timer_pending() timer-forwarding
calls, but a single del_timer*() will stop the timer
from being reactivated again.
Also while at it:
- optimize the regular mod_timer() path some more, the
timer-stat and a debug check was needlessly duplicated
in __mod_timer().
- make the exports come straight after the function, as
most other exports in timer.c already did.
- eliminate __mod_timer() as an external API, change the
users to mod_timer().
The regular mod_timer() code path is not impacted
significantly, due to inlining optimizations and due to
the simplifications.
Based-on-patch-from: Stephen Hemminger [off-list ref]
Acked-by: Stephen Hemminger [off-list ref]
Cc: "David S. Miller" [off-list ref]
Cc: Patrick McHardy [off-list ref]
Cc: netdev@vger.kernel.org
Cc: Oleg Nesterov [off-list ref]
Cc: Andrew Morton [off-list ref]
Signed-off-by: Ingo Molnar [off-list ref]
Note that what this will do is call later mod_timer() for
rx_poll_timer, the right thing to do then, which would
be equivalent to your patch is to modify the ath_start_rx_poll()
to instead use the new API mod_timer_pending() added on v2.6.30
via commit 74019224. This would not re-arm the timer if it was
previously removed.
Thanks for the details Luis. Converting to mod_timer_pending() seems to do
the trick as well.
Parag
Thanks for the patch! Please note the style issue here, you should
use a tab, but other than that lets review what happened.
quoted
quoted
if (!test_bit(SC_OP_PRIM_STA_VIF, &sc->sc_flags))
return;
Note that what this will do is call later mod_timer() for
rx_poll_timer, the right thing to do then, which would
be equivalent to your patch is to modify the ath_start_rx_poll()
to instead use the new API mod_timer_pending() added on v2.6.30
via commit 74019224. This would not re-arm the timer if it was
previously removed.
But isn't this prevent to run timer in case it was not running, but
we want to start it ?
Looking at this makes me think we should review all usage of
mod_timer all over our 802.11 drivers, and mac80211, cfg80211 as
well.
I mac80211 we use local->suspended and local->quiesce booleans to
prevent reschedule of timers when going to suspend for example.
Works use ifmgd->associted to prevent reschedule when we are
disassociating.
I think on ath9k also some boolean variable should be used, not only
for rx_poll_timer but also for other works i.e. tx_complete_work.
Is possible to use SC_OP_INVALID flags, since mac80211 call ath9k_stop
on suspend and ath9k_start on resume.
Stanislaw
Thanks for the patch! Please note the style issue here, you should
use a tab, but other than that lets review what happened.
quoted
quoted
if (!test_bit(SC_OP_PRIM_STA_VIF, &sc->sc_flags))
return;
Note that what this will do is call later mod_timer() for
rx_poll_timer, the right thing to do then, which would
be equivalent to your patch is to modify the ath_start_rx_poll()
to instead use the new API mod_timer_pending() added on v2.6.30
via commit 74019224. This would not re-arm the timer if it was
previously removed.
But isn't this prevent to run timer in case it was not running, but
we want to start it ?
No you're right, this would never have it run, sorry about that.
But lets look at this a little closer. The issue is at suspend
time, and the issue is ath9k is trying to schedule work while
going to suspend.
So when does this work get called?
Given the trace this is hit when the timer rx_poll_timer runs,
which in turn calls ath_rx_poll() to schedule hw_check_work work.
The rx_poll_timer however was originally only set at the end of
the routine that hw_check_work sets off but also at other entry
points (ath_start_rx_poll() callers). Once ath_start_rx_poll()
gets called though we can go on looping as follows:
work timer work
hw_check_work --> rx_poll_timer --> hw_check_work
At suspend time we do this though:
ath_cancel_work(sc);
del_timer_sync(&sc->rx_poll_timer);
So perhaps what we need is:
@@ -2151,9 +2152,9 @@ static int ath9k_suspend(struct ieee80211_hw *hw,mutex_lock(&sc->mutex);+del_timer_sync(&sc->rx_poll_timer);ath_cancel_work(sc);ath_stop_ani(sc);-del_timer_sync(&sc->rx_poll_timer);if(test_bit(SC_OP_INVALID,&sc->sc_flags)){ath_dbg(common,ANY,"Device not present\n");
But then we have the chicken and the egg problem, as the work item
could fire off the timer so it would seem to be good to prevent
adding new work when suspending.
quoted
Looking at this makes me think we should review all usage of
mod_timer all over our 802.11 drivers, and mac80211, cfg80211 as
well.
In mac80211 we use local->suspended and local->quiesce booleans to
prevent reschedule of timers when going to suspend for example.
Works use ifmgd->associted to prevent reschedule when we are
disassociating.
I think on ath9k also some boolean variable should be used, not only
for rx_poll_timer but also for other works i.e. tx_complete_work.
Is possible to use SC_OP_INVALID flags, since mac80211 call ath9k_stop
on suspend and ath9k_start on resume.
Indeed however ieee80211_queue_work() already does a suspend check for
us, it just complains as many drivers including mac80211 were setting
up work incorrectly. The warning was put in place to help us find the
issues. Using SC_OP_INVALID seems fair but we could also add a routine
ieee80211_queue_work_safe() that silently fails if we are quiescing or
suspended and not resuming but I can see that creating very sloppy
driver writing and everyone abusing it.
OK how about this for stable for now:
On Thu, Mar 21, 2013 at 12:33:31PM -0700, Luis R. Rodriguez wrote:
So when does this work get called?
Given the trace this is hit when the timer rx_poll_timer runs,
which in turn calls ath_rx_poll() to schedule hw_check_work work.
The rx_poll_timer however was originally only set at the end of
the routine that hw_check_work sets off but also at other entry
points (ath_start_rx_poll() callers). Once ath_start_rx_poll()
gets called though we can go on looping as follows:
work timer work
hw_check_work --> rx_poll_timer --> hw_check_work
At suspend time we do this though:
ath_cancel_work(sc);
del_timer_sync(&sc->rx_poll_timer);
+ del_timer_sync(&sc->rx_poll_timer);
ath_cancel_work(sc);
ath_stop_ani(sc);
- del_timer_sync(&sc->rx_poll_timer);
[snip]
But then we have the chicken and the egg problem, as the work item
could fire off the timer so it would seem to be good to prevent
adding new work when suspending.
Yup, this is egg and chicken problem, I think it can not be fixed by
changing ordering of del_timer and cancel_work, it would be like:
del_timer_sync(&sc->rx_poll_timer);
/* but timer could schedule work */
ath_cancel_work(sc);
/* but work could schedule timer */
del_timer_sync(&sc->rx_poll_timer);
/* but timer could schedule work */
ath_cancel_work(sc);
/* And so on ... */
Check is needed in work or timer callback, depending what is canceled
last, to fix the problem ...
quoted
In mac80211 we use local->suspended and local->quiesce booleans to
prevent reschedule of timers when going to suspend for example.
Works use ifmgd->associted to prevent reschedule when we are
disassociating.
I think on ath9k also some boolean variable should be used, not only
for rx_poll_timer but also for other works i.e. tx_complete_work.
Is possible to use SC_OP_INVALID flags, since mac80211 call ath9k_stop
on suspend and ath9k_start on resume.
Indeed however ieee80211_queue_work() already does a suspend check for
us, it just complains as many drivers including mac80211 were setting
up work incorrectly. The warning was put in place to help us find the
issues. Using SC_OP_INVALID seems fair but we could also add a routine
ieee80211_queue_work_safe() that silently fails if we are quiescing or
suspended and not resuming but I can see that creating very sloppy
driver writing and everyone abusing it.
We also have to reliable cancel works on ath9k_stop() if device goes
down for other reason than suspend, new mac80211 ieee80211_queue_work_safe()
routine will not help with that.
@@ -170,7 +170,8 @@ void ath_rx_poll(unsigned long data){structath_softc*sc=(structath_softc*)data;-ieee80211_queue_work(sc->hw,&sc->hw_check_work);+if(!test_bit(SC_OP_INVALID,&sc->sc_flags))+ieee80211_queue_work(sc->hw,&sc->hw_check_work);}
That looks ok for me as -stable fix
Reviewed-by: Stanislaw Gruszka <redacted>
Stanislaw
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html