Re: [ath5k-devel] Oops with current kernel and ath5k

[Elias Oltmanns <eo@nebensachen.de>]

Bob Copeland <me@bobcopeland.com> 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@bobcopeland.com>
> ---
>  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