Re: [PATCH V2 1/2] ath6kl: Fix race in aggregation reorder logic

[Kalle Valo <kvalo@qca.qualcomm.com>]

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.

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.
	 */

> I'll change this.

Thanks.

Kalle