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(-)
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 <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;
diff --git a/drivers/net/wireless/ath/ath10k/qmi.h b/drivers/net/wireless/ath/ath10k/qmi.h
index 7697d24..47af020 100644
--- a/drivers/net/wireless/ath/ath10k/qmi.h
+++ b/drivers/net/wireless/ath/ath10k/qmi.h
@@ -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_ */
--
1.9.1
Govind Singh <[email protected]> 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 <[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
--
https://patchwork.kernel.org/patch/10307183/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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 <[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(-)
>>
>> 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 <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;
>
> 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, &src_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, &src_perms, &dst_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->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.
>
Sure, will remove this redundant logging.
>> + 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().
>
>> + 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.
>> + 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.
>
>> + 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;
>
> With the use of a local variable you should be able to avoid the line
> breaks here.
>
Sure, will do in next version.
>> +
>> + 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.
>
>> + 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().
>
>> + ret = -resp->resp.result;
>
> This will be interpreted by future readers as an errno.
>
>> + }
>> +
>> + pr_debug("MSA mem ready request completed\n");
>
> dev_dbg().
>
>> + 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);
>
> Use dev_err() and drop the msa_mem_size and ret from the printout, it
> doesn't add any value.
>
>> + 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.
>
>> + 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.
>
>> +
>> + 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...
>
>> + ath10k_qmi_setup_msa_resources(&pdev->dev, qmi);
>> ret = ath10k_alloc_qmi_resources(qmi);
>> if (ret < 0)
>> goto err;
>> diff --git a/drivers/net/wireless/ath/ath10k/qmi.h
>> b/drivers/net/wireless/ath/ath10k/qmi.h
>> index 7697d24..47af020 100644
>> --- a/drivers/net/wireless/ath/ath10k/qmi.h
>> +++ b/drivers/net/wireless/ath/ath10k/qmi.h
>> @@ -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 {
>
> Afaict this struct is only used in qmi.c, so move it there.
>
>> + 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.
>> + u32 size;
>
> this is a size_t
>
>> + 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.
>> +};
>> +
>> 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;
>> };
>
> Regards,
> Bjorn
BR,
Govind
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 <[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(-)
>
> 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 <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;
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, &src_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, &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;
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->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.
> + 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().
> + 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.
> + 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.
> + 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;
With the use of a local variable you should be able to avoid the line
breaks here.
> + 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;
This is 4 bytes,
> + struct wlfw_msa_ready_req_msg_v01 *req;
and this is 1 byte, use the stack.
> + 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);
As above.
> + 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().
> + ret = -resp->resp.result;
This will be interpreted by future readers as an errno.
> + }
> +
> + pr_debug("MSA mem ready request completed\n");
dev_dbg().
> + 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);
Use dev_err() and drop the msa_mem_size and ret from the printout, it
doesn't add any value.
> + 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.
> + 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.
> +
> + 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...
> + ath10k_qmi_setup_msa_resources(&pdev->dev, qmi);
> ret = ath10k_alloc_qmi_resources(qmi);
> if (ret < 0)
> goto err;
> diff --git a/drivers/net/wireless/ath/ath10k/qmi.h b/drivers/net/wireless/ath/ath10k/qmi.h
> index 7697d24..47af020 100644
> --- a/drivers/net/wireless/ath/ath10k/qmi.h
> +++ b/drivers/net/wireless/ath/ath10k/qmi.h
> @@ -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 {
Afaict this struct is only used in qmi.c, so move it there.
> + 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,
> + u32 size;
this is a size_t
> + u8 secure_flag;
And based on how you use this, just make it bool secure (and invert the
logic?)
> +};
> +
> 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;
> };
Regards,
Bjorn