[PATCH v3] ath10k: fix Rx aggregation reordering

[Michal Kazior]

On 16 July 2014 14:35, Michal Kazior <michal.kazior at tieto.com> wrote:
> On 16 July 2014 13:38, Varka Bhadram <varkabhadram at gmail.com> wrote:
>> On 07/16/2014 04:49 PM, Michal Kazior wrote:
>>
>> (...)
>>
>>
>>> +static void ath10k_htt_rx_addba(struct ath10k *ar, struct htt_resp *resp)
>>> +{
>>> +       struct htt_rx_addba *ev = &resp->rx_addba;
>>> +       struct ath10k_peer *peer;
>>> +       struct ath10k_vif *arvif;
>>> +       u16 info0, tid, peer_id;
>>> +
>>> +       info0 = __le32_to_cpu(ev->info0);
>>> +       tid = MS(info0, HTT_RX_BA_INFO0_TID);
>>> +       peer_id = MS(info0, HTT_RX_BA_INFO0_PEER_ID);
>>> +
>>> +       ath10k_dbg(ATH10K_DBG_HTT,
>>> +                  "htt rx addba tid %hu peer_id %hu size %hhu\n",
>>> +                  tid, peer_id, ev->window_size);
>>> +
>>> +       spin_lock_bh(&ar->data_lock);
>>> +       peer = ath10k_peer_find_by_id(ar, peer_id);
>>> +       if (!peer) {
>>> +               ath10k_warn("received addba event for invalid peer_id:
>>> %hu\n",
>>> +                           peer_id);
>>> +               spin_unlock_bh(&ar->data_lock);
>>> +               return;
>>> +       }
>>
>>
>> Here my concern in amount of time holding the lock...
>>
>> spin_lock_bh(&ar->data_lock);
>> peer = ath10k_peer_find_by_id(ar, peer_id);
>> if (!peer) {
>>         spin_unlock_bh(&ar->data_lock);
>>
>>         ath10k_warn("received addba event for invalid peer_id: %hu\n",
>>                     peer_id);
>>         return;
>> }
>>
>> No need to of putting the debug message inside the critical region...  :-)
>
> Sounds reasonable in this case as I'm not printing spinlock-protected values.

..and I realized this isn't true upon hitting the send button.

The other print uses peer->vdev_id. The peer was acquired under a lock
and must not be used after the lock is released. It'll just look
confusing if I mix ordering of unlock/print in some cases so I'll
leave it as is.

(sorry for the noise)


Michał