[PATCH v2 3/5] ath10k: drain tx before restarting hw

[Michal Kazior]

On 23 May 2014 10:45, Kalle Valo <kvalo at qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior at tieto.com> writes:
>
>> On 23 May 2014 10:27, Kalle Valo <kvalo at qca.qualcomm.com> wrote:
>>> Michal Kazior <michal.kazior at tieto.com> writes:
>>>
>>>> +static void ath10k_drain_tx(struct ath10k *ar)
>>>> +{
>>>> +     /* workers can hold conf_mutex -- avoid deadlock */
>>>> +     WARN_ON(lockdep_is_held(&ar->conf_mutex));
>>>
>>> If CONFIG_LOCKDEP is disabled this will fail with:
>>>
>>> drivers/net/wireless/ath/ath10k/mac.c:2301:2: error: implicit declaration of function 'lockdep_is_held' [-Werror=implicit-function-declaration]
>>>
>>> I wasn't able to come up with any other way than adding '#ifdef
>>> CONFIG_LOCKDEP' which is quite ugly. What should we do?
>>
>> I guess we could define lockdep_assert_not_held() in debug.h?
>
> Sure, but I still think it's a bit ugly. The right way to fix this would
> be to add it to include/lockdep.h, instead of adding custom checks to a
> driver.

Good point. I wonder if it's generic enough to justify.


> But does this check even make sense? There's nothing preventing
> to another thread to take lock just after the WARN_ON(), right?

There's nothing wrong with other thread holding it. Actually that's
the reason for this very check.

The point is to prevent ath10k_drain_tx() being called while caller
(current thread) holds conf_mutex. If it were to hold conf_mutex then
cancel_work_sync() can deadlock as both workers it tries to stop try
to get a hold of the lock too.


> Oh, and if we add it to debug.h we should name ath10k_assert_not_held().
> We should not use lockdep's namespace for anything inside ath10k.

Good point.


Michał