Re: [PATCH V4 2/2] mailbox: tmelite-qmp: Introduce TMEL QMP mailbox driver

[Sricharan Ramabadhran]

On 4/26/2025 3:14 PM, Konrad Dybcio wrote:
> On 3/27/25 7:17 PM, Sricharan R wrote:
> > From: Sricharan Ramabadhran <quic_srichara@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
>
> [...]
>
> > +/*
> > + * mbox data can be shared over mem or sram
> > + */
>
> /* foo */
ok.

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

> [...]
>
> > +/**
> > + * 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
ok

> > +
> > +	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
ok, but i ran kerneldoc check and did not get any error. Anyways let me
re-check with W=1 once.

> [...]
>
> > +	/* read remote_desc from mailbox register */
>
> All other comments start with an uppercase letter, please make it
> consistent
ok.

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

> [...]
>
> > +	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
ok, will fix.

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

> [...]
>
> > +	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?
There is no direct mapping, hence this sideband conversion.

> [...]
>
> > +	/*
> > +	 * Kick start the SM from the negotiation phase
>
> Please spell out state machine, it's not obvious
ok.

> [...]
>
> > +
> > +	ret = platform_get_irq(pdev, 0);
>
> This return value is never checked
ok, will fix.

Regards,
 Sricharan