Skip to contentSkip to conversation

ath10k: Add MSA handshake QMI mgs support

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

lore
Share & tools
lore

Govind Singh

author
#0 lore raw

PATCH 7/12 ath10k: Add MSA handshake QMI mgs support

HOST allocates 2mb of region for modem and WCN3990 secure access and provides the address and access control to target for secure access.
Add MSA handshake request/response messages.

Signed-off-by Govind Singh <[email protected]>
---
 drivers/net/wireless/ath/ath10k/qmi.c | 288 +++++++++++++++++++++++++++++++++-
 drivers/net/wireless/ath/ath10k/qmi.h |  14 ++
 2 files changed, 300 insertions(+), 2 deletions(-)
drivers/net/wireless/ath/ath10k/qmi.c +286 -2
index bc80b8f..763b812 100644
@@ -26,6 +26,8 @@
 #include <linux/string.h>
 #include <net/sock.h>
 #include <linux/soc/qcom/qmi.h>
+#include <linux/qcom_scm.h>
+#include <linux/of.h>
 #include "qmi.h"
 #include "qmi_svc_v01.h"
 

@@ -35,6 +37,240 @@
 static struct ath10k_qmi *qmi;
 
 static int
+ath10k_qmi_map_msa_permissions(struct ath10k_msa_mem_region_info *mem_region)
+{
+	struct qcom_scm_vmperm dst_perms[3];
+	unsigned int src_perms;
+	phys_addr_t addr;
+	u32 perm_count;
+	u32 size;
+	int ret;
+
+	addr = mem_region->reg_addr;
+	size = mem_region->size;
+
+	src_perms = BIT(QCOM_SCM_VMID_HLOS);
+
+	dst_perms[0].vmid = QCOM_SCM_VMID_MSS_MSA;
+	dst_perms[0].perm = QCOM_SCM_PERM_RW;
+	dst_perms[1].vmid = QCOM_SCM_VMID_WLAN;
+	dst_perms[1].perm = QCOM_SCM_PERM_RW;
+
+	if (!mem_region->secure_flag) {
+		dst_perms[2].vmid = QCOM_SCM_VMID_WLAN_CE;
+		dst_perms[2].perm = QCOM_SCM_PERM_RW;
+		perm_count = 3;
+	} else {
+		dst_perms[2].vmid = 0;
+		dst_perms[2].perm = 0;
+		perm_count = 2;
+	}
+
+	ret = qcom_scm_assign_mem(addr, size, &src_perms,
+				  dst_perms, perm_count);
+	if (ret < 0)
+		pr_err("msa map permission failed=%d\n", ret);
+
+	return ret;
+}
+
+static int
+ath10k_qmi_unmap_msa_permissions(struct ath10k_msa_mem_region_info *mem_region)
+{
+	struct qcom_scm_vmperm dst_perms;
+	unsigned int src_perms;
+	phys_addr_t addr;
+	u32 size;
+	int ret;
+
+	addr = mem_region->reg_addr;
+	size = mem_region->size;
+
+	src_perms = BIT(QCOM_SCM_VMID_MSS_MSA) | BIT(QCOM_SCM_VMID_WLAN);
+
+	if (!mem_region->secure_flag)
+		src_perms |= BIT(QCOM_SCM_VMID_WLAN_CE);
+
+	dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+	dst_perms.perm = QCOM_SCM_PERM_RW;
+
+	ret = qcom_scm_assign_mem(addr, size, &src_perms, &dst_perms, 1);
+	if (ret < 0)
+		pr_err("msa unmap permission failed=%d\n", ret);
+
+	return ret;
+}
+
+static int ath10k_qmi_setup_msa_permissions(struct ath10k_qmi *qmi)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < qmi->nr_mem_region; i++) {
+		ret = ath10k_qmi_map_msa_permissions(&qmi->mem_region[i]);
+		if (ret)
+			goto err_unmap;
+	}
+
+	return 0;
+
+err_unmap:
+	for (i--; i >= 0; i--)
+		ath10k_qmi_unmap_msa_permissions(&qmi->mem_region[i]);
+	return ret;
+}
+
+static void ath10k_qmi_remove_msa_permissions(struct ath10k_qmi *qmi)
+{
+	int i;
+
+	for (i = 0; i < qmi->nr_mem_region; i++)
+		ath10k_qmi_unmap_msa_permissions(&qmi->mem_region[i]);
+}
+
+static int
+	ath10k_qmi_msa_mem_info_send_sync_msg(struct ath10k_qmi *qmi)
+{
+	struct wlfw_msa_info_resp_msg_v01 *resp;
+	struct wlfw_msa_info_req_msg_v01 *req;
+	struct qmi_txn txn;
+	int ret;
+	int i;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+
+	resp = kzalloc(sizeof(*resp), GFP_KERNEL);
+	if (!resp) {
+		kfree(req);
+		return -ENOMEM;
+	}
+
+	req->msa_addr = qmi->msa_pa;
+	req->size = qmi->msa_mem_size;
+
+	ret = qmi_txn_init(&qmi->qmi_hdl, &txn,
+			   wlfw_msa_info_resp_msg_v01_ei, resp);
+	if (ret < 0) {
+		pr_err("fail to init txn for MSA mem info resp %d\n",
+		       ret);
+		goto out;
+	}
+
+	ret = qmi_send_request(&qmi->qmi_hdl, NULL, &txn,
+			       QMI_WLFW_MSA_INFO_REQ_V01,
+			       WLFW_MSA_INFO_REQ_MSG_V01_MAX_MSG_LEN,
+			       wlfw_msa_info_req_msg_v01_ei, req);
+	if (ret < 0) {
+		qmi_txn_cancel(&txn);
+		pr_err("fail to send MSA mem info req %d\n", ret);
+		goto out;
+	}
+
+	ret = qmi_txn_wait(&txn, WLFW_TIMEOUT * HZ);
+	if (ret < 0)
+		goto out;
+
+	if (resp->resp.result != QMI_RESULT_SUCCESS_V01) {
+		pr_err("MSA mem info request rejected, result:%d error:%d\n",
+		       resp->resp.result, resp->resp.error);
+		ret = -resp->resp.result;
+		goto out;
+	}
+
+	pr_debug("receive mem_region_info_len: %d\n",
+		 resp->mem_region_info_len);
+
+	if (resp->mem_region_info_len > QMI_WLFW_MAX_NUM_MEMORY_REGIONS_V01) {
+		pr_err("invalid memory region length received: %d\n",
+		       resp->mem_region_info_len);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	qmi->nr_mem_region = resp->mem_region_info_len;
+	for (i = 0; i < resp->mem_region_info_len; i++) {
+		qmi->mem_region[i].reg_addr =
+			resp->mem_region_info[i].region_addr;
+		qmi->mem_region[i].size =
+			resp->mem_region_info[i].size;
+		qmi->mem_region[i].secure_flag =
+			resp->mem_region_info[i].secure_flag;
+		pr_debug("mem region: %d Addr: 0x%llx Size: 0x%x Flag: 0x%08x\n",
+			 i, qmi->mem_region[i].reg_addr,
+			 qmi->mem_region[i].size,
+			 qmi->mem_region[i].secure_flag);
+	}
+
+	pr_debug("MSA mem info request completed\n");
+	kfree(resp);
+	kfree(req);
+	return 0;
+
+out:
+	kfree(resp);
+	kfree(req);
+	return ret;
+}
+
+static int ath10k_qmi_msa_ready_send_sync_msg(struct ath10k_qmi *qmi)
+{
+	struct wlfw_msa_ready_resp_msg_v01 *resp;
+	struct wlfw_msa_ready_req_msg_v01 *req;
+	struct qmi_txn txn;
+	int ret;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+
+	resp = kzalloc(sizeof(*resp), GFP_KERNEL);
+	if (!resp) {
+		kfree(req);
+		return -ENOMEM;
+	}
+
+	ret = qmi_txn_init(&qmi->qmi_hdl, &txn,
+			   wlfw_msa_ready_resp_msg_v01_ei, resp);
+	if (ret < 0) {
+		pr_err("fail to init txn for MSA mem ready resp %d\n",
+		       ret);
+		goto out;
+	}
+
+	ret = qmi_send_request(&qmi->qmi_hdl, NULL, &txn,
+			       QMI_WLFW_MSA_READY_REQ_V01,
+			       WLFW_MSA_READY_REQ_MSG_V01_MAX_MSG_LEN,
+			       wlfw_msa_ready_req_msg_v01_ei, req);
+	if (ret < 0) {
+		qmi_txn_cancel(&txn);
+		pr_err("fail to send MSA mem ready req %d\n", ret);
+		goto out;
+	}
+
+	ret = qmi_txn_wait(&txn, WLFW_TIMEOUT * HZ);
+	if (ret < 0)
+		goto out;
+
+	if (resp->resp.result != QMI_RESULT_SUCCESS_V01) {
+		pr_err("qmi MSA mem ready request rejected, result:%d error:%d\n",
+		       resp->resp.result, resp->resp.error);
+		ret = -resp->resp.result;
+	}
+
+	pr_debug("MSA mem ready request completed\n");
+	kfree(resp);
+	kfree(req);
+	return 0;
+
+out:
+	kfree(resp);
+	kfree(req);
+	return ret;
+}
+
+static int
 ath10k_qmi_ind_register_send_sync_msg(struct ath10k_qmi *qmi)
 {
 	struct wlfw_ind_register_resp_msg_v01 *resp;

@@ -173,7 +409,27 @@ static void ath10k_qmi_event_server_arrive(struct work_struct *work)
 	if (ret)
 		return;
 
-	ath10k_qmi_ind_register_send_sync_msg(qmi);
+	ret = ath10k_qmi_ind_register_send_sync_msg(qmi);
+	if (ret)
+		return;
+
+	ret = ath10k_qmi_msa_mem_info_send_sync_msg(qmi);
+	if (ret)
+		return;
+
+	ret = ath10k_qmi_setup_msa_permissions(qmi);
+	if (ret)
+		return;
+
+	ret = ath10k_qmi_msa_ready_send_sync_msg(qmi);
+	if (ret)
+		goto err_setup_msa;
+
+	return;
+
+err_setup_msa:
+	ath10k_qmi_remove_msa_permissions(qmi);
+	return;
 }
 
 static void ath10k_qmi_event_server_exit(struct work_struct *work)

@@ -252,6 +508,34 @@ static int ath10k_alloc_qmi_resources(struct ath10k_qmi *qmi)
 	return ret;
 }
 
+static int ath10k_qmi_setup_msa_resources(struct device *dev,
+					  struct ath10k_qmi *qmi)
+{
+	int ret;
+
+	ret = of_property_read_u32(dev->of_node, "qcom,wlan-msa-memory",
+				   &qmi->msa_mem_size);
+
+	if (ret || qmi->msa_mem_size == 0) {
+		pr_err("fail to get MSA memory size: %u, ret: %d\n",
+		       qmi->msa_mem_size, ret);
+		return -ENOMEM;
+	}
+
+	qmi->msa_va = dmam_alloc_coherent(dev, qmi->msa_mem_size,
+					  &qmi->msa_pa, GFP_KERNEL);
+	if (!qmi->msa_va) {
+		pr_err("dma alloc failed for MSA\n");
+		return -ENOMEM;
+	}
+
+	pr_debug("MSA pa: %pa, MSA va: 0x%p\n",
+		 &qmi->msa_pa,
+		 qmi->msa_va);
+
+	return 0;
+}
+
 static void ath10k_remove_qmi_resources(struct ath10k_qmi *qmi)
 {
 	cancel_work_sync(&qmi->work_svc_arrive);

@@ -272,7 +556,7 @@ static int ath10k_qmi_probe(struct platform_device *pdev)
 
 	qmi->pdev = pdev;
 	platform_set_drvdata(pdev, qmi);
-
+	ath10k_qmi_setup_msa_resources(&pdev->dev, qmi);
 	ret = ath10k_alloc_qmi_resources(qmi);
 	if (ret < 0)
 		goto err;
drivers/net/wireless/ath/ath10k/qmi.h +14 -0
index 7697d24..47af020 100644
@@ -16,6 +16,8 @@
 #ifndef _QMI_H_
 #define _QMI_H_
 
+#define MAX_NUM_MEMORY_REGIONS			2
+
 enum ath10k_qmi_driver_event_type {
 	ATH10K_QMI_EVENT_SERVER_ARRIVE,
 	ATH10K_QMI_EVENT_SERVER_EXIT,

@@ -23,6 +25,12 @@ enum ath10k_qmi_driver_event_type {
 	ATH10K_QMI_EVENT_MAX,
 };
 
+struct ath10k_msa_mem_region_info {
+	u64 reg_addr;
+	u32 size;
+	u8 secure_flag;
+};
+
 struct ath10k_qmi {
 	struct platform_device *pdev;
 	struct qmi_handle qmi_hdl;

@@ -33,5 +41,11 @@ struct ath10k_qmi {
 	struct work_struct work_svc_exit;
 	struct workqueue_struct *event_wq;
 	spinlock_t event_lock; /* spinlock for fw ready status*/
+	u32 nr_mem_region;
+	struct ath10k_msa_mem_region_info
+		mem_region[MAX_NUM_MEMORY_REGIONS];
+	phys_addr_t msa_pa;
+	u32 msa_mem_size;
+	void *msa_va;
 };
 #endif /* _QMI_H_ */
Signature · 1 line
1.9.1
#1 lore raw
Quoted 7 lines · Govind Singh

HOST allocates 2mb of region for modem and WCN3990 secure access and provides the address and access control to target for secure access.
Add MSA handshake request/response messages.

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

This added a new ath10k-check warning:

drivers/net/wireless/ath/ath10k/qmi.c:433: void function return statements are not generally useful

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

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#2 lore raw
In reply to Govind Singh
Quoted 7 lines · Govind Singh

HOST allocates 2mb of region for modem and WCN3990 secure access and provides the address and access control to target for secure access.
Add MSA handshake request/response messages.

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

This added a new ath10k-check warning:

drivers/net/wireless/ath/ath10k/qmi.c:433: void function return statements are not generally useful

Signature · 9 lines
https://patchwork.kernel.org/patch/10307183/

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


_______________________________________________
ath10k mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/ath10k
#3 lore raw
In reply to Govind Singh
Quoted 29 lines · Govind Singh

HOST allocates 2mb of region for modem and WCN3990 secure access and provides the address and access control to target for secure access.
Add MSA handshake request/response messages.

Signed-off-by Govind Singh <[email protected]>
---
 drivers/net/wireless/ath/ath10k/qmi.c | 288 +++++++++++++++++++++++++++++++++-
 drivers/net/wireless/ath/ath10k/qmi.h |  14 ++
 2 files changed, 300 insertions(+), 2 deletions(-)
drivers/net/wireless/ath/ath10k/qmi.c +3 -0
index bc80b8f..763b812 100644
@@ -26,6 +26,8 @@
 #include <linux/string.h>
 #include <net/sock.h>
 #include <linux/soc/qcom/qmi.h>
+#include <linux/qcom_scm.h>
+#include <linux/of.h>
 #include "qmi.h"
 #include "qmi_svc_v01.h"
 

@@ -35,6 +37,240 @@
 static struct ath10k_qmi *qmi;
 
 static int
+ath10k_qmi_map_msa_permissions(struct ath10k_msa_mem_region_info *mem_region)

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

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +10 -0
+{
+	struct qcom_scm_vmperm dst_perms[3];
+	unsigned int src_perms;
+	phys_addr_t addr;
+	u32 perm_count;
+	u32 size;
+	int ret;
+
+	addr = mem_region->reg_addr;
+	size = mem_region->size;

Skip the local variables.

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

Quoted 9 lines
drivers/net/wireless/ath/ath10k/qmi.c +9 -0
+
+	src_perms = BIT(QCOM_SCM_VMID_HLOS);
+
+	dst_perms[0].vmid = QCOM_SCM_VMID_MSS_MSA;
+	dst_perms[0].perm = QCOM_SCM_PERM_RW;
+	dst_perms[1].vmid = QCOM_SCM_VMID_WLAN;
+	dst_perms[1].perm = QCOM_SCM_PERM_RW;
+
+	if (!mem_region->secure_flag) {

So with secure_flag equal to 0 we give less subsystems access to the data? Is this logic inverted?

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

Quoted 7 lines
drivers/net/wireless/ath/ath10k/qmi.c +7 -0
+		dst_perms[2].vmid = QCOM_SCM_VMID_WLAN_CE;
+		dst_perms[2].perm = QCOM_SCM_PERM_RW;
+		perm_count = 3;
+	} else {
+		dst_perms[2].vmid = 0;
+		dst_perms[2].perm = 0;
+		perm_count = 2;

If you set perm_count to 2 you don't need to clear vmid and perm of dst_perms[2].

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

Quoted 6 lines
drivers/net/wireless/ath/ath10k/qmi.c +6 -0
+	}
+
+	ret = qcom_scm_assign_mem(addr, size, &src_perms,
+				  dst_perms, perm_count);
+	if (ret < 0)
+		pr_err("msa map permission failed=%d\n", ret);

Use dev_err()

Quoted 5 lines
drivers/net/wireless/ath/ath10k/qmi.c +5 -0
+
+	return ret;
+}
+
+static int

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

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +10 -0
+ath10k_qmi_unmap_msa_permissions(struct ath10k_msa_mem_region_info *mem_region)
+{
+	struct qcom_scm_vmperm dst_perms;
+	unsigned int src_perms;
+	phys_addr_t addr;
+	u32 size;
+	int ret;
+
+	addr = mem_region->reg_addr;
+	size = mem_region->size;

Skip the local variables.

Quoted 37 lines
drivers/net/wireless/ath/ath10k/qmi.c +37 -0
+
+	src_perms = BIT(QCOM_SCM_VMID_MSS_MSA) | BIT(QCOM_SCM_VMID_WLAN);
+
+	if (!mem_region->secure_flag)
+		src_perms |= BIT(QCOM_SCM_VMID_WLAN_CE);
+
+	dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+	dst_perms.perm = QCOM_SCM_PERM_RW;
+
+	ret = qcom_scm_assign_mem(addr, size, &src_perms, &dst_perms, 1);
+	if (ret < 0)
+		pr_err("msa unmap permission failed=%d\n", ret);
+
+	return ret;
+}
+
+static int ath10k_qmi_setup_msa_permissions(struct ath10k_qmi *qmi)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < qmi->nr_mem_region; i++) {
+		ret = ath10k_qmi_map_msa_permissions(&qmi->mem_region[i]);
+		if (ret)
+			goto err_unmap;
+	}
+
+	return 0;
+
+err_unmap:
+	for (i--; i >= 0; i--)
+		ath10k_qmi_unmap_msa_permissions(&qmi->mem_region[i]);
+	return ret;
+}
+
+static void ath10k_qmi_remove_msa_permissions(struct ath10k_qmi *qmi)
+{

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

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +10 -0
+	int i;
+
+	for (i = 0; i < qmi->nr_mem_region; i++)
+		ath10k_qmi_unmap_msa_permissions(&qmi->mem_region[i]);
+}
+
+static int
+	ath10k_qmi_msa_mem_info_send_sync_msg(struct ath10k_qmi *qmi)
+{
+	struct wlfw_msa_info_resp_msg_v01 *resp;

This is 40 bytes,

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

Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.c +1 -0
+	struct wlfw_msa_info_req_msg_v01 *req;

This is 6 bytes.

So just put them on the stack and skip the memory management.

Quoted 12 lines
drivers/net/wireless/ath/ath10k/qmi.c +12 -0
+	struct qmi_txn txn;
+	int ret;
+	int i;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+
+	resp = kzalloc(sizeof(*resp), GFP_KERNEL);
+	if (!resp) {
+		kfree(req);
+		return -ENOMEM;

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

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +10 -0
+	}
+
+	req->msa_addr = qmi->msa_pa;
+	req->size = qmi->msa_mem_size;
+
+	ret = qmi_txn_init(&qmi->qmi_hdl, &txn,
+			   wlfw_msa_info_resp_msg_v01_ei, resp);
+	if (ret < 0) {
+		pr_err("fail to init txn for MSA mem info resp %d\n",
+		       ret);

Unless we have 2 billion outstanding transactions "ret" is going to be ENOMEM here, in which case there is already a printout telling you about the problem.

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

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +10 -0
+		goto out;
+	}
+
+	ret = qmi_send_request(&qmi->qmi_hdl, NULL, &txn,
+			       QMI_WLFW_MSA_INFO_REQ_V01,
+			       WLFW_MSA_INFO_REQ_MSG_V01_MAX_MSG_LEN,
+			       wlfw_msa_info_req_msg_v01_ei, req);
+	if (ret < 0) {
+		qmi_txn_cancel(&txn);
+		pr_err("fail to send MSA mem info req %d\n", ret);

Use dev_err().

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

Quoted 4 lines
drivers/net/wireless/ath/ath10k/qmi.c +4 -0
+		goto out;
+	}
+
+	ret = qmi_txn_wait(&txn, WLFW_TIMEOUT * HZ);

The timeout is in jiffies, but I doubt you intended to the timeout to be 500 seconds either.

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

Quoted 7 lines
drivers/net/wireless/ath/ath10k/qmi.c +7 -0
+	if (ret < 0)
+		goto out;
+
+	if (resp->resp.result != QMI_RESULT_SUCCESS_V01) {
+		pr_err("MSA mem info request rejected, result:%d error:%d\n",
+		       resp->resp.result, resp->resp.error);
+		ret = -resp->resp.result;

Future readers will expect this to be a errno, in particular as most other exit paths return errnos.

Quoted 7 lines
drivers/net/wireless/ath/ath10k/qmi.c +7 -0
+		goto out;
+	}
+
+	pr_debug("receive mem_region_info_len: %d\n",
+		 resp->mem_region_info_len);
+
+	if (resp->mem_region_info_len > QMI_WLFW_MAX_NUM_MEMORY_REGIONS_V01) {

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

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +10 -0
+		pr_err("invalid memory region length received: %d\n",
+		       resp->mem_region_info_len);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	qmi->nr_mem_region = resp->mem_region_info_len;
+	for (i = 0; i < resp->mem_region_info_len; i++) {
+		qmi->mem_region[i].reg_addr =
+			resp->mem_region_info[i].region_addr;

With the use of a local variable you should be able to avoid the line breaks here.

Quoted 14 lines
drivers/net/wireless/ath/ath10k/qmi.c +14 -0
+		qmi->mem_region[i].size =
+			resp->mem_region_info[i].size;
+		qmi->mem_region[i].secure_flag =
+			resp->mem_region_info[i].secure_flag;
+		pr_debug("mem region: %d Addr: 0x%llx Size: 0x%x Flag: 0x%08x\n",
+			 i, qmi->mem_region[i].reg_addr,
+			 qmi->mem_region[i].size,
+			 qmi->mem_region[i].secure_flag);
+	}
+
+	pr_debug("MSA mem info request completed\n");
+	kfree(resp);
+	kfree(req);
+	return 0;

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

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +10 -0
+
+out:
+	kfree(resp);
+	kfree(req);
+	return ret;
+}
+
+static int ath10k_qmi_msa_ready_send_sync_msg(struct ath10k_qmi *qmi)
+{
+	struct wlfw_msa_ready_resp_msg_v01 *resp;

This is 4 bytes,

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

Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.c +1 -0
+	struct wlfw_msa_ready_req_msg_v01 *req;

and this is 1 byte, use the stack.

Quoted 22 lines
drivers/net/wireless/ath/ath10k/qmi.c +22 -0
+	struct qmi_txn txn;
+	int ret;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+
+	resp = kzalloc(sizeof(*resp), GFP_KERNEL);
+	if (!resp) {
+		kfree(req);
+		return -ENOMEM;
+	}
+
+	ret = qmi_txn_init(&qmi->qmi_hdl, &txn,
+			   wlfw_msa_ready_resp_msg_v01_ei, resp);
+	if (ret < 0) {
+		pr_err("fail to init txn for MSA mem ready resp %d\n",
+		       ret);
+		goto out;
+	}
+
+	ret = qmi_send_request(&qmi->qmi_hdl, NULL, &txn,

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

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +10 -0
+			       QMI_WLFW_MSA_READY_REQ_V01,
+			       WLFW_MSA_READY_REQ_MSG_V01_MAX_MSG_LEN,
+			       wlfw_msa_ready_req_msg_v01_ei, req);
+	if (ret < 0) {
+		qmi_txn_cancel(&txn);
+		pr_err("fail to send MSA mem ready req %d\n", ret);
+		goto out;
+	}
+
+	ret = qmi_txn_wait(&txn, WLFW_TIMEOUT * HZ);

As above.

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

Quoted 6 lines
drivers/net/wireless/ath/ath10k/qmi.c +6 -0
+	if (ret < 0)
+		goto out;
+
+	if (resp->resp.result != QMI_RESULT_SUCCESS_V01) {
+		pr_err("qmi MSA mem ready request rejected, result:%d error:%d\n",
+		       resp->resp.result, resp->resp.error);

Clean up the print statement, there's no reason to print resp->resp.result, it is 1. And use dev_err().

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

Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.c +1 -0
+		ret = -resp->resp.result;

This will be interpreted by future readers as an errno.

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

Quoted 3 lines
drivers/net/wireless/ath/ath10k/qmi.c +3 -0
+	}
+
+	pr_debug("MSA mem ready request completed\n");

dev_dbg().

Quoted 48 lines
drivers/net/wireless/ath/ath10k/qmi.c +33 -1
+	kfree(resp);
+	kfree(req);
+	return 0;
+
+out:
+	kfree(resp);
+	kfree(req);
+	return ret;
+}
+
+static int
 ath10k_qmi_ind_register_send_sync_msg(struct ath10k_qmi *qmi)
 {
 	struct wlfw_ind_register_resp_msg_v01 *resp;

@@ -173,7 +409,27 @@ static void ath10k_qmi_event_server_arrive(struct work_struct *work)
 	if (ret)
 		return;
 
-	ath10k_qmi_ind_register_send_sync_msg(qmi);
+	ret = ath10k_qmi_ind_register_send_sync_msg(qmi);
+	if (ret)
+		return;
+
+	ret = ath10k_qmi_msa_mem_info_send_sync_msg(qmi);
+	if (ret)
+		return;
+
+	ret = ath10k_qmi_setup_msa_permissions(qmi);
+	if (ret)
+		return;
+
+	ret = ath10k_qmi_msa_ready_send_sync_msg(qmi);
+	if (ret)
+		goto err_setup_msa;
+
+	return;
+
+err_setup_msa:
+	ath10k_qmi_remove_msa_permissions(qmi);
+	return;
 }
 
 static void ath10k_qmi_event_server_exit(struct work_struct *work)

@@ -252,6 +508,34 @@ static int ath10k_alloc_qmi_resources(struct ath10k_qmi *qmi)
 	return ret;
 }
 
+static int ath10k_qmi_setup_msa_resources(struct device *dev,

drivers/net/wireless/ath/ath10k/qmi.c static int ath10k_alloc_qmi_resources(struct ath10k_qmi *qmi) · line 521

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +10 -0
+					  struct ath10k_qmi *qmi)
+{
+	int ret;
+
+	ret = of_property_read_u32(dev->of_node, "qcom,wlan-msa-memory",
+				   &qmi->msa_mem_size);
+
+	if (ret || qmi->msa_mem_size == 0) {
+		pr_err("fail to get MSA memory size: %u, ret: %d\n",
+		       qmi->msa_mem_size, ret);

Use dev_err() and drop the msa_mem_size and ret from the printout, it doesn't add any value.

drivers/net/wireless/ath/ath10k/qmi.c static int ath10k_alloc_qmi_resources(struct ath10k_qmi *qmi) · line 526

Quoted 5 lines
drivers/net/wireless/ath/ath10k/qmi.c +5 -0
+		return -ENOMEM;
+	}
+
+	qmi->msa_va = dmam_alloc_coherent(dev, qmi->msa_mem_size,
+					  &qmi->msa_pa, GFP_KERNEL);

The third parameter is supposed to be a dma_addr_t, not a phys_addr_t. But it looks like you can just change the type.

drivers/net/wireless/ath/ath10k/qmi.c static int ath10k_alloc_qmi_resources(struct ath10k_qmi *qmi) · line 534

Quoted 8 lines
drivers/net/wireless/ath/ath10k/qmi.c +8 -0
+	if (!qmi->msa_va) {
+		pr_err("dma alloc failed for MSA\n");
+		return -ENOMEM;
+	}
+
+	pr_debug("MSA pa: %pa, MSA va: 0x%p\n",
+		 &qmi->msa_pa,
+		 qmi->msa_va);

dev_dbg() and the appropriate types are %pad and %pK.

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

Quoted 12 lines
drivers/net/wireless/ath/ath10k/qmi.c +4 -1
+
+	return 0;
+}
+
 static void ath10k_remove_qmi_resources(struct ath10k_qmi *qmi)
 {
 	cancel_work_sync(&qmi->work_svc_arrive);

@@ -272,7 +556,7 @@ static int ath10k_qmi_probe(struct platform_device *pdev)
 
 	qmi->pdev = pdev;
 	platform_set_drvdata(pdev, qmi);
-

This newline was a nice divider between the "chunks" of statements, like paragraphs in a book...

Quoted 18 lines
drivers/net/wireless/ath/ath10k/qmi.c +1 -0
+	ath10k_qmi_setup_msa_resources(&pdev->dev, qmi);
 	ret = ath10k_alloc_qmi_resources(qmi);
 	if (ret < 0)
 		goto err;
drivers/net/wireless/ath/ath10k/qmi.h +2 -0
index 7697d24..47af020 100644
@@ -16,6 +16,8 @@
 #ifndef _QMI_H_
 #define _QMI_H_
 
+#define MAX_NUM_MEMORY_REGIONS			2
+
 enum ath10k_qmi_driver_event_type {
 	ATH10K_QMI_EVENT_SERVER_ARRIVE,
 	ATH10K_QMI_EVENT_SERVER_EXIT,

drivers/net/wireless/ath/ath10k/qmi.h enum ath10k_qmi_driver_event_type { · line 28

Quoted 4 lines
drivers/net/wireless/ath/ath10k/qmi.h +1 -0
@@ -23,6 +25,12 @@ enum ath10k_qmi_driver_event_type {
 	ATH10K_QMI_EVENT_MAX,
 };
 
+struct ath10k_msa_mem_region_info {

Afaict this struct is only used in qmi.c, so move it there.

drivers/net/wireless/ath/ath10k/qmi.h enum ath10k_qmi_driver_event_type { · line 29

Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.h +1 -0
+	u64 reg_addr;

This is your driver-local copy of the data being received in a wlfw_msa_info_resp_msg_v01, use kernel native types instead.

I.e. make this phys_addr_t,

drivers/net/wireless/ath/ath10k/qmi.h enum ath10k_qmi_driver_event_type { · line 30

Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.h +1 -0
+	u32 size;

this is a size_t

drivers/net/wireless/ath/ath10k/qmi.h enum ath10k_qmi_driver_event_type { · line 31

Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.h +1 -0
+	u8 secure_flag;

And based on how you use this, just make it bool secure (and invert the logic?)

Quoted 6 lines
drivers/net/wireless/ath/ath10k/qmi.h +2 -0
+};
+
 struct ath10k_qmi {
 	struct platform_device *pdev;
 	struct qmi_handle qmi_hdl;

drivers/net/wireless/ath/ath10k/qmi.h struct ath10k_qmi { · line 50

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.h +6 -0
@@ -33,5 +41,11 @@ struct ath10k_qmi {
 	struct work_struct work_svc_exit;
 	struct workqueue_struct *event_wq;
 	spinlock_t event_lock; /* spinlock for fw ready status*/
+	u32 nr_mem_region;
+	struct ath10k_msa_mem_region_info
+		mem_region[MAX_NUM_MEMORY_REGIONS];
+	phys_addr_t msa_pa;
+	u32 msa_mem_size;
+	void *msa_va;
 };

Regards,
Bjorn

author
#4 lore raw

Hi Bjorn,
Thanks for the review.

Quoted 30 lines · Bjorn Andersson
Quoted 28 lines · Govind Singh

HOST allocates 2mb of region for modem and WCN3990 secure access and provides the address and access control to target for secure access.
Add MSA handshake request/response messages.

Signed-off-by Govind Singh <[email protected]>
---
 drivers/net/wireless/ath/ath10k/qmi.c | 288 
+++++++++++++++++++++++++++++++++-
 drivers/net/wireless/ath/ath10k/qmi.h |  14 ++
 2 files changed, 300 insertions(+), 2 deletions(-)
drivers/net/wireless/ath/ath10k/qmi.c +2 -0
index bc80b8f..763b812 100644
@@ -26,6 +26,8 @@
 #include <linux/string.h>
 #include <net/sock.h>
 #include <linux/soc/qcom/qmi.h>
+#include <linux/qcom_scm.h>
+#include <linux/of.h>
 #include "qmi.h"
 #include "qmi_svc_v01.h"
 

@@ -35,6 +37,240 @@
 static struct ath10k_qmi *qmi;
Quoted 16 lines

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

Quoted 13 lines
drivers/net/wireless/ath/ath10k/qmi.c +11 -0
 static int
+ath10k_qmi_map_msa_permissions(struct ath10k_msa_mem_region_info 
 *mem_region)
+{
+	struct qcom_scm_vmperm dst_perms[3];
+	unsigned int src_perms;
+	phys_addr_t addr;
+	u32 perm_count;
+	u32 size;
+	int ret;
+
+	addr = mem_region->reg_addr;
+	size = mem_region->size;

Skip the local variables.

Sure, will do in next version.

Quoted 4 lines
Quoted 4 lines
drivers/net/wireless/ath/ath10k/qmi.c +4 -0
+
+	src_perms = BIT(QCOM_SCM_VMID_HLOS);
+
+	dst_perms[0].vmid = QCOM_SCM_VMID_MSS_MSA;
Quoted 9 lines

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

Quoted 5 lines
drivers/net/wireless/ath/ath10k/qmi.c +5 -0
+	dst_perms[0].perm = QCOM_SCM_PERM_RW;
+	dst_perms[1].vmid = QCOM_SCM_VMID_WLAN;
+	dst_perms[1].perm = QCOM_SCM_PERM_RW;
+
+	if (!mem_region->secure_flag) {

So with secure_flag equal to 0 we give less subsystems access to the data? Is this logic inverted?

Yes with secure flag mean less privileges i,e: Copy engine hardware can not access the region.

Quoted 11 lines

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

Quoted 7 lines
drivers/net/wireless/ath/ath10k/qmi.c +7 -0
+		dst_perms[2].vmid = QCOM_SCM_VMID_WLAN_CE;
+		dst_perms[2].perm = QCOM_SCM_PERM_RW;
+		perm_count = 3;
+	} else {
+		dst_perms[2].vmid = 0;
+		dst_perms[2].perm = 0;
+		perm_count = 2;

If you set perm_count to 2 you don't need to clear vmid and perm of dst_perms[2].

ok, will remove.

Quoted 19 lines

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

Quoted 6 lines
drivers/net/wireless/ath/ath10k/qmi.c +6 -0
+	}
+
+	ret = qcom_scm_assign_mem(addr, size, &src_perms,
+				  dst_perms, perm_count);
+	if (ret < 0)
+		pr_err("msa map permission failed=%d\n", ret);

Use dev_err()

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +9 -0
+
+	return ret;
+}
+
+static int
+ath10k_qmi_unmap_msa_permissions(struct ath10k_msa_mem_region_info 
 *mem_region)
+{
+	struct qcom_scm_vmperm dst_perms;
+	unsigned int src_perms;
Quoted 9 lines

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

Quoted 6 lines
drivers/net/wireless/ath/ath10k/qmi.c +6 -0
+	phys_addr_t addr;
+	u32 size;
+	int ret;
+
+	addr = mem_region->reg_addr;
+	size = mem_region->size;

Skip the local variables.

Sure, will do in next version.

Quoted 17 lines
Quoted 16 lines
drivers/net/wireless/ath/ath10k/qmi.c +16 -0
+
+	src_perms = BIT(QCOM_SCM_VMID_MSS_MSA) | BIT(QCOM_SCM_VMID_WLAN);
+
+	if (!mem_region->secure_flag)
+		src_perms |= BIT(QCOM_SCM_VMID_WLAN_CE);
+
+	dst_perms.vmid = QCOM_SCM_VMID_HLOS;
+	dst_perms.perm = QCOM_SCM_PERM_RW;
+
+	ret = qcom_scm_assign_mem(addr, size, &src_perms, &dst_perms, 1);
+	if (ret < 0)
+		pr_err("msa unmap permission failed=%d\n", ret);
+
+	return ret;
+}
+
Quoted 7 lines

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

Quoted 4 lines
drivers/net/wireless/ath/ath10k/qmi.c +4 -0
+static int
+	ath10k_qmi_msa_mem_info_send_sync_msg(struct ath10k_qmi *qmi)
+{
+	struct wlfw_msa_info_resp_msg_v01 *resp;

This is 40 bytes,

Sure, will do in next version for all instances.

Quoted 6 lines

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

Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.c +1 -0
+	struct wlfw_msa_info_req_msg_v01 *req;

This is 6 bytes.

So just put them on the stack and skip the memory management.

Sure, will do in next version.

Quoted 18 lines
Quoted 18 lines
drivers/net/wireless/ath/ath10k/qmi.c +18 -0
+	struct qmi_txn txn;
+	int ret;
+	int i;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (!req)
+		return -ENOMEM;
+
+	resp = kzalloc(sizeof(*resp), GFP_KERNEL);
+	if (!resp) {
+		kfree(req);
+		return -ENOMEM;
+	}
+
+	req->msa_addr = qmi->msa_pa;
+	req->size = qmi->msa_mem_size;
+
+	ret = qmi_txn_init(&qmi->qmi_hdl, &txn,
Quoted 9 lines

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

Quoted 4 lines
drivers/net/wireless/ath/ath10k/qmi.c +4 -0
+			   wlfw_msa_info_resp_msg_v01_ei, resp);
+	if (ret < 0) {
+		pr_err("fail to init txn for MSA mem info resp %d\n",
+		       ret);

Unless we have 2 billion outstanding transactions "ret" is going to be ENOMEM here, in which case there is already a printout telling you about the problem.

Sure, will remove this redundant logging.

Quoted 13 lines

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

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +10 -0
+		goto out;
+	}
+
+	ret = qmi_send_request(&qmi->qmi_hdl, NULL, &txn,
+			       QMI_WLFW_MSA_INFO_REQ_V01,
+			       WLFW_MSA_INFO_REQ_MSG_V01_MAX_MSG_LEN,
+			       wlfw_msa_info_req_msg_v01_ei, req);
+	if (ret < 0) {
+		qmi_txn_cancel(&txn);
+		pr_err("fail to send MSA mem info req %d\n", ret);

Use dev_err().

Quoted 8 lines

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

Quoted 4 lines
drivers/net/wireless/ath/ath10k/qmi.c +4 -0
+		goto out;
+	}
+
+	ret = qmi_txn_wait(&txn, WLFW_TIMEOUT * HZ);

The timeout is in jiffies, but I doubt you intended to the timeout to be 500 seconds either.

yes, its pretty high. I will reduce this.

Quoted 24 lines

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

Quoted 7 lines
drivers/net/wireless/ath/ath10k/qmi.c +7 -0
+	if (ret < 0)
+		goto out;
+
+	if (resp->resp.result != QMI_RESULT_SUCCESS_V01) {
+		pr_err("MSA mem info request rejected, result:%d error:%d\n",
+		       resp->resp.result, resp->resp.error);
+		ret = -resp->resp.result;

Future readers will expect this to be a errno, in particular as most other exit paths return errnos.

Quoted 13 lines
drivers/net/wireless/ath/ath10k/qmi.c +12 -0
+		goto out;
+	}
+
+	pr_debug("receive mem_region_info_len: %d\n",
+		 resp->mem_region_info_len);
+
+	if (resp->mem_region_info_len > QMI_WLFW_MAX_NUM_MEMORY_REGIONS_V01) 
 {
+		pr_err("invalid memory region length received: %d\n",
+		       resp->mem_region_info_len);
+		ret = -EINVAL;
+		goto out;
+	}
Quoted 9 lines

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

Quoted 5 lines
drivers/net/wireless/ath/ath10k/qmi.c +5 -0
+
+	qmi->nr_mem_region = resp->mem_region_info_len;
+	for (i = 0; i < resp->mem_region_info_len; i++) {
+		qmi->mem_region[i].reg_addr =
+			resp->mem_region_info[i].region_addr;

With the use of a local variable you should be able to avoid the line breaks here.

Sure, will do in next version.

Quoted 161 lines

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

Quoted 12 lines
drivers/net/wireless/ath/ath10k/qmi.c +12 -0
+
+	ret = qmi_send_request(&qmi->qmi_hdl, NULL, &txn,
+			       QMI_WLFW_MSA_READY_REQ_V01,
+			       WLFW_MSA_READY_REQ_MSG_V01_MAX_MSG_LEN,
+			       wlfw_msa_ready_req_msg_v01_ei, req);
+	if (ret < 0) {
+		qmi_txn_cancel(&txn);
+		pr_err("fail to send MSA mem ready req %d\n", ret);
+		goto out;
+	}
+
+	ret = qmi_txn_wait(&txn, WLFW_TIMEOUT * HZ);

As above.

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

Quoted 6 lines
drivers/net/wireless/ath/ath10k/qmi.c +6 -0
+	if (ret < 0)
+		goto out;
+
+	if (resp->resp.result != QMI_RESULT_SUCCESS_V01) {
+		pr_err("qmi MSA mem ready request rejected, result:%d error:%d\n",
+		       resp->resp.result, resp->resp.error);

Clean up the print statement, there's no reason to print resp->resp.result, it is 1. And use dev_err().

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

Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.c +1 -0
+		ret = -resp->resp.result;

This will be interpreted by future readers as an errno.

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

Quoted 3 lines
drivers/net/wireless/ath/ath10k/qmi.c +3 -0
+	}
+
+	pr_debug("MSA mem ready request completed\n");

dev_dbg().

Quoted 50 lines
drivers/net/wireless/ath/ath10k/qmi.c +32 -1
+	kfree(resp);
+	kfree(req);
+	return 0;
+
+out:
+	kfree(resp);
+	kfree(req);
+	return ret;
+}
+
+static int
 ath10k_qmi_ind_register_send_sync_msg(struct ath10k_qmi *qmi)
 {
 	struct wlfw_ind_register_resp_msg_v01 *resp;

@@ -173,7 +409,27 @@ static void ath10k_qmi_event_server_arrive(struct 
 work_struct *work)
 	if (ret)
 		return;
 
-	ath10k_qmi_ind_register_send_sync_msg(qmi);
+	ret = ath10k_qmi_ind_register_send_sync_msg(qmi);
+	if (ret)
+		return;
+
+	ret = ath10k_qmi_msa_mem_info_send_sync_msg(qmi);
+	if (ret)
+		return;
+
+	ret = ath10k_qmi_setup_msa_permissions(qmi);
+	if (ret)
+		return;
+
+	ret = ath10k_qmi_msa_ready_send_sync_msg(qmi);
+	if (ret)
+		goto err_setup_msa;
+
+	return;
+
+err_setup_msa:
+	ath10k_qmi_remove_msa_permissions(qmi);
+	return;
 }

static void ath10k_qmi_event_server_exit(struct work_struct *work)

drivers/net/wireless/ath/ath10k/qmi.c +1 -0
@@ -252,6 +508,34 @@ static int ath10k_alloc_qmi_resources(struct 
 ath10k_qmi *qmi)
 	return ret;
 }
 
+static int ath10k_qmi_setup_msa_resources(struct device *dev,

drivers/net/wireless/ath/ath10k/qmi.c static int ath10k_alloc_qmi_resources(struct · line 522

Quoted 10 lines
drivers/net/wireless/ath/ath10k/qmi.c +10 -0
+					  struct ath10k_qmi *qmi)
+{
+	int ret;
+
+	ret = of_property_read_u32(dev->of_node, "qcom,wlan-msa-memory",
+				   &qmi->msa_mem_size);
+
+	if (ret || qmi->msa_mem_size == 0) {
+		pr_err("fail to get MSA memory size: %u, ret: %d\n",
+		       qmi->msa_mem_size, ret);

Use dev_err() and drop the msa_mem_size and ret from the printout, it doesn't add any value.

drivers/net/wireless/ath/ath10k/qmi.c static int ath10k_alloc_qmi_resources(struct ath10k_qmi *qmi) · line 526

Quoted 5 lines
drivers/net/wireless/ath/ath10k/qmi.c +5 -0
+		return -ENOMEM;
+	}
+
+	qmi->msa_va = dmam_alloc_coherent(dev, qmi->msa_mem_size,
+					  &qmi->msa_pa, GFP_KERNEL);

The third parameter is supposed to be a dma_addr_t, not a phys_addr_t. But it looks like you can just change the type.

drivers/net/wireless/ath/ath10k/qmi.c static int ath10k_alloc_qmi_resources(struct ath10k_qmi *qmi) · line 534

Quoted 8 lines
drivers/net/wireless/ath/ath10k/qmi.c +8 -0
+	if (!qmi->msa_va) {
+		pr_err("dma alloc failed for MSA\n");
+		return -ENOMEM;
+	}
+
+	pr_debug("MSA pa: %pa, MSA va: 0x%p\n",
+		 &qmi->msa_pa,
+		 qmi->msa_va);

dev_dbg() and the appropriate types are %pad and %pK.

drivers/net/wireless/ath/ath10k/qmi.c static int ath10k_qmi_probe(struct platform_device · line 276

Quoted 13 lines
drivers/net/wireless/ath/ath10k/qmi.c +4 -0
+
+	return 0;
+}
+
 static void ath10k_remove_qmi_resources(struct ath10k_qmi *qmi)
 {
drivers/net/wireless/ath/ath10k/qmi.c +0 -1
 	cancel_work_sync(&qmi->work_svc_arrive);

@@ -272,7 +556,7 @@ static int ath10k_qmi_probe(struct platform_device 
 *pdev)
 
 	qmi->pdev = pdev;
 	platform_set_drvdata(pdev, qmi);
-

This newline was a nice divider between the "chunks" of statements, like paragraphs in a book...

Quoted 18 lines
drivers/net/wireless/ath/ath10k/qmi.c +1 -0
+	ath10k_qmi_setup_msa_resources(&pdev->dev, qmi);
 	ret = ath10k_alloc_qmi_resources(qmi);
 	if (ret < 0)
drivers/net/wireless/ath/ath10k/qmi.c +0 -0
 		goto err;
drivers/net/wireless/ath/ath10k/qmi.h +2 -0
index 7697d24..47af020 100644
@@ -16,6 +16,8 @@
 #ifndef _QMI_H_
 #define _QMI_H_
 
+#define MAX_NUM_MEMORY_REGIONS			2
+
 enum ath10k_qmi_driver_event_type {
 	ATH10K_QMI_EVENT_SERVER_ARRIVE,
 	ATH10K_QMI_EVENT_SERVER_EXIT,

drivers/net/wireless/ath/ath10k/qmi.h enum ath10k_qmi_driver_event_type { · line 28

Quoted 4 lines
drivers/net/wireless/ath/ath10k/qmi.h +1 -0
@@ -23,6 +25,12 @@ enum ath10k_qmi_driver_event_type {
 	ATH10K_QMI_EVENT_MAX,
 };
 
+struct ath10k_msa_mem_region_info {

Afaict this struct is only used in qmi.c, so move it there.

Quoted 7 lines

drivers/net/wireless/ath/ath10k/qmi.h enum ath10k_qmi_driver_event_type { · line 29

Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.h +1 -0
+	u64 reg_addr;

This is your driver-local copy of the data being received in a wlfw_msa_info_resp_msg_v01, use kernel native types instead.

I.e. make this phys_addr_t,

Sure, will do in next version.

Quoted 9 lines

drivers/net/wireless/ath/ath10k/qmi.h enum ath10k_qmi_driver_event_type { · line 30

Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.h +1 -0
+	u32 size;

this is a size_t

drivers/net/wireless/ath/ath10k/qmi.h enum ath10k_qmi_driver_event_type { · line 31

Quoted 1 line
drivers/net/wireless/ath/ath10k/qmi.h +1 -0
+	u8 secure_flag;

And based on how you use this, just make it bool secure (and invert the logic?)

Sure, will do in next version.

Quoted 11 lines
Quoted 11 lines
drivers/net/wireless/ath/ath10k/qmi.h +4 -0
+};
+
 struct ath10k_qmi {
 	struct platform_device *pdev;
 	struct qmi_handle qmi_hdl;

@@ -33,5 +41,11 @@ struct ath10k_qmi {
 	struct work_struct work_svc_exit;
 	struct workqueue_struct *event_wq;
 	spinlock_t event_lock; /* spinlock for fw ready status*/
+	u32 nr_mem_region;
+	struct ath10k_msa_mem_region_info
Quoted 8 lines

drivers/net/wireless/ath/ath10k/qmi.h struct ath10k_qmi { · line 50

Quoted 5 lines
drivers/net/wireless/ath/ath10k/qmi.h +4 -0
+		mem_region[MAX_NUM_MEMORY_REGIONS];
+	phys_addr_t msa_pa;
+	u32 msa_mem_size;
+	void *msa_va;
 };

Regards,
Bjorn

BR,
Govind