In case wcn36xx_smd_rsp_process() is called more than once before
hal_ind_work was dispatched, the messages will end up in hal_ind_queue,
but wcn36xx_ind_smd_work() will only look at the first message in that
list.
Fix this by dequeing the messages from the list in a loop, and only stop
when it's empty.
This issue was found during a review of the driver. In my tests, that
race never actually occured.
Signed-off-by: Daniel Mack <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
v2: amended the commit log slightly to state that this issue hasn't
occured in the wild.
drivers/net/wireless/ath/wcn36xx/smd.c | 95 +++++++++++++++++++---------------
1 file changed, 52 insertions(+), 43 deletions(-)
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 7cc29285e052..a6b5352f59e9 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2409,54 +2409,63 @@ static void wcn36xx_ind_smd_work(struct work_struct *work)
{
struct wcn36xx *wcn =
container_of(work, struct wcn36xx, hal_ind_work);
- struct wcn36xx_hal_msg_header *msg_header;
- struct wcn36xx_hal_ind_msg *hal_ind_msg;
- unsigned long flags;
- spin_lock_irqsave(&wcn->hal_ind_lock, flags);
+ for (;;) {
+ struct wcn36xx_hal_msg_header *msg_header;
+ struct wcn36xx_hal_ind_msg *hal_ind_msg;
+ unsigned long flags;
- hal_ind_msg = list_first_entry(&wcn->hal_ind_queue,
- struct wcn36xx_hal_ind_msg,
- list);
- list_del(wcn->hal_ind_queue.next);
- spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
+ spin_lock_irqsave(&wcn->hal_ind_lock, flags);
- msg_header = (struct wcn36xx_hal_msg_header *)hal_ind_msg->msg;
+ if (list_empty(&wcn->hal_ind_queue)) {
+ spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
+ return;
+ }
- switch (msg_header->msg_type) {
- case WCN36XX_HAL_COEX_IND:
- case WCN36XX_HAL_DEL_BA_IND:
- case WCN36XX_HAL_AVOID_FREQ_RANGE_IND:
- break;
- case WCN36XX_HAL_OTA_TX_COMPL_IND:
- wcn36xx_smd_tx_compl_ind(wcn,
- hal_ind_msg->msg,
- hal_ind_msg->msg_len);
- break;
- case WCN36XX_HAL_MISSED_BEACON_IND:
- wcn36xx_smd_missed_beacon_ind(wcn,
- hal_ind_msg->msg,
- hal_ind_msg->msg_len);
- break;
- case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- wcn36xx_smd_delete_sta_context_ind(wcn,
- hal_ind_msg->msg,
- hal_ind_msg->msg_len);
- break;
- case WCN36XX_HAL_PRINT_REG_INFO_IND:
- wcn36xx_smd_print_reg_info_ind(wcn,
- hal_ind_msg->msg,
- hal_ind_msg->msg_len);
- break;
- case WCN36XX_HAL_SCAN_OFFLOAD_IND:
- wcn36xx_smd_hw_scan_ind(wcn, hal_ind_msg->msg,
- hal_ind_msg->msg_len);
- break;
- default:
- wcn36xx_err("SMD_EVENT (%d) not supported\n",
- msg_header->msg_type);
+ hal_ind_msg = list_first_entry(&wcn->hal_ind_queue,
+ struct wcn36xx_hal_ind_msg,
+ list);
+ list_del(&hal_ind_msg->list);
+ spin_unlock_irqrestore(&wcn->hal_ind_lock, flags);
+
+ msg_header = (struct wcn36xx_hal_msg_header *)hal_ind_msg->msg;
+
+ switch (msg_header->msg_type) {
+ case WCN36XX_HAL_COEX_IND:
+ case WCN36XX_HAL_DEL_BA_IND:
+ case WCN36XX_HAL_AVOID_FREQ_RANGE_IND:
+ break;
+ case WCN36XX_HAL_OTA_TX_COMPL_IND:
+ wcn36xx_smd_tx_compl_ind(wcn,
+ hal_ind_msg->msg,
+ hal_ind_msg->msg_len);
+ break;
+ case WCN36XX_HAL_MISSED_BEACON_IND:
+ wcn36xx_smd_missed_beacon_ind(wcn,
+ hal_ind_msg->msg,
+ hal_ind_msg->msg_len);
+ break;
+ case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
+ wcn36xx_smd_delete_sta_context_ind(wcn,
+ hal_ind_msg->msg,
+ hal_ind_msg->msg_len);
+ break;
+ case WCN36XX_HAL_PRINT_REG_INFO_IND:
+ wcn36xx_smd_print_reg_info_ind(wcn,
+ hal_ind_msg->msg,
+ hal_ind_msg->msg_len);
+ break;
+ case WCN36XX_HAL_SCAN_OFFLOAD_IND:
+ wcn36xx_smd_hw_scan_ind(wcn, hal_ind_msg->msg,
+ hal_ind_msg->msg_len);
+ break;
+ default:
+ wcn36xx_err("SMD_EVENT (%d) not supported\n",
+ msg_header->msg_type);
+ }
+
+ kfree(hal_ind_msg);
}
- kfree(hal_ind_msg);
}
int wcn36xx_smd_open(struct wcn36xx *wcn)
{
--
2.14.3
Daniel Mack <[email protected]> wrote:
> In case wcn36xx_smd_rsp_process() is called more than once before
> hal_ind_work was dispatched, the messages will end up in hal_ind_queue,
> but wcn36xx_ind_smd_work() will only look at the first message in that
> list.
>
> Fix this by dequeing the messages from the list in a loop, and only stop
> when it's empty.
>
> This issue was found during a review of the driver. In my tests, that
> race never actually occured.
>
> Signed-off-by: Daniel Mack <[email protected]>
> Reviewed-by: Bjorn Andersson <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
Patch applied to ath-next branch of ath.git, thanks.
f276ba06e8b2 wcn36xx: dequeue all pending indicator messages
--
https://patchwork.kernel.org/patch/10291683/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches