Bob Copeland <me@xxxxxxxxxxxxxxx> wrote:
[...]
> Subject: [PATCH] ath5k: correct hardware startup sequence in resume
>
> Based on a patch by Elias Oltmanns, we call ath5k_init in resume even
> if we didn't previously open the device. Besides starting up the
> device unnecessarily, this also causes an oops on rmmod because
> mac80211 will not invoke ath5k_stop and softirqs are left running after
> the module has been unloaded. Add a new state bit, ATH_STAT_STARTED,
> to indicate that we have been started up.
>
> Signed-off-by: Bob Copeland <me@xxxxxxxxxxxxxxx>
> ---
> drivers/net/wireless/ath5k/base.c | 28 ++++++++++++++++++++--------
> drivers/net/wireless/ath5k/base.h | 3 ++-
> 2 files changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
> index c151588..5388de8 100644
> --- a/drivers/net/wireless/ath5k/base.c
> +++ b/drivers/net/wireless/ath5k/base.c
[...]
> @@ -2248,7 +2257,7 @@ ath5k_stop_locked(struct ath5k_softc *sc)
> * stop is preempted).
> */
> static int
> -ath5k_stop_hw(struct ath5k_softc *sc)
> +ath5k_stop_hw(struct ath5k_softc *sc, bool update_status)
^^^^^^^^^^^^^
That should be is_suspend for the sake of consistency.
> {
> int ret;
>
> @@ -2280,6 +2289,9 @@ ath5k_stop_hw(struct ath5k_softc *sc)
> }
> ath5k_txbuf_free(sc, sc->bbuf);
> mmiowb();
> +
> + if (update_status)
> + __clear_bit(ATH_STAT_STARTED, sc->status);
This cannot possibly be right. The condition has to be
if (!is_suspend)
> mutex_unlock(&sc->lock);
>
> del_timer_sync(&sc->calib_tim);
> @@ -2676,12 +2688,12 @@ ath5k_reset_wake(struct ath5k_softc *sc)
>
> static int ath5k_start(struct ieee80211_hw *hw)
> {
> - return ath5k_init(hw->priv);
> + return ath5k_init(hw->priv, false);
> }
>
> static void ath5k_stop(struct ieee80211_hw *hw)
> {
> - ath5k_stop_hw(hw->priv);
> + ath5k_stop_hw(hw->priv, false);
> }
>
> static int ath5k_add_interface(struct ieee80211_hw *hw,
Looking through the code, I'm wondering about the atomicity requirements
of sc->status. In my opinion, __set_bit() is not permissible in various
places (including your use case). But since this is a problem that has
been around before, I will send a separate patch once yours has been
merged.
Regards,
Elias
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
