ath10k: add support to start and stop qmi service

PATCH In review Govind Singh 2 participants 3 messages Mar 26, 2018 – May 14, 2018 Qualcomm Atheros ATH10K wireless driver

lore

Share & tools

lore

Govind Singh

author

2018-03-26 05:39 UTC #0 lore raw

PATCH 4/12 ath10k: add support to start and stop qmi service

Add support to start qmi service to configure the wlan firmware component and register event notifier to communicate with the WLAN firmware over qmi communication interface.

Signed-off-by Govind Singh <[email protected]>

---
 drivers/net/wireless/ath/ath10k/qmi.c | 155 ++++++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/qmi.h |  13 +++
 2 files changed, 160 insertions(+), 8 deletions(-)

drivers/net/wireless/ath/ath10k/qmi.c +147 -8

index 2235182..3a7fcc6 100644
@@ -31,15 +31,115 @@
 
 static struct ath10k_qmi *qmi;
 
+static int ath10k_qmi_event_fw_ready_ind(struct ath10k_qmi *qmi)
+{
+	pr_debug("fw ready event received\n");
+	spin_lock(&qmi->event_lock);
+	qmi->fw_ready = true;
+	spin_unlock(&qmi->event_lock);
+
+	return 0;
+}
+
+static void ath10k_qmi_fw_ready_ind(struct qmi_handle *qmi_hdl,
+				    struct sockaddr_qrtr *sq,
+				    struct qmi_txn *txn, const void *data)
+{
+	struct ath10k_qmi *qmi = container_of(qmi_hdl, struct ath10k_qmi, qmi_hdl);
+
+	ath10k_qmi_event_fw_ready_ind(qmi);
+}
+
+static void ath10k_qmi_msa_ready_ind(struct qmi_handle *qmi_hdl,
+				     struct sockaddr_qrtr *sq,
+				     struct qmi_txn *txn, const void *data)
+{
+	struct ath10k_qmi *qmi = container_of(qmi_hdl, struct ath10k_qmi, qmi_hdl);
+
+	qmi->msa_ready = true;
+}
+
+static struct qmi_msg_handler qmi_msg_handler[] = {
+	{
+		.type = QMI_INDICATION,
+		.msg_id = QMI_WLFW_FW_READY_IND_V01,
+		.ei = wlfw_fw_ready_ind_msg_v01_ei,
+		.decoded_size = sizeof(struct wlfw_fw_ready_ind_msg_v01),
+		.fn = ath10k_qmi_fw_ready_ind,
+	},
+	{
+		.type = QMI_INDICATION,
+		.msg_id = QMI_WLFW_MSA_READY_IND_V01,
+		.ei = wlfw_msa_ready_ind_msg_v01_ei,
+		.decoded_size = sizeof(struct wlfw_msa_ready_ind_msg_v01),
+		.fn = ath10k_qmi_msa_ready_ind,
+	},
+	{}
+};
+
+static int ath10k_qmi_connect_to_fw_server(struct ath10k_qmi *qmi)
+{
+	struct qmi_handle *qmi_hdl = &qmi->qmi_hdl;
+	int ret;
+
+	ret = kernel_connect(qmi_hdl->sock, (struct sockaddr *)&qmi->sq,
+			     sizeof(qmi->sq), 0);
+	if (ret) {
+		pr_err("fail to connect to remote service port\n");
+		return ret;
+	}
+
+	pr_info("wlan qmi service connected\n");
+
+	return 0;
+}
+
+static void ath10k_qmi_event_server_arrive(struct work_struct *work)
+{
+	struct ath10k_qmi *qmi = container_of(work, struct ath10k_qmi,
+					      work_svc_arrive);
+	int ret;
+
+	ret = ath10k_qmi_connect_to_fw_server(qmi);
+	if (ret)
+		return;
+
+	pr_debug("qmi server arrive\n");
+}
+
+static void ath10k_qmi_event_server_exit(struct work_struct *work)
+{
+	struct ath10k_qmi *qmi = container_of(work, struct ath10k_qmi,
+					      work_svc_exit);
+
+	spin_lock(&qmi->event_lock);
+	qmi->fw_ready = false;
+	spin_unlock(&qmi->event_lock);
+	pr_info("wlan fw service disconnected\n");
+}
+
 static int ath10k_qmi_new_server(struct qmi_handle *qmi_hdl,
 				 struct qmi_service *service)
 {
+	struct ath10k_qmi *qmi = container_of(qmi_hdl, struct ath10k_qmi, qmi_hdl);
+	struct sockaddr_qrtr *sq = &qmi->sq;
+
+	sq->sq_family = AF_QIPCRTR;
+	sq->sq_node = service->node;
+	sq->sq_port = service->port;
+
+	queue_work(qmi->event_wq, &qmi->work_svc_arrive);
+
 	return 0;
 }
 
 static void ath10k_qmi_del_server(struct qmi_handle *qmi_hdl,
 				  struct qmi_service *service)
 {
+	struct ath10k_qmi *qmi =
+		container_of(qmi_hdl, struct ath10k_qmi, qmi_hdl);
+
+	queue_work(qmi->event_wq, &qmi->work_svc_exit);
 }
 
 static struct qmi_ops ath10k_qmi_ops = {

@@ -47,6 +147,51 @@ static void ath10k_qmi_del_server(struct qmi_handle *qmi_hdl,
 	.del_server = ath10k_qmi_del_server,
 };
 
+static int ath10k_alloc_qmi_resources(struct ath10k_qmi *qmi)
+{
+	int ret;
+
+	ret = qmi_handle_init(&qmi->qmi_hdl,
+			      WLFW_BDF_DOWNLOAD_REQ_MSG_V01_MAX_MSG_LEN,
+			      &ath10k_qmi_ops, qmi_msg_handler);
+	if (ret)
+		goto err;
+
+	qmi->event_wq = alloc_workqueue("qmi_driver_event",
+					WQ_UNBOUND, 1);
+	if (!qmi->event_wq) {
+		pr_err("workqueue alloc failed\n");
+		ret = -EFAULT;
+		goto err_qmi_service;
+	}
+
+	spin_lock_init(&qmi->event_lock);
+	INIT_WORK(&qmi->work_svc_arrive, ath10k_qmi_event_server_arrive);
+	INIT_WORK(&qmi->work_svc_exit, ath10k_qmi_event_server_exit);
+
+	ret = qmi_add_lookup(&qmi->qmi_hdl, WLFW_SERVICE_ID_V01,
+			     WLFW_SERVICE_VERS_V01, 0);
+	if (ret)
+		goto err_qmi_service;
+
+	return 0;
+
+err_qmi_service:
+	qmi_handle_release(&qmi->qmi_hdl);
+
+err:
+	return ret;
+}
+
+static void ath10k_remove_qmi_resources(struct ath10k_qmi *qmi)
+{
+	cancel_work_sync(&qmi->work_svc_arrive);
+	cancel_work_sync(&qmi->work_svc_exit);
+	destroy_workqueue(qmi->event_wq);
+	qmi_handle_release(&qmi->qmi_hdl);
+	qmi = NULL;
+}
+
 static int ath10k_qmi_probe(struct platform_device *pdev)
 {
 	int ret;

@@ -58,14 +203,8 @@ static int ath10k_qmi_probe(struct platform_device *pdev)
 
 	qmi->pdev = pdev;
 	platform_set_drvdata(pdev, qmi);
-	ret = qmi_handle_init(&qmi->qmi_hdl,
-			      WLFW_BDF_DOWNLOAD_REQ_MSG_V01_MAX_MSG_LEN,
-			      &ath10k_qmi_ops, NULL);
-	if (ret < 0)
-		goto err;
 
-	ret = qmi_add_lookup(&qmi->qmi_hdl, WLFW_SERVICE_ID_V01,
-			     WLFW_SERVICE_VERS_V01, 0);
+	ret = ath10k_alloc_qmi_resources(qmi);
 	if (ret < 0)
 		goto err;

@@ -81,7 +220,7 @@ static int ath10k_qmi_remove(struct platform_device *pdev)
 {
 	struct ath10k_qmi *qmi = platform_get_drvdata(pdev);
 
-	qmi_handle_release(&qmi->qmi_hdl);
+	ath10k_remove_qmi_resources(qmi);
 
 	return 0;
 }

drivers/net/wireless/ath/ath10k/qmi.h +13 -0

index ad256ef..7697d24 100644
@@ -16,9 +16,22 @@
 #ifndef _QMI_H_
 #define _QMI_H_
 
+enum ath10k_qmi_driver_event_type {
+	ATH10K_QMI_EVENT_SERVER_ARRIVE,
+	ATH10K_QMI_EVENT_SERVER_EXIT,
+	ATH10K_QMI_EVENT_FW_READY_IND,
+	ATH10K_QMI_EVENT_MAX,
+};
+
 struct ath10k_qmi {
 	struct platform_device *pdev;
 	struct qmi_handle qmi_hdl;
 	struct sockaddr_qrtr sq;
+	bool fw_ready;
+	bool msa_ready;
+	struct work_struct work_svc_arrive;
+	struct work_struct work_svc_exit;
+	struct workqueue_struct *event_wq;
+	spinlock_t event_lock; /* spinlock for fw ready status*/
 };
 #endif /* _QMI_H_ */

Signature · 1 line

1.9.1

Bjorn Andersson

2018-05-11 17:42 UTC #1 lore raw

Quoted 15 lines · Govind Singh

Add support to start qmi service to configure the wlan firmware component and register event notifier to communicate with the WLAN firmware over qmi communication interface.
Signed-off-by Govind Singh <[email protected]>
---
 drivers/net/wireless/ath/ath10k/qmi.c | 155 ++++++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/qmi.h |  13 +++
 2 files changed, 160 insertions(+), 8 deletions(-)
drivers/net/wireless/ath/ath10k/qmi.c +0 -0
index 2235182..3a7fcc6 100644

drivers/net/wireless/ath/ath10k/qmi.c line 36

Quoted 6 lines

drivers/net/wireless/ath/ath10k/qmi.c +3 -0

@@ -31,15 +31,115 @@
 
 static struct ath10k_qmi *qmi;
 
+static int ath10k_qmi_event_fw_ready_ind(struct ath10k_qmi *qmi)
+{
+	pr_debug("fw ready event received\n");

Please use dev_dbg.

drivers/net/wireless/ath/ath10k/qmi.c line 39

Quoted 3 lines

drivers/net/wireless/ath/ath10k/qmi.c +3 -0

+	spin_lock(&qmi->event_lock);
+	qmi->fw_ready = true;
+	spin_unlock(&qmi->event_lock);

I see no reason for not just putting this code in
ath10k_qmi_fw_ready_ind().

drivers/net/wireless/ath/ath10k/qmi.c line 40

Quoted 1 line

drivers/net/wireless/ath/ath10k/qmi.c +1 -0
+

fw_ready isn't used for anything, what purpose does this code have?

Quoted 9 lines

drivers/net/wireless/ath/ath10k/qmi.c +9 -0

+	return 0;
+}
+
+static void ath10k_qmi_fw_ready_ind(struct qmi_handle *qmi_hdl,
+				    struct sockaddr_qrtr *sq,
+				    struct qmi_txn *txn, const void *data)
+{
+	struct ath10k_qmi *qmi = container_of(qmi_hdl, struct ath10k_qmi, qmi_hdl);
+

drivers/net/wireless/ath/ath10k/qmi.c line 59

Quoted 10 lines

drivers/net/wireless/ath/ath10k/qmi.c +10 -0

+	ath10k_qmi_event_fw_ready_ind(qmi);
+}
+
+static void ath10k_qmi_msa_ready_ind(struct qmi_handle *qmi_hdl,
+				     struct sockaddr_qrtr *sq,
+				     struct qmi_txn *txn, const void *data)
+{
+	struct ath10k_qmi *qmi = container_of(qmi_hdl, struct ath10k_qmi, qmi_hdl);
+
+	qmi->msa_ready = true;

msa_ready is not only unused in this patch, it's unused after all 11 patches. Can you drop it?

Quoted 62 lines

drivers/net/wireless/ath/ath10k/qmi.c +61 -0

+}
+
+static struct qmi_msg_handler qmi_msg_handler[] = {
+	{
+		.type = QMI_INDICATION,
+		.msg_id = QMI_WLFW_FW_READY_IND_V01,
+		.ei = wlfw_fw_ready_ind_msg_v01_ei,
+		.decoded_size = sizeof(struct wlfw_fw_ready_ind_msg_v01),
+		.fn = ath10k_qmi_fw_ready_ind,
+	},
+	{
+		.type = QMI_INDICATION,
+		.msg_id = QMI_WLFW_MSA_READY_IND_V01,
+		.ei = wlfw_msa_ready_ind_msg_v01_ei,
+		.decoded_size = sizeof(struct wlfw_msa_ready_ind_msg_v01),
+		.fn = ath10k_qmi_msa_ready_ind,
+	},
+	{}
+};
+
+static int ath10k_qmi_connect_to_fw_server(struct ath10k_qmi *qmi)
+{
+	struct qmi_handle *qmi_hdl = &qmi->qmi_hdl;
+	int ret;
+
+	ret = kernel_connect(qmi_hdl->sock, (struct sockaddr *)&qmi->sq,
+			     sizeof(qmi->sq), 0);
+	if (ret) {
+		pr_err("fail to connect to remote service port\n");
+		return ret;
+	}
+
+	pr_info("wlan qmi service connected\n");
+
+	return 0;
+}
+
+static void ath10k_qmi_event_server_arrive(struct work_struct *work)
+{
+	struct ath10k_qmi *qmi = container_of(work, struct ath10k_qmi,
+					      work_svc_arrive);
+	int ret;
+
+	ret = ath10k_qmi_connect_to_fw_server(qmi);
+	if (ret)
+		return;
+
+	pr_debug("qmi server arrive\n");
+}
+
+static void ath10k_qmi_event_server_exit(struct work_struct *work)
+{
+	struct ath10k_qmi *qmi = container_of(work, struct ath10k_qmi,
+					      work_svc_exit);
+
+	spin_lock(&qmi->event_lock);
+	qmi->fw_ready = false;
+	spin_unlock(&qmi->event_lock);
+	pr_info("wlan fw service disconnected\n");
+}
+
 static int ath10k_qmi_new_server(struct qmi_handle *qmi_hdl,

drivers/net/wireless/ath/ath10k/qmi.c line 131

Quoted 10 lines

drivers/net/wireless/ath/ath10k/qmi.c +8 -0

 				 struct qmi_service *service)
 {
+	struct ath10k_qmi *qmi = container_of(qmi_hdl, struct ath10k_qmi, qmi_hdl);
+	struct sockaddr_qrtr *sq = &qmi->sq;
+
+	sq->sq_family = AF_QIPCRTR;
+	sq->sq_node = service->node;
+	sq->sq_port = service->port;
+
+	queue_work(qmi->event_wq, &qmi->work_svc_arrive);

This is being called in a sleepable context and kernel_connect() will not block, so I see no reason for queue work here to invoke ath10k_qmi_event_server_arrive() just to call ath10k_qmi_connect_to_fw_server().

Just put the kernel_connect() call here.

This gives the added benefit that you don't need to use qmi->sq as a way to pass parameters to ath10k_qmi_connect_to_fw_server().

drivers/net/wireless/ath/ath10k/qmi.c line 142

Quoted 11 lines

drivers/net/wireless/ath/ath10k/qmi.c +5 -0

+
 	return 0;
 }
 
 static void ath10k_qmi_del_server(struct qmi_handle *qmi_hdl,
 				  struct qmi_service *service)
 {
+	struct ath10k_qmi *qmi =
+		container_of(qmi_hdl, struct ath10k_qmi, qmi_hdl);
+
+	queue_work(qmi->event_wq, &qmi->work_svc_exit);

I see no reason to queue work to set a boolean to false, just inline the code here.

Quoted 16 lines

drivers/net/wireless/ath/ath10k/qmi.c +9 -0

 }
 
 static struct qmi_ops ath10k_qmi_ops = {

@@ -47,6 +147,51 @@ static void ath10k_qmi_del_server(struct qmi_handle *qmi_hdl,
 	.del_server = ath10k_qmi_del_server,
 };
 
+static int ath10k_alloc_qmi_resources(struct ath10k_qmi *qmi)
+{
+	int ret;
+
+	ret = qmi_handle_init(&qmi->qmi_hdl,
+			      WLFW_BDF_DOWNLOAD_REQ_MSG_V01_MAX_MSG_LEN,
+			      &ath10k_qmi_ops, qmi_msg_handler);
+	if (ret)
+		goto err;

drivers/net/wireless/ath/ath10k/qmi.c static void ath10k_qmi_del_server(struct qmi_handle *qmi_hdl, · line 168

Quoted 10 lines

drivers/net/wireless/ath/ath10k/qmi.c +10 -0

+
+	qmi->event_wq = alloc_workqueue("qmi_driver_event",
+					WQ_UNBOUND, 1);
+	if (!qmi->event_wq) {
+		pr_err("workqueue alloc failed\n");
+		ret = -EFAULT;
+		goto err_qmi_service;
+	}
+
+	spin_lock_init(&qmi->event_lock);

All calls from the qmi helpers are done from a single context, so there's no reason to use this lock.

Quoted 31 lines

drivers/net/wireless/ath/ath10k/qmi.c +26 -0

+	INIT_WORK(&qmi->work_svc_arrive, ath10k_qmi_event_server_arrive);
+	INIT_WORK(&qmi->work_svc_exit, ath10k_qmi_event_server_exit);
+
+	ret = qmi_add_lookup(&qmi->qmi_hdl, WLFW_SERVICE_ID_V01,
+			     WLFW_SERVICE_VERS_V01, 0);
+	if (ret)
+		goto err_qmi_service;
+
+	return 0;
+
+err_qmi_service:
+	qmi_handle_release(&qmi->qmi_hdl);
+
+err:
+	return ret;
+}
+
+static void ath10k_remove_qmi_resources(struct ath10k_qmi *qmi)
+{
+	cancel_work_sync(&qmi->work_svc_arrive);
+	cancel_work_sync(&qmi->work_svc_exit);
+	destroy_workqueue(qmi->event_wq);
+	qmi_handle_release(&qmi->qmi_hdl);
+	qmi = NULL;
+}
+
 static int ath10k_qmi_probe(struct platform_device *pdev)
 {
 	int ret;

@@ -58,14 +203,8 @@ static int ath10k_qmi_probe(struct platform_device *pdev)

drivers/net/wireless/ath/ath10k/qmi.c static int ath10k_qmi_probe(struct platform_device *pdev) · line 68

Quoted 10 lines

drivers/net/wireless/ath/ath10k/qmi.c +0 -7

 	qmi->pdev = pdev;
 	platform_set_drvdata(pdev, qmi);
-	ret = qmi_handle_init(&qmi->qmi_hdl,
-			      WLFW_BDF_DOWNLOAD_REQ_MSG_V01_MAX_MSG_LEN,
-			      &ath10k_qmi_ops, NULL);
-	if (ret < 0)
-		goto err;
 
-	ret = qmi_add_lookup(&qmi->qmi_hdl, WLFW_SERVICE_ID_V01,
-			     WLFW_SERVICE_VERS_V01, 0);

Please plan ahead, there's no reason for adding this here in patch 3 and then just move it in patch 4. That said, with the removal of the queues I don't see a good reason to create a helper for this.

drivers/net/wireless/ath/ath10k/qmi.c static int ath10k_qmi_remove(struct platform_device *pdev) · line 223

Quoted 10 lines

drivers/net/wireless/ath/ath10k/qmi.c +2 -1

+	ret = ath10k_alloc_qmi_resources(qmi);
 	if (ret < 0)
 		goto err;

@@ -81,7 +220,7 @@ static int ath10k_qmi_remove(struct platform_device *pdev)
 {
 	struct ath10k_qmi *qmi = platform_get_drvdata(pdev);
 
-	qmi_handle_release(&qmi->qmi_hdl);
+	ath10k_remove_qmi_resources(qmi);

Just inline ath10k_remove_qmi_resources() here, so one doesn't have to jump around to figure out what's actually going on.

Quoted 19 lines

drivers/net/wireless/ath/ath10k/qmi.c +0 -0

 	return 0;
 }

drivers/net/wireless/ath/ath10k/qmi.h +7 -0

index ad256ef..7697d24 100644
@@ -16,9 +16,22 @@
 #ifndef _QMI_H_
 #define _QMI_H_
 
+enum ath10k_qmi_driver_event_type {
+	ATH10K_QMI_EVENT_SERVER_ARRIVE,
+	ATH10K_QMI_EVENT_SERVER_EXIT,
+	ATH10K_QMI_EVENT_FW_READY_IND,
+	ATH10K_QMI_EVENT_MAX,
+};
+
 struct ath10k_qmi {

drivers/net/wireless/ath/ath10k/qmi.h line 36

Quoted 10 lines

drivers/net/wireless/ath/ath10k/qmi.h +6 -0

 	struct platform_device *pdev;
 	struct qmi_handle qmi_hdl;
 	struct sockaddr_qrtr sq;
+	bool fw_ready;
+	bool msa_ready;
+	struct work_struct work_svc_arrive;
+	struct work_struct work_svc_exit;
+	struct workqueue_struct *event_wq;
+	spinlock_t event_lock; /* spinlock for fw ready status*/
 };

Regards,
Bjorn

Quoted 1 line

Govind Singh

author

2018-05-14 13:29 UTC #2 lore raw

Quoted 19 lines · Bjorn Andersson

Quoted 17 lines · Govind Singh
Add support to start qmi service to configure the wlan firmware component and register event notifier to communicate with the WLAN firmware over qmi communication interface.
Signed-off-by Govind Singh <[email protected]>
---
 drivers/net/wireless/ath/ath10k/qmi.c | 155 
++++++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/qmi.h |  13 +++
 2 files changed, 160 insertions(+), 8 deletions(-)
drivers/net/wireless/ath/ath10k/qmi.c +0 -0
index 2235182..3a7fcc6 100644
@@ -31,15 +31,115 @@
 

Quoted 8 lines

drivers/net/wireless/ath/ath10k/qmi.c line 36
Quoted 5 lines
drivers/net/wireless/ath/ath10k/qmi.c +3 -0
 static struct ath10k_qmi *qmi;
 
+static int ath10k_qmi_event_fw_ready_ind(struct ath10k_qmi *qmi)
+{
+	pr_debug("fw ready event received\n");
Please use dev_dbg.

I have migrated from pr_ to dev_ logging for the whole set, will share in next version.

Quoted 11 lines

drivers/net/wireless/ath/ath10k/qmi.c line 39
Quoted 3 lines
drivers/net/wireless/ath/ath10k/qmi.c +3 -0
+	spin_lock(&qmi->event_lock);
+	qmi->fw_ready = true;
+	spin_unlock(&qmi->event_lock);
I see no reason for not just putting this code in
ath10k_qmi_fw_ready_ind().
drivers/net/wireless/ath/ath10k/qmi.c line 40
Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.c +1 -0
+
fw_ready isn't used for anything, what purpose does this code have?

fw_ready will be used in later set of changes for qmi client debugfs and for synchronization.I will decouple this and add as bit mask in appropriate change.

Quoted 16 lines

drivers/net/wireless/ath/ath10k/qmi.c +15 -0

+	return 0;
+}
+
+static void ath10k_qmi_fw_ready_ind(struct qmi_handle *qmi_hdl,
+				    struct sockaddr_qrtr *sq,
+				    struct qmi_txn *txn, const void *data)
+{
+	struct ath10k_qmi *qmi = container_of(qmi_hdl, struct ath10k_qmi, 
 qmi_hdl);
+
+	ath10k_qmi_event_fw_ready_ind(qmi);
+}
+
+static void ath10k_qmi_msa_ready_ind(struct qmi_handle *qmi_hdl,
+				     struct sockaddr_qrtr *sq,
+				     struct qmi_txn *txn, const void *data)

Quoted 9 lines

drivers/net/wireless/ath/ath10k/qmi.c line 59
Quoted 5 lines
drivers/net/wireless/ath/ath10k/qmi.c +4 -0
+{
+	struct ath10k_qmi *qmi = container_of(qmi_hdl, struct ath10k_qmi, 
 qmi_hdl);
+
+	qmi->msa_ready = true;
msa_ready is not only unused in this patch, it's unused after all 11 patches. Can you drop it?

Sure, will remove in next version.

Quoted 32 lines

Quoted 16 lines
drivers/net/wireless/ath/ath10k/qmi.c +13 -0
+static void ath10k_qmi_event_server_arrive(struct work_struct *work)
+{
+	struct ath10k_qmi *qmi = container_of(work, struct ath10k_qmi,
+					      work_svc_arrive);
+	int ret;
+
+	ret = ath10k_qmi_connect_to_fw_server(qmi);
+	if (ret)
+		return;
+
+	pr_debug("qmi server arrive\n");
+}
+
 
 static int ath10k_qmi_new_server(struct qmi_handle *qmi_hdl,
 				 struct qmi_service *service)
drivers/net/wireless/ath/ath10k/qmi.c
Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +8 -0
 {
+	struct ath10k_qmi *qmi = container_of(qmi_hdl, struct ath10k_qmi, 
 qmi_hdl);
+	struct sockaddr_qrtr *sq = &qmi->sq;
+
+	sq->sq_family = AF_QIPCRTR;
+	sq->sq_node = service->node;
+	sq->sq_port = service->port;
+
+	queue_work(qmi->event_wq, &qmi->work_svc_arrive);
This is being called in a sleepable context and kernel_connect() will not block, so I see no reason for queue work here to invoke ath10k_qmi_event_server_arrive() just to call ath10k_qmi_connect_to_fw_server().

Quoted 3 lines

Just put the kernel_connect() call here.

In this change it just calls ath10k_qmi_connect_to_fw_server, but most of the handshaking is done in server arrive callback. Successive changes adds the request/response of each handshake.
Is it ok to block new_server callback till all client handshaking gets completed. I thought it might block other client notification overlapped on the same time slot(ex: IPA). Do you see any concern here or should be ok.
I have not profiled yet, i will share the approx time it can take.

Quoted 7 lines

This gives the added benefit that you don't need to use qmi->sq as a way to pass parameters to ath10k_qmi_connect_to_fw_server().
Quoted 4 lines
drivers/net/wireless/ath/ath10k/qmi.c +1 -0
+
 	return 0;
 }
 

Quoted 11 lines

drivers/net/wireless/ath/ath10k/qmi.c line 142
Quoted 7 lines
drivers/net/wireless/ath/ath10k/qmi.c +4 -0
 static void ath10k_qmi_del_server(struct qmi_handle *qmi_hdl,
 				  struct qmi_service *service)
 {
+	struct ath10k_qmi *qmi =
+		container_of(qmi_hdl, struct ath10k_qmi, qmi_hdl);
+
+	queue_work(qmi->event_wq, &qmi->work_svc_exit);
I see no reason to queue work to set a boolean to false, just inline the code here.

sure, this i can remove.

Quoted 70 lines

Quoted 16 lines

static struct qmi_ops ath10k_qmi_ops = {

drivers/net/wireless/ath/ath10k/qmi.c +9 -0

@@ -47,6 +147,51 @@ static void ath10k_qmi_del_server(struct 
 qmi_handle *qmi_hdl,
 	.del_server = ath10k_qmi_del_server,
 };
 
+static int ath10k_alloc_qmi_resources(struct ath10k_qmi *qmi)
+{
+	int ret;
+
+	ret = qmi_handle_init(&qmi->qmi_hdl,
+			      WLFW_BDF_DOWNLOAD_REQ_MSG_V01_MAX_MSG_LEN,
+			      &ath10k_qmi_ops, qmi_msg_handler);
+	if (ret)
+		goto err;

drivers/net/wireless/ath/ath10k/qmi.c static void ath10k_qmi_del_server(struct · line 169

Quoted 10 lines

drivers/net/wireless/ath/ath10k/qmi.c +10 -0

+
+	qmi->event_wq = alloc_workqueue("qmi_driver_event",
+					WQ_UNBOUND, 1);
+	if (!qmi->event_wq) {
+		pr_err("workqueue alloc failed\n");
+		ret = -EFAULT;
+		goto err_qmi_service;
+	}
+
+	spin_lock_init(&qmi->event_lock);

All calls from the qmi helpers are done from a single context, so there's no reason to use this lock.

Quoted 40 lines

drivers/net/wireless/ath/ath10k/qmi.c +26 -0

+	INIT_WORK(&qmi->work_svc_arrive, ath10k_qmi_event_server_arrive);
+	INIT_WORK(&qmi->work_svc_exit, ath10k_qmi_event_server_exit);
+
+	ret = qmi_add_lookup(&qmi->qmi_hdl, WLFW_SERVICE_ID_V01,
+			     WLFW_SERVICE_VERS_V01, 0);
+	if (ret)
+		goto err_qmi_service;
+
+	return 0;
+
+err_qmi_service:
+	qmi_handle_release(&qmi->qmi_hdl);
+
+err:
+	return ret;
+}
+
+static void ath10k_remove_qmi_resources(struct ath10k_qmi *qmi)
+{
+	cancel_work_sync(&qmi->work_svc_arrive);
+	cancel_work_sync(&qmi->work_svc_exit);
+	destroy_workqueue(qmi->event_wq);
+	qmi_handle_release(&qmi->qmi_hdl);
+	qmi = NULL;
+}
+
 static int ath10k_qmi_probe(struct platform_device *pdev)
 {

drivers/net/wireless/ath/ath10k/qmi.c +0 -5

 	int ret;

@@ -58,14 +203,8 @@ static int ath10k_qmi_probe(struct platform_device 
 *pdev)
 
 	qmi->pdev = pdev;
 	platform_set_drvdata(pdev, qmi);
-	ret = qmi_handle_init(&qmi->qmi_hdl,
-			      WLFW_BDF_DOWNLOAD_REQ_MSG_V01_MAX_MSG_LEN,
-			      &ath10k_qmi_ops, NULL);
-	if (ret < 0)
-		goto err;

Quoted 7 lines

drivers/net/wireless/ath/ath10k/qmi.c static int ath10k_qmi_probe(struct platform_device *pdev) · line 68
Quoted 2 lines
drivers/net/wireless/ath/ath10k/qmi.c +0 -2
-	ret = qmi_add_lookup(&qmi->qmi_hdl, WLFW_SERVICE_ID_V01,
-			     WLFW_SERVICE_VERS_V01, 0);
Please plan ahead, there's no reason for adding this here in patch 3 and then just move it in patch 4. That said, with the removal of the queues I don't see a good reason to create a helper for this.

Sure, will do in next version.

Quoted 9 lines

drivers/net/wireless/ath/ath10k/qmi.c +1 -0

+	ret = ath10k_alloc_qmi_resources(qmi);
 	if (ret < 0)
 		goto err;

@@ -81,7 +220,7 @@ static int ath10k_qmi_remove(struct platform_device 
 *pdev)
 {
 	struct ath10k_qmi *qmi = platform_get_drvdata(pdev);

Quoted 6 lines

drivers/net/wireless/ath/ath10k/qmi.c static int ath10k_qmi_remove(struct platform_device *pdev) · line 223
Quoted 2 lines
drivers/net/wireless/ath/ath10k/qmi.c +1 -1
-	qmi_handle_release(&qmi->qmi_hdl);
+	ath10k_remove_qmi_resources(qmi);
Just inline ath10k_remove_qmi_resources() here, so one doesn't have to jump around to figure out what's actually going on.

Sure, will do in next version.

return 0;

Quoted 4 lines

Regards,
Bjorn
Quoted 1 line

BR,
Govind