wcn36xx: check for DMA mapping errors in wcn36xx_dxe_tx_frame()

PATCH In review Daniel Mack 3 participants 7 messages Apr 3, 2018 – Apr 10, 2018 Qualcomm WCN36XX wireless driver

Share & tools

Daniel Mack

author

2018-04-03 16:52 UTC #0 lore raw

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

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 <[email protected]>

---
 drivers/net/wireless/ath/wcn36xx/dxe.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/net/wireless/ath/wcn36xx/dxe.c +5 -0

index 6e9a3583c447..e8ad8f989ccd 100644
@@ -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;

Signature · 1 line

2.14.3

Daniel Mack

author

2018-04-03 16:52 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/net/wireless/ath/wcn36xx/dxe.c  |  6 +++---
 drivers/net/wireless/ath/wcn36xx/txrx.c | 15 ++++++++++++++-
 2 files changed, 17 insertions(+), 4 deletions(-)

drivers/net/wireless/ath/wcn36xx/dxe.c +3 -3

index e8ad8f989ccd..abf7b051e1ff 100644
@@ -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;

drivers/net/wireless/ath/wcn36xx/txrx.c +14 -1

index b1768ed6b0be..a6902371e89c 100644
@@ -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;
 }

Signature · 1 line

2.14.3

Daniel Mack

author

2018-04-03 16:52 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/net/wireless/ath/wcn36xx/main.c | 1 +
 drivers/net/wireless/ath/wcn36xx/smd.c  | 6 ++++++
 2 files changed, 7 insertions(+)

drivers/net/wireless/ath/wcn36xx/main.c +1 -0

index 69d6be59d97f..32bbd6e2fd09 100644
@@ -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);

drivers/net/wireless/ath/wcn36xx/smd.c +6 -0

index 8932af5e4d8d..5be07e40a86d 100644
@@ -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;

Signature · 1 line

2.14.3

Ramon Fried

2018-04-04 05:40 UTC #3 lore raw

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

Quoted 47 lines · 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 <[email protected]>
---
 drivers/net/wireless/ath/wcn36xx/main.c | 1 +
 drivers/net/wireless/ath/wcn36xx/smd.c  | 6 ++++++
 2 files changed, 7 insertions(+)
drivers/net/wireless/ath/wcn36xx/main.c +1 -0
index 69d6be59d97f..32bbd6e2fd09 100644
@@ -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);
 
drivers/net/wireless/ath/wcn36xx/smd.c +4 -0
index 8932af5e4d8d..5be07e40a86d 100644
@@ -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;

drivers/net/wireless/ath/wcn36xx/smd.c int wcn36xx_smd_delete_bss(struct wcn36xx *wcn, struct ieee80211_vif *vif) · line 1475

Quoted 8 lines

drivers/net/wireless/ath/wcn36xx/smd.c +2 -0

@@ -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 ?

Signature · 2 lines

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

Daniel Mack

author

2018-04-04 06:39 UTC #4 lore raw

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

Quoted 10 lines · Ramon Fried

Quoted 9 lines · 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

Quoted 8 lines

Quoted 5 lines
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 <[email protected]>
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

Ramon Fried

Acked-by on v1 1/3

2018-04-04 05:37 UTC #5 lore raw

In reply to Daniel Mack

Quoted 14 lines · 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 <[email protected]>
---
 drivers/net/wireless/ath/wcn36xx/dxe.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/wireless/ath/wcn36xx/dxe.c +0 -0
index 6e9a3583c447..e8ad8f989ccd 100644
@@ -707,6 +707,11 @@ int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn,
 					  ctl->skb->data,

drivers/net/wireless/ath/wcn36xx/dxe.c int wcn36xx_dxe_tx_frame(struct wcn36xx *wcn, · line 717

Quoted 10 lines

drivers/net/wireless/ath/wcn36xx/dxe.c +5 -0

 					  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 <[email protected]>

Signature · 2 lines

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

Kalle Valo

2018-04-10 14:38 UTC #6 lore raw

In reply to Daniel Mack

Quoted 6 lines · 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 <[email protected]>
Acked-by Ramon Fried <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

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

Signature · 3 lines

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

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