[ath9k-devel] [PATCH 1/2] ath10k: simplify txi lifespan

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

Michal Kazior <michal.kazior@tieto.com> writes:

> Up until now we were using two booleans to keep
> track of htt and htc tx completions. These can
> come in no specific order.
>
> Keep in mind that it was safe to access those
> without any locks since tx completion processing
> is done one endpoint at a time (that's the current
> PCI backend implementation).
>
> The new approach uses refcount. Since we need 2
> completions we setup refcount=2 for mgmt and
> data tx. This also depends on the tx completion
> behaviour.
>
> If the PCI tx completion scheme ever changes we'll
> need locking to synchronize tx completions for
> htt.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Few comments, can you send a followup patch to address those separately,
please?

[...]

>  int ath10k_htt_h2t_ver_req_msg(struct htt_struct *htt)
> @@ -315,6 +277,7 @@ int ath10k_htt_mgmt_tx(struct htt_struct *htt, struct sk_buff *msdu)
>  
>  	skb_cb = ATH10K_SKB_CB(txi->txdesc);
>  	skb_cb->htt.msdu_id = txi->msdu_id;
> +	skb_cb->htt.refcount = 2;
>  
>  	res = ath10k_htc_send(htt->htc, htt->eid, txi->txdesc);
>  	if (res)
> @@ -436,6 +399,7 @@ int ath10k_htt_tx(struct htt_struct *htt, struct sk_buff *msdu)
>  
>  	skb_cb = ATH10K_SKB_CB(txi->txdesc);
>  	skb_cb->htt.msdu_id = txi->msdu_id;
> +	skb_cb->htt.refcount = 2;

It would be good to have a comment in the code why refcount needs to be
2. I know you had it in the commit log already, but people go through
commit logs only when they are really desparate :)


[...]

> --- a/drivers/net/wireless/ath/ath10k/txrx.c
> +++ b/drivers/net/wireless/ath/ath10k/txrx.c
> @@ -44,22 +44,17 @@ out:
>  	spin_unlock_bh(&ar->data_lock);
>  }
>  
> -void ath10k_txrx_tx_completed(struct htt_struct *htt,
> -			      const struct htt_tx_done *tx_done)
> +void ath10k_txrx_tx_unref(struct htt_struct *htt, struct htt_tx_info *txi)
>  {
>  	struct device *dev = htt->ar->dev;
>  	struct ieee80211_tx_info *info;
> -	struct htt_tx_info *txi;
>  	int ret;
>  
> -	ath10k_dbg(ATH10K_DBG_HTT, "htt tx completion msdu_id %u discard %d no_ack %d\n",
> -		   tx_done->msdu_id, !!tx_done->discard, !!tx_done->no_ack);
> +	if (ATH10K_SKB_CB(txi->txdesc)->htt.refcount == 0)
> +		return;
>  
> -	txi = ath10k_htt_tx_info_lookup(htt, tx_done->msdu_id);
> -	if (!txi) {
> -		ath10k_dbg(ATH10K_DBG_HTT, "htt txi lookup fail\n");
> +	if (--ATH10K_SKB_CB(txi->txdesc)->htt.refcount > 0)
>  		return;
> -	}

The 'if (--foo > 0)' is correct but IMHO bad style. It's easy to miss
the operator in front and that's why I would prefer:

foo--;

if (foo > 0)
   ...

-- 
Kalle Valo