Re: [PATCH v2 11/22] firmware: ti_sci: Use rx_message as message receive buffer

[Andrew Davis]

On 9/30/26 11:05 AM, Beleswar Padhi wrote:
> From: Andrew Davis <afd@xxxxxx>
>
> Previously we allocated a message buffer big enough for the largest
> message we could receive for every message we could have concurrently
> in flight. Now have the rx_message buffer during the whole xfer process

s/Now have/Now that we have

> we can use that as our message receive buffer. This removes an extra copy
> and the amount of memory we need to pre-allocate.
>
> Note: rx_buf now points to the caller's on-stack response buffer. If
> the system firmware replies after ti_sci_do_xfer() has timed out, the
> rx callback can race with the caller returning and write into a stale
> stack frame. A reply that arrives after the timeout means the system

To add, the RX callback checks that the message is still expected
and returns safely if not. After a timeout the ti_sci_do_xfer()
function marks the message as no longer expected, before the stack
frame goes out of scope. The only way this can be an issue is if
the RX callback happens exactly after the timeout and is able
to get past the check before we set it, and then also not finish
the memcpy until after we return from setting that expected check.

We could probably still solve that with some additional locking, but
in practice this exact sequence is never going to happen. And the only
time a message timeouts is when the TI-SCI firmware has crashed, so
your system is already borked.

Andrew

> firmware is not responding within its specified bounds, after which the
> system cannot be expected to operate correctly anyway, so this is not
> handled.
>
> Signed-off-by: Andrew Davis <afd@xxxxxx>
> Co-developed-by: Beleswar Padhi <b-padhi@xxxxxx>
> Signed-off-by: Beleswar Padhi <b-padhi@xxxxxx>
> ---
> v2: Changelog:
> 1. None to this patch.
>
> Link to v1:
> https://lore.kernel.org/all/20260929201746.4078803-12-b-padhi@xxxxxx/
>
>   drivers/firmware/ti_sci.c | 21 +++++----------------
>   1 file changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> index 6ed67160c6a7f..fef7028d40e39 100644
> --- a/drivers/firmware/ti_sci.c
> +++ b/drivers/firmware/ti_sci.c
> @@ -45,14 +45,12 @@ static DEFINE_MUTEX(ti_sci_list_mutex);
>    * @tx_message:	Transmit message
>    * @rx_buf:	Pointer to store received message
>    * @rx_len:	Receive message length
> - * @xfer_buf:	Preallocated buffer to store receive message
>    * @done:	completion event
>    */
>   struct ti_sci_xfer {
>   	struct ti_msgmgr_message tx_message;
>   	void *rx_buf;
>   	u8 rx_len;
> -	u8 *xfer_buf;
>   	struct completion done;
>   };
>   
> @@ -287,7 +285,7 @@ static void ti_sci_rx_callback(struct mbox_client *cl, void *m)
>   
>   	ti_sci_dump_header_dbg(dev, hdr);
>   	/* Take a copy to the rx buffer.. */
> -	memcpy(xfer->xfer_buf, mbox_msg->buf, xfer->rx_len);
> +	memcpy(xfer->rx_buf, mbox_msg->buf, xfer->rx_len);
>   	complete(&xfer->done);
>   }
>   
> @@ -522,12 +520,10 @@ static inline int ti_sci_do_xfer(const struct ti_sci_handle *handle,
>   	 * state, then ensure that the response is an ACK
>   	 */
>   	if (response_expected && ret == 0) {
> -		if (!ti_sci_is_response_ack(xfer->xfer_buf)) {
> +		if (!ti_sci_is_response_ack(xfer->rx_buf)) {
>   			dev_warn(dev, "Message response not acknowledged (caller: %pS)\n",
>   				 caller);
>   			ret = -ENODEV;
> -		} else {
> -			memcpy(xfer->rx_buf, xfer->xfer_buf, xfer->rx_len);
>   		}
>   	}
>   
> @@ -3326,7 +3322,6 @@ static int ti_sci_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	const struct ti_sci_desc *desc;
> -	struct ti_sci_xfer *xfer;
>   	struct ti_sci_info *info = NULL;
>   	struct ti_sci_xfers_info *minfo;
>   	struct mbox_client *cl;
> @@ -3380,15 +3375,9 @@ static int ti_sci_probe(struct platform_device *pdev)
>   	if (!minfo->xfer_alloc_table)
>   		return -ENOMEM;
>   
> -	/* Pre-initialize the buffer pointer to pre-allocated buffers */
> -	for (i = 0, xfer = minfo->xfer_block; i < desc->max_msgs; i++, xfer++) {
> -		xfer->xfer_buf = devm_kzalloc(dev, desc->max_msg_size,
> -					      GFP_KERNEL);
> -		if (!xfer->xfer_buf)
> -			return -ENOMEM;
> -
> -		init_completion(&xfer->done);
> -	}
> +	/* Initialize the xfer completions */
> +	for (i = 0; i < desc->max_msgs; i++)
> +		init_completion(&minfo->xfer_block[i].done);
>   
>   	ret = ti_sci_debugfs_create(pdev, info);
>   	if (ret)