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

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

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.

One way to make it clear is to modify the Mailbox API is to provide an 
additional function:

int mbox_send_int_message(struct mbox_chan *chan, int mssg);

And modify the ring buffer to store this value. Convert the field in the 
channel from

void *msg_data[MBOX_TX_QUEUE_LEN];

To a union.

union message_data {
int int_data;
void * message;

}

union message_data msg_data[MBOX_TX_QUEUE_LEN];

Or even a structure that includes a type field to know which it stores:

struct message_data {
int message_type; /*int or void **/
union data{
int int_data;
void * message;
}

The message_type field would be set to MBOX_INT 
for mbox_send_int_message and MBOX_VOID for mbox_send_message