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

[Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>]

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