[PATCH 1/3] wcn36xx: check for DMA mapping errors in wcn36xx_dxe_tx_frame()

[PATCH 1/3] wcn36xx: check for DMA mapping errors in wcn36xx_dxe_tx_frame()

[Daniel Mack]

Bail out if the mapping fails. Even though this hasn't occured during
tests, this unlikely case should still be handled.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/net/wireless/ath/wcn36xx/dxe.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
index 6e9a3583c447..e8ad8f989ccd 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.c
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
@@ -707,6 +707,11 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
 					  ctl->skb->data,
 					  ctl->skb->len,
 					  DMA_TO_DEVICE);
+	if (dma_mapping_error(wcn->dev, desc->src_addr_l)) {
+		dev_err(wcn->dev, "unable to DMA map src_addr_l\n");
+		ret = -ENOMEM;
+		goto unlock;
+	}
 
 	desc->dst_addr_l = ch->dxe_wq;
 	desc->fr_len = ctl->skb->len;
-- 
2.14.3

[PATCH 2/3] wcn36xx: don't keep reference to skb if transmission failed

[Daniel Mack]

When wcn36xx_dxe_tx_frame() fails to transmit the TX frame, the driver
will call into ieee80211_free_txskb() for the skb in flight, so it'll no
longer be valid. Hence, we shouldn't keep a reference to it in ctl->skb.
Also, if the skb has IEEE80211_TX_CTL_REQ_TX_STATUS set, a pointer to
it will currently remain in wcn->tx_ack_skb, which will potentially lead
to a crash if accessed later.

Fix this by checking the return value of wcn36xx_dxe_tx_frame(), and
nullify wcn->tx_ack_skb again in case of errors. Move the assignment
of ctl->skb in wcn36xx_dxe_tx_frame() so it only happens when the
transmission is successful.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/net/wireless/ath/wcn36xx/dxe.c  |  6 +++---
 drivers/net/wireless/ath/wcn36xx/txrx.c | 15 ++++++++++++++-
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
index e8ad8f989ccd..abf7b051e1ff 100644
--- a/drivers/net/wireless/ath/wcn36xx/dxe.c
+++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
@@ -695,7 +695,6 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
 
 	/* Set source address of the SKB we send */
 	ctl = ctl->next;
-	ctl->skb = skb;
 	desc = ctl->desc;
 	if (ctl->bd_cpu_addr) {
 		wcn36xx_err("bd_cpu_addr cannot be NULL for skb DXE\n");
@@ -704,8 +703,8 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
 	}
 
 	desc->src_addr_l = dma_map_single(wcn->dev,
-					  ctl->skb->data,
-					  ctl->skb->len,
+					  skb->data,
+					  skb->len,
 					  DMA_TO_DEVICE);
 	if (dma_mapping_error(wcn->dev, desc->src_addr_l)) {
 		dev_err(wcn->dev, "unable to DMA map src_addr_l\n");
@@ -713,6 +712,7 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
 		goto unlock;
 	}
 
+	ctl->skb = skb;
 	desc->dst_addr_l = ch->dxe_wq;
 	desc->fr_len = ctl->skb->len;
 
diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c
index b1768ed6b0be..a6902371e89c 100644
--- a/drivers/net/wireless/ath/wcn36xx/txrx.c
+++ b/drivers/net/wireless/ath/wcn36xx/txrx.c
@@ -273,6 +273,7 @@ int wcn36xx_start_tx(struct wcn36xx *wcn,
 	bool bcast = is_broadcast_ether_addr(hdr->addr1) ||
 		is_multicast_ether_addr(hdr->addr1);
 	struct wcn36xx_tx_bd bd;
+	int ret;
 
 	memset(&bd, 0, sizeof(bd));
 
@@ -317,5 +318,17 @@ int wcn36xx_start_tx(struct wcn36xx *wcn,
 	buff_to_be((u32 *)&bd, sizeof(bd)/sizeof(u32));
 	bd.tx_bd_sign = 0xbdbdbdbd;
 
-	return wcn36xx_dxe_tx_frame(wcn, vif_priv, &bd, skb, is_low);
+	ret = wcn36xx_dxe_tx_frame(wcn, vif_priv, &bd, skb, is_low);
+	if (ret && bd.tx_comp) {
+		/* If the skb has not been transmitted,
+		 * don't keep a reference to it.
+		 */
+		spin_lock_irqsave(&wcn->dxe_lock, flags);
+		wcn->tx_ack_skb = NULL;
+		spin_unlock_irqrestore(&wcn->dxe_lock, flags);
+
+		ieee80211_wake_queues(wcn->hw);
+	}
+
+	return ret;
 }
-- 
2.14.3

[PATCH 3/3] wcn36xx: don't delete invalid bss indices

[Daniel Mack]

The firmware code cannot cope with requests to remove BSS indices that have
not previously been added. This primarily happens when the device is
suspended and then resumed. ieee80211_reconfig() then calls into
wcn36xx_bss_info_changed() with an empty bssid and BSS_CHANGED_BSSID set,
which subsequently leads to a firmware crash:

[   43.647928] qcom-wcnss-pil a204000.wcnss: fatal error received: halMsg.c:4964:halMsg_DelBss: Invalid BSSIndex 0
[   43.647959] remoteproc remoteproc0: crash detected in a204000.wcnss: type fatal error

To fix this, set bss_index to WCN36XX_HAL_BSS_INVALID_IDX for all bss
that have not been configured in the firmware, and don't call into the
firmware with invalid indices.

Signed-off-by: Daniel Mack <daniel@zonque.org>
---
 drivers/net/wireless/ath/wcn36xx/main.c | 1 +
 drivers/net/wireless/ath/wcn36xx/smd.c  | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index 69d6be59d97f..32bbd6e2fd09 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -953,6 +953,7 @@ static int wcn36xx_add_interface(struct ieee80211_hw *hw,
 
 	mutex_lock(&wcn->conf_mutex);
 
+	vif_priv->bss_index = WCN36XX_HAL_BSS_INVALID_IDX;
 	list_add(&vif_priv->list, &wcn->vif_list);
 	wcn36xx_smd_add_sta_self(wcn, vif);
 
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 8932af5e4d8d..5be07e40a86d 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -1446,6 +1446,10 @@ int wcn36xx_smd_delete_bss(struct wcn36xx *wcn, struct ieee80211_vif *vif)
 	int ret = 0;
 
 	mutex_lock(&wcn->hal_mutex);
+
+	if (vif_priv->bss_index == WCN36XX_HAL_BSS_INVALID_IDX)
+		goto out;
+
 	INIT_HAL_MSG(msg_body, WCN36XX_HAL_DELETE_BSS_REQ);
 
 	msg_body.bss_index = vif_priv->bss_index;
@@ -1464,6 +1468,8 @@ int wcn36xx_smd_delete_bss(struct wcn36xx *wcn, struct ieee80211_vif *vif)
 		wcn36xx_err("hal_delete_bss response failed err=%d\n", ret);
 		goto out;
 	}
+
+	vif_priv->bss_index = WCN36XX_HAL_BSS_INVALID_IDX;
 out:
 	mutex_unlock(&wcn->hal_mutex);
 	return ret;
-- 
2.14.3

Re: [PATCH 3/3] wcn36xx: don't delete invalid bss indices

[Ramon Fried]

On 4/3/2018 7:51 PM, Daniel Mack wrote:
> The firmware code cannot cope with requests to remove BSS indices that have
> not previously been added. This primarily happens when the device is
> suspended and then resumed. ieee80211_reconfig() then calls into
> wcn36xx_bss_info_changed() with an empty bssid and BSS_CHANGED_BSSID set,
> which subsequently leads to a firmware crash:
>
> [   43.647928] qcom-wcnss-pil a204000.wcnss: fatal error received: halMsg.c:4964:halMsg_DelBss: Invalid BSSIndex 0
> [   43.647959] remoteproc remoteproc0: crash detected in a204000.wcnss: type fatal error
>
> To fix this, set bss_index to WCN36XX_HAL_BSS_INVALID_IDX for all bss
> that have not been configured in the firmware, and don't call into the
> firmware with invalid indices.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  drivers/net/wireless/ath/wcn36xx/main.c | 1 +
>  drivers/net/wireless/ath/wcn36xx/smd.c  | 6 ++++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
> index 69d6be59d97f..32bbd6e2fd09 100644
> --- a/drivers/net/wireless/ath/wcn36xx/main.c
> +++ b/drivers/net/wireless/ath/wcn36xx/main.c
> @@ -953,6 +953,7 @@ static int wcn36xx_add_interface(struct ieee80211_hw *hw,
>  
>  	mutex_lock(&wcn->conf_mutex);
>  
> +	vif_priv->bss_index = WCN36XX_HAL_BSS_INVALID_IDX;
>  	list_add(&vif_priv->list, &wcn->vif_list);
>  	wcn36xx_smd_add_sta_self(wcn, vif);
>  
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 8932af5e4d8d..5be07e40a86d 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -1446,6 +1446,10 @@ int wcn36xx_smd_delete_bss(struct wcn36xx *wcn, struct ieee80211_vif *vif)
>  	int ret = 0;
>  
>  	mutex_lock(&wcn->hal_mutex);
> +
> +	if (vif_priv->bss_index == WCN36XX_HAL_BSS_INVALID_IDX)
> +		goto out;
> +
>  	INIT_HAL_MSG(msg_body, WCN36XX_HAL_DELETE_BSS_REQ);
>  
>  	msg_body.bss_index = vif_priv->bss_index;
> @@ -1464,6 +1468,8 @@ int wcn36xx_smd_delete_bss(struct wcn36xx *wcn, struct ieee80211_vif *vif)
>  		wcn36xx_err("hal_delete_bss response failed err=%d\n", ret);
>  		goto out;
>  	}
> +
> +	vif_priv->bss_index = WCN36XX_HAL_BSS_INVALID_IDX;
>  out:
>  	mutex_unlock(&wcn->hal_mutex);
>  	return ret;
Interesting. I have never seen this bug before.
Do you have a way of recreating it so I can test it on my side ?

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] wcn36xx: don't delete invalid bss indices

[Daniel Mack]

On Wednesday, April 04, 2018 07:40 AM, Ramon Fried wrote:
> On 4/3/2018 7:51 PM, Daniel Mack wrote:
>> The firmware code cannot cope with requests to remove BSS indices that have
>> not previously been added. This primarily happens when the device is
>> suspended and then resumed. ieee80211_reconfig() then calls into
>> wcn36xx_bss_info_changed() with an empty bssid and BSS_CHANGED_BSSID set,
>> which subsequently leads to a firmware crash:
>>
>> [   43.647928] qcom-wcnss-pil a204000.wcnss: fatal error received: halMsg.c:4964:halMsg_DelBss: Invalid BSSIndex 0
>> [   43.647959] remoteproc remoteproc0: crash detected in a204000.wcnss: type fatal error
>>
>> To fix this, set bss_index to WCN36XX_HAL_BSS_INVALID_IDX for all bss
>> that have not been configured in the firmware, and don't call into the
>> firmware with invalid indices.
>>
>> Signed-off-by: Daniel Mack <daniel@zonque.org>

> Interesting. I have never seen this bug before.
> Do you have a way of recreating it so I can test it on my side ?

I tested this by putting the machine to suspend with

  # echo freeze >/sys/power/state

right after boot, without connecting to a network before. Resume will
then fail without this patch. I haven't see it in any other cases either
though.


Thanks,
Daniel

Re: [PATCH 1/3] wcn36xx: check for DMA mapping errors in wcn36xx_dxe_tx_frame()

[Ramon Fried]

On 4/3/2018 7:51 PM, Daniel Mack wrote:
> Bail out if the mapping fails. Even though this hasn't occured during
> tests, this unlikely case should still be handled.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
>  drivers/net/wireless/ath/wcn36xx/dxe.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/dxe.c b/drivers/net/wireless/ath/wcn36xx/dxe.c
> index 6e9a3583c447..e8ad8f989ccd 100644
> --- a/drivers/net/wireless/ath/wcn36xx/dxe.c
> +++ b/drivers/net/wireless/ath/wcn36xx/dxe.c
> @@ -707,6 +707,11 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
>  					  ctl->skb->data,
>  					  ctl->skb->len,
>  					  DMA_TO_DEVICE);
> +	if (dma_mapping_error(wcn->dev, desc->src_addr_l)) {
> +		dev_err(wcn->dev, "unable to DMA map src_addr_l\n");
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
>  
>  	desc->dst_addr_l = ch->dxe_wq;
>  	desc->fr_len = ctl->skb->len;
I have the exact patch ready for submission, you got a head of me :)
Acked-by: Ramon Fried <rfried@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [1/3] wcn36xx: check for DMA mapping errors in wcn36xx_dxe_tx_frame()

[Kalle Valo]

Daniel Mack <daniel@zonque.org> wrote:

> Bail out if the mapping fails. Even though this hasn't occured during
> tests, this unlikely case should still be handled.
> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> Acked-by: Ramon Fried <rfried@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

3 patches applied to ath-next branch of ath.git, thanks.

7cae35199bee wcn36xx: check for DMA mapping errors in wcn36xx_dxe_tx_frame()
271f1e65ff38 wcn36xx: don't keep reference to skb if transmission failed
2edfcf2b303c wcn36xx: don't delete invalid bss indices

-- 
https://patchwork.kernel.org/patch/10321545/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches