On 16 July 2014 13:38, Varka Bhadram <varkabhadram@xxxxxxxxx> 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.
[...]
>> +static int ath10k_ampdu_action(struct ieee80211_hw *hw,
>> + struct ieee80211_vif *vif,
>> + enum ieee80211_ampdu_mlme_action action,
>> + struct ieee80211_sta *sta, u16 tid, u16
>> *ssn,
>> + u8 buf_size)
>> +{
>> + struct ath10k_vif *arvif = ath10k_vif_to_arvif(vif);
>> +
>> + ath10k_dbg(ATH10K_DBG_MAC, "mac ampdu vdev_id %i sta %pM tid %hu
>> action %d\n",
>> + arvif->vdev_id, sta->addr, tid, action);
>> +
>> + switch (action) {
>> + case IEEE80211_AMPDU_RX_START:
>> + case IEEE80211_AMPDU_RX_STOP:
>> + /* HTT AddBa/DelBa events trigger mac80211 Rx BA session
>> + * creation/removal. Do we need to verify this?
>> + */
>> + return 0;
>> + case IEEE80211_AMPDU_TX_START:
>> + case IEEE80211_AMPDU_TX_STOP_CONT:
>> + case IEEE80211_AMPDU_TX_STOP_FLUSH:
>> + case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
>> + case IEEE80211_AMPDU_TX_OPERATIONAL:
>> + /* Firmware offloads Tx aggregation entirely so deny
>> mac80211
>> + * Tx aggregation requests.
>> + */
>> + break;
>
>
> Instead of break here we can directly do: return -EOPNOTSUPP;
True.
Thanks for review. I'll wait a bit more before I re-post v4 (although
there's no rush since the patch needs mac80211 patches to be applied
first).
Michał
--
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
