Re: [PATCH 07/12] ath10k: Add MSA handshake QMI mgs support

[Govind Singh]

Hi Bjorn,
Thanks for the review.


On 2018-05-12 00:13, Bjorn Andersson wrote:

On Sun 25 Mar 22:40 PDT 2018, Govind Singh wrote:


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 
---
 drivers/net/wireless/ath/ath10k/qmi.c | 288 
+-

 drivers/net/wireless/ath/ath10k/qmi.h |  14 ++
 2 files changed, 300 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/qmi.c 
b/drivers/net/wireless/ath/ath10k/qmi.c

index bc80b8f..763b812 100644
--- a/drivers/net/wireless/ath/ath10k/qmi.c
+++ b/drivers/net/wireless/ath/ath10k/qmi.c
@@ -26,6 +26,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #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;


Skip the local variables.



Sure, will do in next version.


+
+   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?



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


+   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.


+   }
+
+   ret = qcom_scm_assign_mem(addr, size, _perms,
+ dst_perms, perm_count);
+   if (ret < 0)
+   pr_err("msa map permission failed=%d\n", ret);


Use dev_err()


+
+   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;


Skip the local variables.



Sure, will do in next version.


+
+   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, _perms, _perms, 1);
+   if (ret < 0)
+   pr_err("msa unmap permission failed=%d\n", ret);
+
+   return ret;
+}
+



+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.


+   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.


+   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_hdl, ,
+  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.


+   goto out;
+   }
+
+   ret = qmi_send_request(>qmi_hdl, NULL, ,
+  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();
+   pr_err("fail to send MSA mem info req %d\n", ret);


Use dev_err().


+   goto out;
+   }
+
+   ret = qmi_txn_wait(, WLFW_TIMEOUT * HZ);


The timeout is in jiffies, but 

Re: [PATCH 07/12] ath10k: Add MSA handshake QMI mgs support

[Bjorn Andersson]

On Sun 25 Mar 22:40 PDT 2018, Govind Singh wrote:

> 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 
> ---
>  drivers/net/wireless/ath/ath10k/qmi.c | 288 
> +-
>  drivers/net/wireless/ath/ath10k/qmi.h |  14 ++
>  2 files changed, 300 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath10k/qmi.c 
> b/drivers/net/wireless/ath/ath10k/qmi.c
> index bc80b8f..763b812 100644
> --- a/drivers/net/wireless/ath/ath10k/qmi.c
> +++ b/drivers/net/wireless/ath/ath10k/qmi.c
> @@ -26,6 +26,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #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;

Skip the local variables.

> +
> + 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?

> + 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].

> + }
> +
> + ret = qcom_scm_assign_mem(addr, size, _perms,
> +   dst_perms, perm_count);
> + if (ret < 0)
> + pr_err("msa map permission failed=%d\n", ret);

Use dev_err()

> +
> + 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;

Skip the local variables.

> +
> + 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, _perms, _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(>mem_region[i]);
> + if (ret)
> + goto err_unmap;
> + }
> +
> + return 0;
> +
> +err_unmap:
> + for (i--; i >= 0; i--)
> + ath10k_qmi_unmap_msa_permissions(>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(>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,

> + struct wlfw_msa_info_req_msg_v01 *req;

This is 6 bytes.

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

> + 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_hdl, ,
> +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.

> + goto out;
> + }
> +
> + ret = qmi_send_request(>qmi_hdl, NULL, ,
> +QMI_WLFW_MSA_INFO_REQ_V01,
> +  

[PATCH 07/12] ath10k: Add MSA handshake QMI mgs support

[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 
---
 drivers/net/wireless/ath/ath10k/qmi.c | 288 +-
 drivers/net/wireless/ath/ath10k/qmi.h |  14 ++
 2 files changed, 300 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/qmi.c 
b/drivers/net/wireless/ath/ath10k/qmi.c
index bc80b8f..763b812 100644
--- a/drivers/net/wireless/ath/ath10k/qmi.c
+++ b/drivers/net/wireless/ath/ath10k/qmi.c
@@ -26,6 +26,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #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, _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, _perms, _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(>mem_region[i]);
+   if (ret)
+   goto err_unmap;
+   }
+
+   return 0;
+
+err_unmap:
+   for (i--; i >= 0; i--)
+   ath10k_qmi_unmap_msa_permissions(>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(>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_hdl, ,
+  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_hdl, NULL, ,
+  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();
+   pr_err("fail to send MSA mem info req %d\n", ret);
+   goto out;
+   }
+
+   ret = qmi_txn_wait(, 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: