diff mbox

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

Message ID 1522042832-25975-1-git-send-email-govinds@codeaurora.org (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show

Commit Message

Govind Singh March 26, 2018, 5:40 a.m. UTC
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 <govinds@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/qmi.c | 288 +++++++++++++++++++++++++++++++++-
 drivers/net/wireless/ath/ath10k/qmi.h |  14 ++
 2 files changed, 300 insertions(+), 2 deletions(-)

Comments

Kalle Valo March 28, 2018, 3:21 p.m. UTC | #1
Govind Singh <govinds@codeaurora.org> 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 <govinds@codeaurora.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

This added a new ath10k-check warning:

drivers/net/wireless/ath/ath10k/qmi.c:433: void function return statements are not generally useful
Bjorn Andersson May 11, 2018, 6:43 p.m. UTC | #2
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 <govinds@codeaurora.org>
> ---
>  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
Govind Singh May 14, 2018, 1:56 p.m. UTC | #3
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 <govinds@codeaurora.org>
>> ---
>>  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
diff mbox

Patch

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_ */