On 08/18/2016 08:01 PM, Manoharan, Rajkumar wrote:
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index 916119c..d96c06e 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -4307,8 +4307,8 @@ void ath10k_mac_tx_push_pending(struct ath10k *ar) int max; int loop_max = 2000; - spin_lock_bh(&ar->txqs_lock); rcu_read_lock(); + spin_lock_bh(&ar->txqs_lock);Ben, It is quite possible that push_pending can be preempted after acquiring rcu_lock which may lead to deadlock. no? I assume to prevent that spin_lock is taken first. Could you please explain how this reordering is fixing dead lock?
It did not obviously fix the spin lock issue, but the issue went away. Maybe it was because I fixed the stale memory access issues at around the same time. But, I don't think you can deadlock by taking rcu lock first and then the spinlock. I checked all places where the spinlock is held, and I do not see any way for any of them to block forever (In my kernel, I have a 2000 time limit on spinning in the push pending method, which can help make sure we don't spin forever). http://dmz2.candelatech.com/?p=linux-4.7.dev.y/.git;a=commitdiff;h=5d192657269d8e20fce733f894bb1b68df71db00 I was also worried that some calls from mac80211 might be holding rcu_read_lock while calling into ath10k code that would grab the spinlock. If that is the case (and I didn't verify it was), then you could have a lock inversion by taking spinlock before rcu read lock in the push-pending method. Anyway, someone that understands locking subtleties better might have more clue about this code than I do. Thanks, Ben -- Ben Greear <greearb@xxxxxxxxxxxxxxx> Candela Technologies Inc http://www.candelatech.com
