[PATCH v2] ath10k: move mgmt descriptor limit handle under mgmt_tx

[Rajkumar Manoharan]

On , Michal Kazior wrote:
> On 6 March 2016 at 08:47, Rajkumar Manoharan 
> <rmanohar at qti.qualcomm.com> wrote:
>> Firmware reserves few descriptors for management frames transmission.
>> In 16 MBSSID scenario, these slots will be easy exhausted due to 
>> frequent
>> probe responses. So for 10.4 based solutions, probe responses are 
>> limited
>> by a threshold (24).
>> 
>> management tx path is separate for all except tlv based solutions. 
>> Since
>> tlv solutions (qca6174 & qca9377) do not support 16 AP interfaces, it 
>> is
>> safe to move management descriptor limitation check under mgmt_tx
>> function. Though CPU improvement is negligible, unlikely conditions or
>> never hit conditions in hot path can be avoided on data transmission.
>> 
>> Signed-off-by: Rajkumar Manoharan <rmanohar at qti.qualcomm.com>
>> ---
>> v2:
>>   - rebased on top of Michal changes
> [...]
>> @@ -3979,13 +3977,22 @@ static void ath10k_mac_op_tx(struct 
>> ieee80211_hw *hw,
>>         is_htt = (txpath == ATH10K_MAC_TX_HTT ||
>>                   txpath == ATH10K_MAC_TX_HTT_MGMT);
>> 
>> -       if (is_htt) {
>> -               spin_lock_bh(&ar->htt.tx_lock);
>> -
>> -               is_mgmt = ieee80211_is_mgmt(hdr->frame_control);
>> +       is_mgmt = ieee80211_is_mgmt(hdr->frame_control);
>> +       spin_lock_bh(&ar->htt.tx_lock);
>> +       if (is_mgmt) {
>>                 is_presp = 
>> ieee80211_is_probe_resp(hdr->frame_control);
>> 
>> -               ret = ath10k_htt_tx_inc_pending(htt, is_mgmt, 
>> is_presp);
>> +               ret = ath10k_htt_tx_mgmt_inc_pending(htt, is_presp);
>> +               if (ret) {
>> +                       ath10k_warn(ar, "failed to increase tx mgmt 
>> pending count: %d, dropping\n",
>> +                                   ret);
>> +                       spin_unlock_bh(&ar->htt.tx_lock);
>> +                       ieee80211_free_txskb(ar->hw, skb);
>> +                       return;
>> +               }
>> +       }
>> +       if (is_htt) {
>> +               ret = ath10k_htt_tx_inc_pending(htt);
>>                 if (ret) {
>>                         ath10k_warn(ar, "failed to increase tx pending 
>> count: %d, dropping\n",
>>                                     ret);
> 
> This doesn't look good.
> 
> You'll call ath10k_htt_tx_mgmt_inc_pending() regardless of the tx path
> being chosen. FWIW It could be WMI on older firmware, oops.
> 
Good catch.. Since inc_pending is moved out of htt_tx, i missed to 
handle it for htt alone.


> Holding the lock for the entire time doesn't make much sense either, 
> does it?
> 
> I think it should be more like:
> 
>  if (is_htt) {
>     is_mgmt = ..;
>     is_presp = ..;
> 
>     lock()
>     ret = inc_pending(htt);
>     if (ret) { unlock(); goto drop }
> 
>     ret = mgmt_inc_pending(htt, is_mgmt, is_presp);
>     if (ret) { dec_pending(htt); unlock(); goto drop }
> 
>     unlock();
>  }
>  ...
>  ret = mac_tx()
>  if (ret) {
>    if (is_htt) {
>      lock();
>      dec_pending();
>      if (is_mgmt)
>         mgmt_dec_pending()
>      unlock();
>  }
> 
> no?
> 
yes.. now it looks good. thanks for your comments

-Rajkumar