Re: [PATCH] Revert "mailbox/pcc: support mailbox management of the shared buffer"

[Adam Young <admiyo@amperemail.onmicrosoft.com>]

Can we get this and the corresponding follow on changes by Sudeep merged?

On 10/5/25 17:29, Jassi Brar wrote:
> On Thu, Oct 2, 2025 at 6:17 PM Adam Young
> <admiyo@amperemail.onmicrosoft.com> wrote:
>>
>> On 10/1/25 16:32, Jassi Brar wrote:
>>> On Wed, Oct 1, 2025 at 12:25 AM Adam Young
>>> <admiyo@amperemail.onmicrosoft.com> wrote:
>>>> On 9/29/25 20:19, Jassi Brar wrote:
>>>>> On Mon, Sep 29, 2025 at 12:11 PM Adam Young
>>>>> <admiyo@amperemail.onmicrosoft.com> wrote:
>>>>>> I posted a patch that addresses a few of these issues.  Here is a top
>>>>>> level description of the isse
>>>>>>
>>>>>>
>>>>>> The correct way to use the mailbox API would be to allocate a buffer for
>>>>>> the message,write the message to that buffer, and pass it in to
>>>>>> mbox_send_message.  The abstraction is designed to then provide
>>>>>> sequential access to the shared resource in order to send the messages
>>>>>> in order.  The existing PCC Mailbox implementation violated this
>>>>>> abstraction.  It requires each individual driver re-implement all of the
>>>>>> sequential ordering to access the shared buffer.
>>>>>>
>>>>>> Why? Because they are all type 2 drivers, and the shared buffer is
>>>>>> 64bits in length:  32bits for signature, 16 bits for command, 16 bits
>>>>>> for status.  It would be execessive to kmalloc a buffer of this size.
>>>>>>
>>>>>> This shows the shortcoming of the mailbox API.  The mailbox API assumes
>>>>>> that there is a large enough buffer passed in to only provide a void *
>>>>>> pointer to the message.  Since the value is small enough to fit into a
>>>>>> single register, it the mailbox abstraction could provide an
>>>>>> implementation that stored a union of a void * and word.
>>>>>>
>>>>> Mailbox api does not make assumptions about the format of message
>>>>> hence it simply asks for void*.
>>>>> Probably I don't understand your requirement, but why can't you pass the pointer
>>>>> to the 'word' you want to use otherwise?
>>>>>
>>>> The mbox_send_message call will then take the pointer value that you
>>>> give it and put it in a ring buffer.  The function then returns, and the
>>>> value may be popped off the stack before the message is actually sent.
>>>> In practice we don't see this because much of the code that calls it is
>>>> blocking code, so the value stays on the stack until it is read.  Or, in
>>>> the case of the PCC mailbox, the value is never read or used.  But, as
>>>> the API is designed, the memory passed into to the function should
>>>> expect to live longer than the function call, and should not be
>>>> allocated on the stack.
>>>>
>>> Mailbox api doesn't dictate the message format, so it simply accepts the message
>>> pointer from the client and passes that to the controller driver. The
>>> message, pointed
>>> to by the submitted pointer, should be available to the controller
>>> driver until transmitted.
>>> So yes, the message should be allocated either not on stack or, if on stack, not
>>> popped until tx_done. You see it as a "shortcoming" because your
>>> message is simply
>>> a word that you want to submit and be done with.
>> Yes.  There seems to be little value in doing a kmalloc for a single
>> word, but without that, the pointer needs to point to memory that lives
>> until the mailbox API is done with it, and that forces it to be a
>> blocking call.
>>
>> This is a  real shortcoming, as it means the that the driver must
>> completely deal with one message before the next one comes in, forcing
>> it to implement its own locking, and reducing the benefit of  the
>> Mailbox API.  the CPPC code in particular suffers from the  need to keep
>> track if reads and writes are  interleaved: this is where an API like
>> Mailbox should provide a big benefit.
>>
>> If the mailbox API could  deal with single words of data (whatever fits
>> in a register) you could instead store that value in the ring buffer,
>> and then the mailbox API could be fire-and-forget for small messages.
>>
>> I was able to get a prototype working that casts the  uint64 to void *
>> before calling mbox_send_message, and casts the  void * mssg to uint64
>> inside a modified  send_data function.  This is kinda gross, but it does
>> work. Nothing checks if these are valid pointers.
>>
> Even if you pass a pointer to data, what validates that it points to
> the correct message?
>
> API doesn't care what you submit to the controller driver - it may be a pointer
> to data or data itself.  Some drivers (ex MHU) do that, and that is
> how you could do it.
>
> -jassi