Skip to contentSkip to conversation

wcn36xx: dequeue all pending indicator messages

PATCH In review Daniel Mack 2 participants 2 messages Mar 19, 2018 – Mar 29, 2018 Qualcomm WCN36XX wireless driver

lore
Share & tools
lore

Daniel Mack

author
#0 lore raw

PATCH v2 1/1 wcn36xx: dequeue all pending indicator messages

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(-)
drivers/net/wireless/ath/wcn36xx/smd.c +52 -43
index 7cc29285e052..a6b5352f59e9 100644
@@ -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)
 {
Signature · 1 line
2.14.3
#1 lore raw
Quoted 8 lines · Daniel Mack

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.

Quoted 6 lines

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

Signature · 3 lines
https://patchwork.kernel.org/patch/10291683/

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