On Tuesday 29 May 2012 05:01 PM, Kalle Valo wrote:
On 05/29/2012 02:21 PM, Vasanthakumar Thiagarajan wrote:
On Tuesday 29 May 2012 04:43 PM, Kalle Valo wrote:
On 05/25/2012 01:19 PM, Vasanthakumar Thiagarajan wrote:
@@ -1188,6 +1189,7 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid,
rxtid->progress = true;
else
for (idx = 0 ; idx< rxtid->hold_q_sz; idx++) {
+ spin_lock_bh(&rxtid->lock);
if (rxtid->hold_q[idx].skb) {
/*
* There is a frame in the queue and no
@@ -1201,8 +1203,10 @@ static bool aggr_process_recv_frm(struct aggr_info_conn *agg_conn, u8 tid,
HZ * (AGGR_RX_TIMEOUT) / 1000));
rxtid->progress = false;
rxtid->timer_mon = true;
+ spin_unlock_bh(&rxtid->lock);
break;
}
+ spin_unlock_bh(&rxtid->lock);
}
Here you take and release the lock multiple times inside the loop. Why
not take the lock before the loop?
Sounds better to acquire the lock before loop and releasing it after
the loop. I was kind of thinking about protecting the data only
in critical section, but in this case we can bring the loop
in the lock, it is not going to make much difference.
It's also the question of does the lock protect hold_q_sz.
No, it does not.
Also, can you please fix the comment I added to core.h about the lock
and document properly what the lock is actually supposed to protect:
/*
* FIXME: No clue what this should protect. Apparently it should
* protect some of the fields above but they are also accessed
* without taking the lock.
*/
Good point, thanks.
Vasanth
--
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
