Michal Kazior <michal.kazior@xxxxxxxxx> writes:
> On 10 April 2014 09:18, Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> wrote:
>> Michal Kazior <michal.kazior@xxxxxxxxx> writes:
>>
>>> The idea is you need BOTH locks to modify the list structure, but you
>>> need only one of them to iterate over the list safely and
>>> consistently. This means writer will not alter the list structure
>>> until there are no readers.
>>
>> Still not understanding this. Why not then use conf_mutex always, why do
>> we need data_lock at all?
>
> htt peer map/unmap which is in tasklet context needs to iterate over
> the list. It can't take conf_mutex.
>
>> Or are you saying that one can iterate the list by either taking
>> conf_mutex or by taking data_lock, depending on context?
>
> Yes.
Thanks, finally I understand this. At least the comment below needs to
be changed to cover all cases:
--- a/drivers/net/wireless/ath/ath10k/txrx.c
+++ b/drivers/net/wireless/ath/ath10k/txrx.c
@@ -100,13 +100,13 @@ exit:
wake_up(&htt->empty_tx_wq);
}
+/* hold conf_mutex for simple iteration, or conf_mutex+data_lock for
+ * modifications */
struct ath10k_peer *ath10k_peer_find(struct ath10k *ar, int vdev_id,
const u8 *addr)
{
struct ath10k_peer *peer;
- lockdep_assert_held(&ar->data_lock);
-
list_for_each_entry(peer, &ar->peers, list) {
if (peer->vdev_id != vdev_id)
continue;
The other thing is that I really like the idea of documenting locking
requirements with lockdep_assert_held() and I hate to lose that in this
function. I started to think should we come up with our own macro for
checking if either of two locks is held? When I look at the original
macro I think it should be pretty easy:
#define lockdep_assert_held(l) do { \
WARN_ON(debug_locks && !lockdep_is_held(l)); \
} while (0)
--
Kalle Valo
--
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
