On 23 May 2014 10:45, Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> wrote:
> Michal Kazior <michal.kazior@xxxxxxxxx> writes:
>
>> On 23 May 2014 10:27, Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> wrote:
>>> Michal Kazior <michal.kazior@xxxxxxxxx> 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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
