Message-ID: <848fd819-7e57-4340-a78a-37eaf779c6b2@oss.qualcomm.com>
Date: Sat, 26 Apr 2025 11:44:29 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Sricharan R <quic_srichara@...cinc.com>, 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 V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox
 driver

On 3/27/25 7:17 PM, Sricharan R wrote:
> From: Sricharan Ramabadhran <quic_srichara@...cinc.com>
> 
> This mailbox facilitates the communication between the TMEL server
> subsystem (Trust Management Engine Lite) and the TMEL client
> (APPSS/BTSS/AUDIOSS), used for secure services like secure image
> authentication, enable/disable efuses, crypto services etc. Each client in
> the SoC has its own block of message RAM and IRQ for communication with the
> TMEL 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 TMEL to securely authenticate/teardown the images.
> 
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@...cinc.com>
> ---

[...]

> +/*
> + * mbox data can be shared over mem or sram
> + */

/* foo */

[...]

> +enum ipc_type {
> +	IPC_MBOX_MEM,
> +	IPC_MBOX_SRAM,
> +};
> +
> +/*
> + * mbox header indicates the type of payload and action required.
> + */
> +struct ipc_header {
> +	u8 ipc_type:1;
> +	u8 msg_len:7;
> +	u8 msg_type;
> +	u8 action_id;
> +	s8 response;
> +};

You said in the changelog that __packed is not required.. I suppose it's
technically correct, but I think it's good practice to add it on anything
that's bounced between blocks and is of fixed size

[...]

> +/**
> + * tmel_qmp_send_irq() - send an irq to a remote entity as an event signal.
> + * @mdev: Which remote entity that should receive the irq.
> + */
> +static inline void tmel_qmp_send_irq(struct qmp_device *mdev)
> +{
> +	writel(mdev->mcore.val, mdev->mcore_desc);
> +	/* Ensure desc update is visible before IPC */
> +	wmb();

writel (non _relaxed) already includes a write barrier, to ensure write
observability at the other endpoint, you'd have to read back the register
instead

> +
> +	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);
> +}
> +
> +/**
> + * tmel_qmp_send_data() - Send the data to remote and notify.
> + * @mdev: qmp_device to send the data to.
> + * @data: Data to be sent to remote processor, should be in the format of
> + *	  a kvec.
> + *
> + * Copy the data to the channel's mailbox and notify remote subsystem of new
> + * data. This function will return an error if the previous message sent has
> + * not been read.

This is not valid kerneldoc, check make W=1, there are many cases in
this file

[...]

> +	/* read remote_desc from mailbox register */

All other comments start with an uppercase letter, please make it
consistent

[...]

> +	mdev->ucore.val = readl(mdev->ucore_desc);
> +
> +	dev_dbg(mdev->dev, "%s: mcore 0x%x ucore 0x%x", __func__,
> +		mdev->mcore.val, mdev->ucore.val);
> +
> +	spin_lock_irqsave(&mdev->tx_lock, flags);
> +
> +	/* Check if remote link down */
> +	if (mdev->local_state >= LINK_CONNECTED &&
> +	    !(mdev->ucore.bits.link_state)) {
> +		mdev->local_state = LINK_NEGOTIATION;
> +		mdev->mcore.bits.link_state_ack = mdev->ucore.bits.link_state;

You dereference into local_state, mcore and ucore a lot - consider
creating a variable to reduce the constant ->/.-age

[...]

> +	if (sizeof(struct ipc_header) + msg_size <= QMP_MBOX_IPC_PACKET_SIZE) {
> +		/* Mbox only */
> +		msg_hdr->ipc_type = IPC_MBOX_MEM;
> +		msg_hdr->msg_len = msg_size;
> +		memcpy((void *)mbox_payload, msg_buf, msg_size);
> +	} else if (msg_size <= QMP_SRAM_IPC_MAX_BUF_SIZE) {

> +		/* SRAM */
> +		msg_hdr->ipc_type = IPC_MBOX_SRAM;
> +		msg_hdr->msg_len = 8;

I think we should check for == and not <= to rule out some partially
transacted messages

[...]

> +
> +		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) {
> +			dev_err(tdev->dev, "SRAM DMA mapping error: %d\n", ret);
> +			return ret;
> +		}
> +
> +		sram_payload->payload_ptr = tdev->sram_dma_addr;
> +		sram_payload->payload_len = msg_size;
> +	} else {
> +		dev_err(tdev->dev, "Invalid payload length: %zu\n", msg_size);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * tmel_unprepare_message() - Get the response data back for client
> + * @tdev: the tmel device
> + * @msg_buf: payload to be sent
> + * @msg_size: size of the payload
> + */
> +static inline 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_MEM) {
> +		memcpy(msg_buf, 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 inline bool tmel_rx_done(struct tmel *tdev)
> +{
> +	return tdev->rx_done;
> +}
> +
> +/**
> + * tmel_process_request() - process client msg and wait for response
> + * @tdev: the tmel device
> + * @msg_uid: msg_type/action_id combo
> + * @msg_buf: payload to be sent
> + * @msg_size: size of the payload
> + */
> +static inline int tmel_process_request(struct tmel *tdev, u32 msg_uid,
> +				       void *msg_buf, size_t msg_size)
> +{
> +	struct qmp_device *mdev = tdev->mdev;
> +	struct tmel_ipc_pkt *resp_ipc_pkt;
> +	struct mbox_chan *chan;
> +	unsigned long jiffies;
> +	long time_left = 0;
> +	int ret = 0;
> +
> +	chan = &tdev->ctrl.chans[0];
> +
> +	if (!msg_buf || !msg_size) {
> +		dev_err(tdev->dev, "Invalid msg_buf or msg_size\n");
> +		return -EINVAL;
> +	}
> +
> +	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;
> +
> +	tmel_qmp_send_data(mdev, &tdev->pkt);
> +	jiffies = msecs_to_jiffies(QMP_SEND_TIMEOUT);
> +
> +	time_left = wait_event_interruptible_timeout(tdev->waitq,
> +						     tmel_rx_done(tdev),
> +						     jiffies);
> +

Unexpected \n

[...]

> +	if (ret) {
> +		dev_err(dev, "Failed to send IPC: %d\n", ret);
> +	} else if (smsg->msg.resp.status) {
> +		dev_err(dev, "Failed with status: %d", smsg->msg.resp.status);
> +		ret = smsg->msg.resp.status ? -EINVAL : 0;

Do the internal error numbers correspond to linux errnos?

E.g. is there an TMEL_ERROR_TIMEDOUT that could be mapped to
ETIMEDOUT?

[...]

> +	/*
> +	 * Kick start the SM from the negotiation phase

Please spell out state machine, it's not obvious

[...]

> +
> +	ret = platform_get_irq(pdev, 0);

This return value is never checked

Konrad

Powered by blists - more mailing lists