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(+)
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
Daniel Mack <[email protected]> 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 <[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
--
https://patchwork.kernel.org/patch/10321545/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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(+)
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
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(-)
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
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 <[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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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 <[email protected]>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project