[PATCH] Add IEEE80211_TX_CTL_REQ_TX_STATUS support

[Kalle Valo]

Pontus Fuchs <pontus.fuchs at gmail.com> writes:

> As there seems to be no packet id in the status indication message
> from the FW, I stop the queues in order to have only one frame with
> IEEE80211_TX_CTL_REQ_TX_STATUS submitted to the FW.
>
> Signed-off-by: Pontus Fuchs <pontus.fuchs at gmail.com>
> ---

[...]

> +void wcn36xx_dxe_tx_ack_ind(struct wcn36xx *wcn, u32 status)
> +{
> +	struct ieee80211_tx_info *info;
> +	struct sk_buff *skb;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wcn->dxe_lock, flags);
> +	skb = wcn->tx_ack_skb;
> +	wcn->tx_ack_skb = NULL;
> +	spin_unlock_irqrestore(&wcn->dxe_lock, flags);
> +
> +	if (!skb) {
> +		wcn36xx_error("Spurious TX complete indication");
> +		return;

wcn36xx_warn() is more approriate here.

> +	}
> +
> +	info = IEEE80211_SKB_CB(skb);
> +	if (status == 1) {
> +		wcn36xx_dbg(WCN36XX_DBG_DXE, "Got positive ack status");
> +		info->flags |= IEEE80211_TX_STAT_ACK;
> +	} else
> +		wcn36xx_dbg(WCN36XX_DBG_DXE, "Got negative ack status");

You can avoid the else branch with this:

wcn36xx_dbg(WCN36XX_DBG_DXE, "dxe tx status %d", status);

if (status == 1)
        info->flags |= IEEE80211_TX_STAT_ACK;

> @@ -478,14 +507,33 @@ int wcn36xx_dxe_tx(struct wcn36xx *wcn,
>  		   struct sk_buff *skb,
>  		   u8 broadcast,
>  		   bool is_high,
> -		   u32 header_len)
> +		   u32 header_len, bool tx_ack)
>  {
>  	struct wcn36xx_dxe_ctl *ctl = NULL;
>  	struct wcn36xx_dxe_desc *desc = NULL;
>  	struct wcn36xx_dxe_ch *ch = NULL;
> +	unsigned long flags;
>  
>  	ch = is_high ? &wcn->dxe_tx_h_ch : &wcn->dxe_tx_l_ch;
>  
> +	if (tx_ack) {
> +		wcn36xx_dbg(WCN36XX_DBG_DXE, "TX_ACK status requested");
> +		spin_lock_irqsave(&wcn->dxe_lock, flags);
> +		if (wcn->tx_ack_skb) {
> +			spin_unlock_irqrestore(&wcn->dxe_lock, flags);
> +			wcn36xx_error("tx_ack_skb already set");
> +			return -EINVAL;

wcn36xx_warn() is better here. And I don't see skb freed when this
happens.

> @@ -682,6 +729,13 @@ void wcn36xx_dxe_deinit(struct wcn36xx *wcn)
>  {
>  	free_irq(wcn->tx_irq, wcn);
>  	free_irq(wcn->rx_irq, wcn);
> +
> +	if (wcn->tx_ack_skb) {
> +		ieee80211_tx_status_irqsafe(wcn->hw, wcn->tx_ack_skb);
> +		ieee80211_wake_queues(wcn->hw);
> +		wcn->tx_ack_skb = NULL;
> +	}

Calling wake_queues() from deinit() looks bad. Is that really the
intention?

> +static int wcn36xx_smd_tx_compl_ind(struct wcn36xx *wcn, void *buf, size_t len)
> +{
> +	struct tx_compl_ind_msg *rsp = (struct tx_compl_ind_msg *)buf;
> +
> +	if (len != sizeof(*rsp)) {
> +		wcn36xx_error("Bad TX complete indication");
> +		return -EIO;
> +	}

Again wcn36xx_warn() is better.

> +
> +	wcn36xx_dxe_tx_ack_ind(wcn, rsp->status);
> +	return 0;

Style: having an empty line before the return makes the code more
readable.

> --- a/wcn36xx.h
> +++ b/wcn36xx.h
> @@ -20,6 +20,7 @@
>  #include <linux/completion.h>
>  #include <linux/printk.h>
>  #include <linux/firmware.h>
> +#include <linux/spinlock.h>
>  #include <linux/workqueue.h>
>  #include <mach/msm_smd.h>
>  #include <net/mac80211.h>
> @@ -149,9 +150,14 @@ struct wcn36xx {
>  	struct wcn36xx_dxe_ch	dxe_rx_l_ch;	/* RX low */
>  	struct wcn36xx_dxe_ch	dxe_rx_h_ch;	/* RX high */
>  
> +	/* DXE lock */
> +	spinlock_t	dxe_lock;
> +
>  	/* Memory pools */
>  	struct wcn36xx_dxe_mem_pool mgmt_mem_pool;
>  	struct wcn36xx_dxe_mem_pool data_mem_pool;
> +
> +	struct sk_buff		*tx_ack_skb;

It would be good to document that tx_ack_skb is serialised with dxe_lock.

-- 
Kalle Valo