Re: [PATCH 3/3] ath10k: speed up hw recovery

[Kalle Valo <kvalo@qca.qualcomm.com>]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 13 October 2014 10:15, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Michal Kazior <michal.kazior@tieto.com> writes:
>>
>>> In some cases hw recovery was taking an absurdly
>>> long time due to ath10k waiting for things that
>>> would never really complete.
>>>
>>> Instead of waiting for inevitable timeouts poke
>>> all completions and wakequeues and check if it's
>>> still worth waiting.
>>>
>>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>>
>> [...]
>>
>>> --- a/drivers/net/wireless/ath/ath10k/core.c
>>> +++ b/drivers/net/wireless/ath/ath10k/core.c
>>> @@ -685,6 +685,20 @@ static void ath10k_core_restart(struct work_struct *work)
>>>  {
>>>       struct ath10k *ar = container_of(work, struct ath10k, restart_work);
>>>
>>> +     set_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags);
>>> +     barrier();
>>
>> Please document why the barrier is needed.
>
> Okay. It's just to make sure compiler doesn't re-order it (so that
> following complete/wake_ups will be called _after_ the bit is set).

Ok. Can you add a short comment to the code saying why you added the
barrier?

>>> --- a/drivers/net/wireless/ath/ath10k/core.h
>>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>>> @@ -371,6 +371,11 @@ enum ath10k_dev_flags {
>>>       /* Indicates that ath10k device is during CAC phase of DFS */
>>>       ATH10K_CAC_RUNNING,
>>>       ATH10K_FLAG_CORE_REGISTERED,
>>> +
>>> +     /* Device has crashed and needs to restart. This indicates any pending
>>> +      * waiters should immediately cancel instead of waiting for a time out.
>>> +      */
>>> +     ATH10K_FLAG_CRASH_FLUSH,
>>>  };
>>
>> Instead of a dev flag, should this actually be a new state to enum
>> ath10k_state? The reason I ask, I don't see how we would use this flag
>> with other states than with ATH10K_STATE_ON. Or is this needed because
>> of locking?
>
> Locking.
>
> Reading/writing ar->state requires conf_mutex. The problem is waitiers
> might be holding it so you wouldn't even be able to tell the waiters
> to not wait anymore.
>
> A long term idea might involve removing conf_mutex though but I recall
> there's a couple of problems with that when I looked at it.

Makes sense. Can you add this to the commit log, please?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k