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

["Padhi, Beleswar" <b-padhi@ti.com>]

On 10/1/2026 12:22 AM, Andrew Davis wrote:
> On 9/30/26 11:05 AM, Beleswar Padhi wrote:
>> From: Andrew Davis <afd@ti.com>
>>
>> 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.


Yup :D,  LLMs are good in hypothesising a scenario where the patch
could fail, but bottom line being this: "And the only  time a message
timeouts is when the TI-SCI firmware has crashed, so  your system is
already borked." , we're good. I had added this explanation in the
commit message to satisfy sashiko comments.

Thanks,
Beleswar

>
> 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@ti.com>
>> Co-developed-by: Beleswar Padhi <b-padhi@ti.com>
>> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
>> ---
>> v2: Changelog:
>> 1. None to this patch.
>>
>> Link to v1:
>> https://lore.kernel.org/all/20260929201746.4078803-12-b-padhi@ti.com/
>>
>>   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)
>