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

[Elias Oltmanns <eo@nebensachen.de>]

"Bob Copeland" <me@bobcopeland.com> wrote:
> On Thu, 2 Oct 2008 14:37:17 -0400, Bob Copeland wrote
>> On Thu, Oct 2, 2008 at 12:31 PM, Elias Oltmanns <eo@nebensachen.de> wrote:
>
>> > Sorry, but I don't think this is safe. Checking and restoring the
>> > started flag has to be protected too, otherwise there can be races
>> > against ->stop().
>> 
>> Yes, it's a bit of a mess.  Even if it were serialized, a ->stop()
>> happening between suspend's call to hw_stop and actually powering down
>> the device would clear the flag.  Ho hum.
>
> Okay, as usual I'm wrong here; it will clear the flag but we don't care
> since then we just wouldn't power up on resume.
>
> Since no one else chimed in, here's take two with more chewing gum and 
> baling wire to fix the suspend/stop race.
>
> ---
>  drivers/net/wireless/ath5k/base.c |   24 +++++++++++++++++-------
>  drivers/net/wireless/ath5k/base.h |    3 ++-
>  2 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c
> index e09ed2c..7e8fa2e 100644
> --- a/drivers/net/wireless/ath5k/base.c
> +++ b/drivers/net/wireless/ath5k/base.c
[...]
> @@ -666,9 +666,12 @@ ath5k_pci_resume(struct pci_dev *pdev)
>  		goto err_no_irq;
>  	}
>  
> -	err = ath5k_init(sc);
> -	if (err)
> -		goto err_irq;
> +	if (test_bit(ATH_STAT_STARTED, sc->status)) {
> +		err = ath5k_init(sc);

I still feel uneasy about this. Granted, I haven't thought this through
too carefully, but I'd rather not rely on the fact that ath5k_stop_hw()
will not get called between the check for ATH_STAT_STARTED and the call
to ath5k_init if I can help it. Perhaps you can add an argument `reinit'
to ath5k_init() and do something like this under the mutex:

	if (reinit && !test_bit(ATH_STAT_STARTED, sc->status)) {
		mutex_unlock(...);
		return;
	}

Regards,

Elias