Re: [PATCH v7 2/4] ath9k: always call ath_reset from workqueue context

[Felix Fietkau <nbd@openwrt.org>]

On 2011-08-30 7:26 AM, Vasanthakumar Thiagarajan wrote:
> On Mon, Aug 29, 2011 at 08:40:43PM +0200, Felix Fietkau wrote:
>>  This makes it much easier to add further rework to avoid race conditions
>>  between reset and other work items.
>>  Move other functions to make ath_reset static.
>
> Can you please explain where you'll run into race during
> ath_reset()?. It's always called with pcu_lock held.
I didn't actually observe any race, but I'm not sure if all work items 
can properly deal with being interrupted by a full reset, so I'm trying 
to make it more safe here.

>>  @@ -386,9 +386,7 @@ void ath_beacon_tasklet(unsigned long data)
>>   			ath_dbg(common, ATH_DBG_BSTUCK,
>>   				"beacon is officially stuck\n");
>>   			sc->sc_flags |= SC_OP_TSF_RESET;
>>  -			spin_lock(&sc->sc_pcu_lock);
>>  -			ath_reset(sc, true);
>>  -			spin_unlock(&sc->sc_pcu_lock);
>>  +			ieee80211_queue_work(sc->hw,&sc->hw_reset_work);
>
> We'll expect a reset happening as soon possible here. Handling this
> in a work item may not be appropriate.
We've already waited for multiples of 100ms. 1-2ms extra won't hurt now.

>>   void ath9k_tasklet(unsigned long data)
>>   {
>>  @@ -675,9 +607,7 @@ void ath9k_tasklet(unsigned long data)
>>
>>   	if ((status&  ATH9K_INT_FATAL) ||
>>   	(status&  ATH9K_INT_BB_WATCHDOG)) {
>>  -		spin_lock(&sc->sc_pcu_lock);
>>  -		ath_reset(sc, true);
>>  -		spin_unlock(&sc->sc_pcu_lock);
>>  +		ieee80211_queue_work(sc->hw,&sc->hw_reset_work);
>
> Above comment applies here as well.
I don't think the additional delay will be significant enough to matter 
here. What matters to me more is to be able to reliably kill all the 
currently running work, which can only be done if ath_reset gets called 
from process context.

- Felix