lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5337a6fb-063d-4abe-9ab2-eb0f417dbc55@quicinc.com>
Date: Fri, 3 Jan 2025 17:59:22 +0530
From: Sricharan Ramabadhran <quic_srichara@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, <jassisinghbrar@...il.com>,
        <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <andersson@...nel.org>,
        <konradybcio@...nel.org>, <manivannan.sadhasivam@...aro.org>,
        <dmitry.baryshkov@...aro.org>
Subject: Re: [PATCH V2 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox
 driver



On 12/31/2024 1:36 PM, Krzysztof Kozlowski wrote:
> On 31/12/2024 06:49, Sricharan R wrote:
>> From: Sricharan Ramabadhran <quic_srichara@...cinc.com>
>>
>> This mailbox facilitates the communication between the TME-L server based
>> subsystems (Q6) and the TME-L client (APPSS/BTSS/AUDIOSS), used for security
> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
> 
> Thank you.
> </form letter>
> 

ho, will recheck and fix missed comment.

> Still wrong wrapping.
> 
ok, will fix.

>> services like secure image authentication, enable/disable efuses, crypto
>> services. Each client in the   SoC has its own block of message RAM and IRQ
>> for communication with the TME-L SS. The protocol used to communicate in the
>> message RAM is known as Qualcomm Messaging Protocol (QMP).
>>
>> Remote proc driver subscribes to this mailbox and uses the mbox_send_message
>> to use TME-L to securely authenticate/teardown the images.
>>
>> Signed-off-by: Sricharan Ramabadhran <quic_srichara@...cinc.com>
>> ---
>>    [v2] Added worker for mailbox tx processing, since some of the operations can sleep
>>         Fixed checkpatch warnings. Some [CHECK] like below still exist, but that looks
>>         like a false postive.
>>
>>         CHECK: Macro argument 'm' may be better as '(m)' to avoid precedence issues
>>          #1072: FILE: include/linux/mailbox/tmelcom-qmp.h:40:
>>          +#define TMEL_MSG_UID_CREATE(m, a)      ((u32)(((m & 0xff) << 8) | (a & 0xff)))
>>
>>   drivers/mailbox/Kconfig             |   7 +
>>   drivers/mailbox/Makefile            |   2 +
>>   drivers/mailbox/qcom-tmel-qmp.c     | 971 ++++++++++++++++++++++++++++
>>   include/linux/mailbox/tmelcom-qmp.h | 157 +++++
>>   4 files changed, 1137 insertions(+)
>>   create mode 100644 drivers/mailbox/qcom-tmel-qmp.c
>>   create mode 100644 include/linux/mailbox/tmelcom-qmp.h
>>
>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> index 8ecba7fb999e..8ad0b834d617 100644
>> --- a/drivers/mailbox/Kconfig
>> +++ b/drivers/mailbox/Kconfig
>> @@ -306,4 +306,11 @@ config THEAD_TH1520_MBOX
>>   	  kernel is running, and E902 core used for power management among other
>>   	  things.
>>   
>> +config QCOM_TMEL_QMP_MAILBOX
> 
> Did you just place it at the end instead of gropped or sorted?
> 
Ho, placed it in end, will fix it.

>> +	tristate "QCOM Mailbox Protocol(QMP) for TME-L SS"
>> +	help
>> +	  Say yes to add support for the QMP Mailbox Protocol driver for TME-L.
>> +	  QMP is a lightweight communication protocol for sending messages to
>> +	  TME-L. This protocol fits into the Generic Mailbox Framework.
>> +	  QMP uses a mailbox registers.
>>   endif
>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>> index 5f4f5b0ce2cc..4dba283a94ad 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -66,3 +66,5 @@ obj-$(CONFIG_QCOM_CPUCP_MBOX)	+= qcom-cpucp-mbox.o
>>   obj-$(CONFIG_QCOM_IPCC)		+= qcom-ipcc.o
>>   
>>   obj-$(CONFIG_THEAD_TH1520_MBOX)	+= mailbox-th1520.o
>> +
>> +obj-$(CONFIG_QCOM_TMEL_QMP_MAILBOX) += qcom-tmel-qmp.o
> 
> Same problem.
> 
ok, will fix.

>> diff --git a/drivers/mailbox/qcom-tmel-qmp.c b/drivers/mailbox/qcom-tmel-qmp.c
>> new file mode 100644
>> index 000000000000..6de0a418e0ae
>> --- /dev/null
>> +++ b/drivers/mailbox/qcom-tmel-qmp.c
>> @@ -0,0 +1,971 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018,2020 The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/completion.h>
>> +#include <linux/delay.h>
>> +#include <linux/dma-direction.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/kthread.h>
>> +#include <linux/mailbox_client.h>
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/mailbox/tmelcom-qmp.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
> 
> Not used
> 
ok, will remove.

>> +#include <linux/of_platform.h>
> 
> Looks also not used
>
will remove.

>> +#include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/types.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/uio.h>
>> +#include <linux/workqueue.h>
> 
> 
> Several headers here look unused.
> 
ok, will check and fix.

>> +
>> +#define QMP_NUM_CHANS	0x1
>> +#define QMP_TOUT_MS	1000
>> +#define MBOX_ALIGN_BYTES	3
>> +#define QMP_CTRL_DATA_SIZE	4
>> +#define QMP_MAX_PKT_SIZE	0x18
>> +#define QMP_UCORE_DESC_OFFSET	0x1000
>> +
>> +#define QMP_CH_VAR_GET(mdev, desc, var) ((mdev)->desc.bits.var)
>> +#define QMP_CH_VAR_SET(mdev, desc, var) (mdev)->desc.bits.var = 1
>> +#define QMP_CH_VAR_CLR(mdev, desc, var) (mdev)->desc.bits.var = 0
>> +
>> +#define QMP_MCORE_CH_VAR_GET(mdev, var)	QMP_CH_VAR_GET(mdev, mcore, var)
>> +#define QMP_MCORE_CH_VAR_SET(mdev, var)	QMP_CH_VAR_SET(mdev, mcore, var)
>> +#define QMP_MCORE_CH_VAR_CLR(mdev, var)	QMP_CH_VAR_CLR(mdev, mcore, var)
>> +
>> +#define QMP_MCORE_CH_VAR_TOGGLE(mdev, var) \
>> +	(mdev)->mcore.bits.var = !((mdev)->mcore.bits.var)
>> +#define QMP_MCORE_CH_ACKED_CHECK(mdev, var) \
>> +	((mdev)->ucore.bits.var == (mdev)->mcore.bits.var##_ack)
>> +#define QMP_MCORE_CH_ACK_UPDATE(mdev, var) \
>> +	(mdev)->mcore.bits.var##_ack = (mdev)->ucore.bits.var
>> +#define QMP_MCORE_CH_VAR_ACK_CLR(mdev, var) \
>> +	(mdev)->mcore.bits.var##_ack = 0
>> +
>> +#define QMP_UCORE_CH_VAR_GET(mdev, var)	QMP_CH_VAR_GET(mdev, ucore, var)
>> +#define QMP_UCORE_CH_ACKED_CHECK(mdev, var) \
>> +	((mdev)->mcore.bits.var == (mdev)->ucore.bits.var##_ack)
>> +#define QMP_UCORE_CH_VAR_TOGGLED_CHECK(mdev, var) \
>> +	((mdev)->ucore.bits.var != (mdev)->mcore.bits.var##_ack)
>> +
>> +/**
>> + * enum qmp_local_state -	definition of the local state machine
>> + * @LINK_DISCONNECTED:		Init state, waiting for ucore to start
>> + * @LINK_NEGOTIATION:		Set local link state to up, wait for ucore ack
>> + * @LINK_CONNECTED:		Link state up, channel not connected
>> + * @LOCAL_CONNECTING:		Channel opening locally, wait for ucore ack
>> + * @CHANNEL_CONNECTED:		Channel fully opened
>> + * @LOCAL_DISCONNECTING:	Channel closing locally, wait for ucore ack
>> + */
>> +enum qmp_local_state {
>> +	LINK_DISCONNECTED,
>> +	LINK_NEGOTIATION,
>> +	LINK_CONNECTED,
>> +	LOCAL_CONNECTING,
>> +	CHANNEL_CONNECTED,
>> +	LOCAL_DISCONNECTING,
>> +};
>> +
>> +union channel_desc {
>> +	struct {
>> +		u32 link_state:1;
>> +		u32 link_state_ack:1;
>> +		u32 ch_state:1;
>> +		u32 ch_state_ack:1;
>> +		u32 tx:1;
>> +		u32 tx_ack:1;
>> +		u32 rx_done:1;
>> +		u32 rx_done_ack:1;
>> +		u32 read_int:1;
>> +		u32 read_int_ack:1;
>> +		u32 reserved:6;
>> +		u32 frag_size:8;
>> +		u32 rem_frag_count:8;
>> +	} bits;
>> +	unsigned int val;
>> +};
>> +
>> +struct qmp_work {
>> +	struct work_struct work;
>> +	void *data;
>> +};
>> +
>> +/**
>> + * struct qmp_device - local information for managing a single mailbox
>> + * @dev:	    The device that corresponds to this mailbox
>> + * @ctrl:	    The mbox controller for this mailbox
>> + * @mcore_desc:	    Local core (APSS) mailbox descriptor
>> + * @ucore_desc:	    Remote core (TME-L) mailbox descriptor
>> + * @mcore:	    Local core (APSS) channel descriptor
>> + * @ucore:	    Remote core (TME-L) channel descriptor
>> + * @rx_pkt:	    Buffer to pass to client, holds received data from mailbox
>> + * @tx_pkt:	    Buffer from client, holds data to send on mailbox
>> + * @mbox_client:    Mailbox client for the IPC interrupt
>> + * @mbox_chan:	    Mailbox client chan for the IPC interrupt
>> + * @local_state:    Current state of mailbox protocol
>> + * @state_lock:	    Serialize mailbox state changes
>> + * @tx_lock:	    Serialize access for writes to mailbox
>> + * @link_complete:  Use to block until link negotiation with remote proc
>> + * @ch_complete:    Use to block until the channel is fully opened
>> + * @dwork:	    Delayed work to detect timed out tx
>> + * @tx_sent:	    True if tx is sent and remote proc has not sent ack
>> + */
>> +struct qmp_device {
>> +	struct device *dev;
>> +	struct mbox_controller ctrl;
>> +	struct qmp_work qwork;
>> +
>> +	void __iomem *mcore_desc;
>> +	void __iomem *ucore_desc;
>> +	union channel_desc mcore;
>> +	union channel_desc ucore;
>> +
>> +	struct kvec rx_pkt;
>> +	struct kvec tx_pkt;
>> +
>> +	struct mbox_client mbox_client;
>> +	struct mbox_chan *mbox_chan;
>> +
>> +	enum qmp_local_state local_state;
>> +
>> +	/* Lock for QMP link state changes */
> 
> Vague
ok, whole of the locking needs some fixing. Infact, with little
more changes, no need to have the 2 mutexes. will remove them and
just have the spinlock for updates to m/u core registers and
state machine tracking.

> 
>> +	struct mutex state_lock;
>> +	/* Lock to serialize access to mailbox */
> 
> No, I don't see serialized access to mailbox. I see some parts of access
> being protected. Write descriptive lock descriptions.
> 
 >
Yes, wrong comment. spinlock was to protect access to m/u core and
state machine updates. will fix it in next version.

>> +	spinlock_t tx_lock;
>> +
>> +	struct completion link_complete;
>> +	struct completion ch_complete;
>> +	struct delayed_work dwork;
>> +	void *data;
>> +
>> +	bool tx_sent;
>> +	bool ch_in_use;
>> +};
>> +
>> +struct tmel_msg_param_type_buf_in {
>> +	u32 buf;
>> +	u32 buf_len;
>> +};
>> +
>> +struct tmel_secboot_sec_auth_req {
>> +	u32 sw_id;
>> +	struct tmel_msg_param_type_buf_in elf_buf;
>> +	struct tmel_msg_param_type_buf_in region_list;
>> +	u32 relocate;
>> +} __packed;
>> +
>> +struct tmel_secboot_sec_auth_resp {
>> +	u32 first_seg_addr;
>> +	u32 first_seg_len;
>> +	u32 entry_addr;
>> +	u32 extended_error;
>> +	u32 status;
>> +} __packed;
>> +
>> +struct tmel_secboot_sec_auth {
>> +	struct tmel_secboot_sec_auth_req req;
>> +	struct tmel_secboot_sec_auth_resp resp;
>> +} __packed;
>> +
>> +struct tmel_secboot_teardown_req {
>> +	u32 sw_id;
>> +	u32 secondary_sw_id;
>> +} __packed;
>> +
>> +struct tmel_secboot_teardown_resp {
>> +	u32 status;
>> +} __packed;
>> +
>> +struct tmel_secboot_teardown {
>> +	struct tmel_secboot_teardown_req req;
>> +	struct tmel_secboot_teardown_resp resp;
>> +} __packed;
>> +
>> +struct tmel {
>> +	struct device *dev;
>> +	struct qmp_device *mdev;
>> +	struct kvec pkt;
>> +	/* To serialize incoming tmel request */
> 
> No, explain what is exactly protected. We all know that mutex serializes...
> 
ok, will remove this mutex in next version.

>> +	struct mutex lock;
>> +	struct tmel_ipc_pkt *ipc_pkt;
>> +	dma_addr_t sram_dma_addr;
>> +	wait_queue_head_t waitq;
>> +	bool rx_done;
>> +};
>> +
>> +static struct tmel *tmeldev;
> 
> NAK, First: it is not needed, second: even if it in this spaghetti code
> it was needed, answer would be drop it and fix your code not to create
> fake singletons.
> 
ok, no need to have this global. with updated design will remove this
in next spin. Will have it instance specific.

>> +
>> +/**
>> + * qmp_send_irq() - send an irq to a remote entity as an event signal.
>> + * @mdev:       Which remote entity that should receive the irq.
>> + */
>> +static void qmp_send_irq(struct qmp_device *mdev)
>> +{
>> +	/* Update the mcore val to mcore register */
>> +	iowrite32(mdev->mcore.val, mdev->mcore_desc);
>> +	/* Ensure desc update is visible before IPC */
>> +	wmb();
>> +
>> +	dev_dbg(mdev->dev, "%s: mcore 0x%x ucore 0x%x", __func__,
>> +		mdev->mcore.val, mdev->ucore.val);
>> +
>> +	mbox_send_message(mdev->mbox_chan, NULL);
>> +	mbox_client_txdone(mdev->mbox_chan, 0);
>> +}
>> +
> 
> ...
> 
>> +static irqreturn_t qmp_irq_handler(int irq, void *priv)
>> +{
>> +	struct qmp_device *mdev = (struct qmp_device *)priv;
>> +
>> +	qmp_rx(mdev);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int tmel_qmp_parse_devicetree(struct platform_device *pdev,
>> +				     struct qmp_device *mdev)
> 
> 
> Probe functions are always next to each other, not in other part of unit.
> 
ok, will fix it.

>> +{
>> +	struct device *dev = &pdev->dev;
>> +
>> +	mdev->mcore_desc = devm_platform_ioremap_resource(pdev, 0);
>> +	if (!mdev->mcore_desc) {
>> +		dev_err(dev, "ioremap failed for mcore reg\n");
>> +		return -EIO;
>> +	}
>> +
>> +	mdev->ucore_desc = mdev->mcore_desc + QMP_UCORE_DESC_OFFSET;
>> +
>> +	mdev->mbox_client.dev = dev;
>> +	mdev->mbox_client.knows_txdone = false;
>> +	mdev->mbox_chan = mbox_request_channel(&mdev->mbox_client, 0);
>> +	if (IS_ERR(mdev->mbox_chan)) {
>> +		dev_err(dev, "mbox chan for IPC is missing\n");
> 
> Syntax is: return dev_err_probe().
> 
ok, will fix it.

>> +		return PTR_ERR(mdev->mbox_chan);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void tmel_qmp_remove(struct platform_device *pdev)
> 
> Again, why remove call is not next to probe?
> 
ok, will fix it.

>> +{
>> +	struct qmp_device *mdev = platform_get_drvdata(pdev);
>> +
>> +	mbox_controller_unregister(&mdev->ctrl);
>> +	kfree(mdev->rx_pkt.iov_base);
> 
> I don't see this being allocated in probe.
> 
Ho  wrong place. Will fix and update with right kfree's.

>> +}
>> +
>> +static struct device *tmel_get_device(void)
>> +{
>> +	struct tmel *tdev = tmeldev;
> 
> Nope. Do not create singletons.
> 
ok, yeah, will be removed in next version.

>> +
>> +	if (!tdev)
>> +		return NULL;
>> +
>> +	return tdev->dev;
>> +}
>> +
>> +static int tmel_prepare_msg(struct tmel *tdev, u32 msg_uid,
>> +			    void *msg_buf, size_t msg_size)
>> +{
>> +	struct tmel_ipc_pkt *ipc_pkt = tdev->ipc_pkt;
>> +	struct ipc_header *msg_hdr = &ipc_pkt->msg_hdr;
>> +	struct mbox_payload *mbox_payload = &ipc_pkt->payload.mbox_payload;
>> +	struct sram_payload *sram_payload = &ipc_pkt->payload.sram_payload;
>> +	int ret;
>> +
>> +	memset(ipc_pkt, 0, sizeof(struct tmel_ipc_pkt));
>> +
>> +	msg_hdr->msg_type = TMEL_MSG_UID_MSG_TYPE(msg_uid);
>> +	msg_hdr->action_id = TMEL_MSG_UID_ACTION_ID(msg_uid);
>> +
>> +	pr_debug("uid: %d, msg_size: %zu msg_type:%d, action_id:%d\n",
>> +		 msg_uid, msg_size, msg_hdr->msg_type, msg_hdr->action_id);
> 
> dev_dbg, stop using pr_ everywhere
> 
ok.

>> +
>> +	if (sizeof(struct ipc_header) + msg_size <= MBOX_IPC_PACKET_SIZE) {
>> +		/* Mbox only */
>> +		msg_hdr->ipc_type = IPC_MBOX_ONLY;
>> +		msg_hdr->msg_len = msg_size;
>> +		memcpy((void *)mbox_payload, msg_buf, msg_size);
>> +	} else if (msg_size <= SRAM_IPC_MAX_BUF_SIZE) {
>> +		/* SRAM */
>> +		msg_hdr->ipc_type = IPC_MBOX_SRAM;
>> +		msg_hdr->msg_len = 8;
>> +
>> +		tdev->sram_dma_addr = dma_map_single(tdev->dev, msg_buf,
>> +						     msg_size,
>> +						     DMA_BIDIRECTIONAL);
>> +		ret = dma_mapping_error(tdev->dev, tdev->sram_dma_addr);
>> +		if (ret != 0) {
>> +			pr_err("SRAM DMA mapping error: %d\n", ret);
>> +			return ret;
>> +		}
>> +
>> +		sram_payload->payload_ptr = tdev->sram_dma_addr;
>> +		sram_payload->payload_len = msg_size;
>> +	} else {
>> +		pr_err("Invalid payload length: %zu\n", msg_size);
> 
> No, dev_err
> 
ok.

>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void tmel_unprepare_message(struct tmel *tdev,
>> +				   void *msg_buf, size_t msg_size)
>> +{
>> +	struct tmel_ipc_pkt *ipc_pkt = (struct tmel_ipc_pkt *)tdev->pkt.iov_base;
>> +	struct mbox_payload *mbox_payload = &ipc_pkt->payload.mbox_payload;
>> +
>> +	if (ipc_pkt->msg_hdr.ipc_type == IPC_MBOX_ONLY) {
>> +		memcpy(msg_buf, (void *)mbox_payload, msg_size);
>> +	} else if (ipc_pkt->msg_hdr.ipc_type == IPC_MBOX_SRAM) {
>> +		dma_unmap_single(tdev->dev, tdev->sram_dma_addr, msg_size,
>> +				 DMA_BIDIRECTIONAL);
>> +		tdev->sram_dma_addr = 0;
>> +	}
>> +}
>> +
>> +static bool tmel_rx_done(struct tmel *tdev)
>> +{
>> +	return tdev->rx_done;
>> +}
>> +
>> +static int tmel_process_request(u32 msg_uid, void *msg_buf,
>> +				size_t msg_size)
>> +{
>> +	struct tmel *tdev = tmeldev;
>> +	unsigned long jiffies;
>> +	struct tmel_ipc_pkt *resp_ipc_pkt;
>> +	long time_left = 0;
>> +	int ret = 0;
>> +
>> +	/*
>> +	 * Check to handle if probe is not successful or not completed yet
>> +	 */
> 
> No, it is impossible condition. This code cannot be called before probe.
> 
> Clean up your driver from such spaghetti prevention code and unspaghetti
> it, so you will understand the code flow.
> 
ok sure. wrong condition/comment because of initial debug. will remove.

> 
>> +	if (!tdev) {
>> +		pr_err("tmel dev is NULL\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!msg_buf || !msg_size) {
>> +		pr_err("Invalid msg_buf or msg_size\n");
> 
> No, use dev_err. This applies everywhere.
> 
ok.

>> +		return -EINVAL;
>> +	}
>> +
>> +	mutex_lock(&tdev->lock);
>> +	tdev->rx_done = false;
>> +
>> +	ret = tmel_prepare_msg(tdev, msg_uid, msg_buf, msg_size);
>> +	if (ret)
>> +		return ret;
>> +
>> +	tdev->pkt.iov_len = sizeof(struct tmel_ipc_pkt);
>> +	tdev->pkt.iov_base = (void *)tdev->ipc_pkt;
>> +
>> +	qmp_send_data(tdev->mdev, &tdev->pkt);
>> +	jiffies = msecs_to_jiffies(30000);
>> +
>> +	time_left = wait_event_interruptible_timeout(tdev->waitq,
>> +						     tmel_rx_done(tdev),
>> +						     jiffies);
>> +
>> +	if (!time_left) {
>> +		pr_err("Request timed out\n");
>> +		ret = -ETIMEDOUT;
>> +		goto err_exit;
>> +	}
>> +
>> +	if (tdev->pkt.iov_len != sizeof(struct tmel_ipc_pkt)) {
>> +		pr_err("Invalid pkt.size received size: %lu, expected: %zu\n",
>> +		       tdev->pkt.iov_len, sizeof(struct tmel_ipc_pkt));
>> +		ret = -EPROTO;
>> +		goto err_exit;
>> +	}
>> +
>> +	resp_ipc_pkt = (struct tmel_ipc_pkt *)tdev->pkt.iov_base;
>> +	tmel_unprepare_message(tdev, msg_buf, msg_size);
>> +	tdev->rx_done = false;
>> +	ret = resp_ipc_pkt->msg_hdr.response;
>> +
>> +err_exit:
>> +	mutex_unlock(&tdev->lock);
>> +	return ret;
>> +}
>> +
>> +static int tmel_secboot_sec_auth(u32 sw_id, void *metadata, size_t size)
>> +{
>> +	struct device *dev = tmel_get_device();
>> +	struct tmel_secboot_sec_auth *msg;
>> +	dma_addr_t elf_buf_phys;
>> +	void *elf_buf;
>> +	int ret;
>> +
>> +	if (!dev || !metadata)
>> +		return -EINVAL;
>> +
>> +	msg = kzalloc(sizeof(*msg), GFP_KERNEL);
>> +
>> +	elf_buf = dma_alloc_coherent(dev, size, &elf_buf_phys, GFP_KERNEL);
>> +	if (!elf_buf)
>> +		return -ENOMEM;
>> +
>> +	memcpy(elf_buf, metadata, size);
>> +
>> +	msg->req.sw_id = sw_id;
>> +	msg->req.elf_buf.buf = (u32)elf_buf_phys;
>> +	msg->req.elf_buf.buf_len = (u32)size;
>> +
>> +	ret = tmel_process_request(TMEL_MSG_UID_SECBOOT_SEC_AUTH, msg,
>> +				   sizeof(struct tmel_secboot_sec_auth));
>> +	if (ret) {
>> +		pr_err("Failed to send IPC: %d\n", ret);
>> +	} else if (msg->resp.status || msg->resp.extended_error) {
>> +		pr_err("Failed with status: %d error: %d\n",
>> +		       msg->resp.status, msg->resp.extended_error);
>> +		ret = msg->resp.status;
>> +	}
>> +
>> +	kfree(msg);
>> +	dma_free_coherent(dev, size, elf_buf, elf_buf_phys);
>> +
>> +	return ret;
>> +}
>> +
>> +static int tmel_secboot_teardown(u32 sw_id, u32 secondary_sw_id)
>> +{
>> +	struct device *dev = tmel_get_device();
>> +	struct tmel_secboot_teardown msg = {0};
>> +	int ret;
>> +
>> +	if (!dev)
>> +		return -EINVAL;
>> +
>> +	msg.req.sw_id = sw_id;
>> +	msg.req.secondary_sw_id = secondary_sw_id;
>> +	msg.resp.status = TMEL_ERROR_GENERIC;
>> +
>> +	ret = tmel_process_request(TMEL_MSG_UID_SECBOOT_SS_TEAR_DOWN, &msg,
>> +				   sizeof(msg));
>> +	if (ret) {
>> +		pr_err("Failed to send IPC: %d\n", ret);
>> +	} else if (msg.resp.status) {
>> +		pr_err("Failed with status: %d\n", msg.resp.status);
>> +		ret = msg.resp.status;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int tmel_init(struct qmp_device *mdev)
>> +{
>> +	struct tmel *tdev;
>> +
>> +	tdev = devm_kzalloc(mdev->dev, sizeof(*tdev), GFP_KERNEL);
>> +	if (!tdev)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&tdev->lock);
>> +
>> +	tdev->ipc_pkt = devm_kzalloc(mdev->dev, sizeof(struct tmel_ipc_pkt),
>> +				     GFP_KERNEL);
>> +	if (!tdev->ipc_pkt)
>> +		return -ENOMEM;
>> +
>> +	init_waitqueue_head(&tdev->waitq);
>> +
>> +	tdev->rx_done = false;
>> +	tdev->dev = mdev->dev;
>> +
>> +	tmeldev = tdev;
>> +	tmeldev->mdev = mdev;
>> +
>> +	return 0;
>> +}
>> +
>> +static int tmel_qmp_send(struct mbox_chan *chan, void *data)
>> +{
>> +	struct qmp_device *mdev = chan->con_priv;
>> +
>> +	mdev->qwork.data =  data;
>> +
>> +	queue_work(system_wq, &mdev->qwork.work);
>> +
>> +	return 0;
>> +}
>> +
>> +static void tmel_qmp_send_work(struct work_struct *work)
>> +{
>> +	struct qmp_work *qwork = container_of(work, struct qmp_work, work);
>> +	struct qmp_device *mdev = tmeldev->mdev;
>> +	struct mbox_chan *chan = &mdev->ctrl.chans[0];
>> +
>> +	struct tmel_qmp_msg *tmsg = qwork->data;
>> +	struct tmel_sec_auth *smsg = tmsg->msg;
>> +	int ret;
>> +
>> +	switch (tmsg->msg_id) {
>> +	case TMEL_MSG_UID_SECBOOT_SEC_AUTH:
>> +		ret = tmel_secboot_sec_auth(smsg->pas_id,
>> +					    smsg->data,
>> +					    smsg->size);
>> +		break;
>> +	case TMEL_MSG_UID_SECBOOT_SS_TEAR_DOWN:
>> +		ret = tmel_secboot_teardown(smsg->pas_id, 0);
>> +		break;
>> +	}
>> +
>> +	mbox_chan_txdone(chan, 0);
>> +}
>> +
>> +/**
>> + * tmel_qmp_mbox_of_xlate() - Returns a mailbox channel to be used for this mailbox
>> + *		      device. Make sure the channel is not already in use.
>> + * @mbox:       Mailbox device controlls the requested channel.
>> + * @spec:       Device tree arguments to specify which channel is requested.
>> + */
>> +static struct mbox_chan *tmel_qmp_mbox_of_xlate(struct mbox_controller *mbox,
>> +						const struct of_phandle_args *spec)
>> +{
>> +	struct qmp_device *mdev = dev_get_drvdata(mbox->dev);
>> +	unsigned int channel = spec->args[0];
>> +
>> +	if (!mdev)
>> +		return ERR_PTR(-EPROBE_DEFER);
>> +
>> +	if (channel >= mbox->num_chans)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	mutex_lock(&mdev->state_lock);
>> +	if (mdev->ch_in_use) {
>> +		dev_err(mdev->dev, "mbox channel already in use\n");
>> +		mutex_unlock(&mdev->state_lock);
>> +		return ERR_PTR(-EBUSY);
> 
> 
> Why one cannot call xlate twice for the same argument? This looks wrong.
> Xlate does not mean that mailbox is being used.
> 
yeah, same channel could be used for different services. This condition
needs to be removed. Will fix it.

> 
>> +	}
>> +	mdev->ch_in_use = true;
>> +	mutex_unlock(&mdev->state_lock);
>> +
>> +	return &mbox->chans[0];
>> +}
>> +
>> +static struct mbox_chan_ops tmel_qmp_ops = {
>> +	.startup = tmel_qmp_startup,
>> +	.shutdown = tmel_qmp_shutdown,
>> +	.send_data = tmel_qmp_send,
>> +};
>> +
>> +static int tmel_qmp_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *node = pdev->dev.of_node;
>> +	struct mbox_chan *chans;
>> +	struct qmp_device *mdev;
>> +	int ret = 0;
>> +
>> +	mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL);
>> +	if (!mdev)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, mdev);
>> +
>> +	ret = tmel_qmp_parse_devicetree(pdev, mdev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mdev->dev = &pdev->dev;
>> +
>> +	chans = devm_kzalloc(mdev->dev,
> 
> 
> devm_kcalloc
> 
ok.

>> +			     sizeof(*chans) * QMP_NUM_CHANS, GFP_KERNEL);
>> +	if (!chans)
>> +		return -ENOMEM;
>> +
>> +	INIT_WORK(&mdev->qwork.work, tmel_qmp_send_work);
>> +
>> +	mdev->ctrl.dev = &pdev->dev;
>> +	mdev->ctrl.ops = &tmel_qmp_ops;
>> +	mdev->ctrl.chans = chans;
>> +	chans[0].con_priv = mdev;
>> +	mdev->ctrl.num_chans = QMP_NUM_CHANS;
>> +	mdev->ctrl.txdone_irq = true;
>> +	mdev->ctrl.of_xlate = tmel_qmp_mbox_of_xlate;
>> +
>> +	ret = mbox_controller_register(&mdev->ctrl);
>> +	if (ret) {
>> +		dev_err(mdev->dev, "failed to register mbox controller\n");
>> +		return ret;
>> +	}
>> +
>> +	spin_lock_init(&mdev->tx_lock);
>> +	mutex_init(&mdev->state_lock);
>> +	mdev->local_state = LINK_DISCONNECTED;
>> +	init_completion(&mdev->link_complete);
>> +	init_completion(&mdev->ch_complete);
>> +
>> +	INIT_DELAYED_WORK(&mdev->dwork, qmp_notify_timeout);
>> +
>> +	ret = platform_get_irq(pdev, 0);
>> +
>> +	ret = devm_request_threaded_irq(mdev->dev, ret,
>> +					NULL, qmp_irq_handler,
>> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> +					node->name, (void *)mdev);
>> +	if (ret < 0) {
>> +		dev_err(mdev->dev, "request threaded irq failed, ret %d\n",
>> +			ret);
> 
> dev_err_probe
> 
ok.

>> +
> 
> 
> 
> 
>> +
>> +static const struct of_device_id tmel_qmp_dt_match[] = {
>> +	{ .compatible = "qcom,ipq5424-tmel-qmp" },
>> +	{},
>> +};
>> +
>> +static struct platform_driver tmel_qmp_driver = {
>> +	.driver = {
>> +		.name = "tmel_qmp_mbox",
>> +		.of_match_table = tmel_qmp_dt_match,
>> +	},
>> +	.probe = tmel_qmp_probe,
>> +	.remove = tmel_qmp_remove,
>> +};
>> +module_platform_driver(tmel_qmp_driver);
>> +
>> +MODULE_DESCRIPTION("QCOM TMEL QMP DRIVER");
> 
> "driver", this is not an acronym.
> 
ok.

Regards,
  Sricharan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ