Re: [PATCH] Revert "mailbox/pcc: support mailbox management of the shared buffer"
From: Adam Young
Date: Tue Dec 02 2025 - 14:20:34 EST
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@xxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
Even if you pass a pointer to data, what validates that it points to
On 10/1/25 16:32, Jassi Brar wrote:
On Wed, Oct 1, 2025 at 12:25 AM Adam YoungYes. There seems to be little value in doing a kmalloc for a single
<admiyo@xxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
On 9/29/25 20:19, Jassi Brar wrote:Mailbox api doesn't dictate the message format, so it simply accepts the message
On Mon, Sep 29, 2025 at 12:11 PM Adam YoungThe mbox_send_message call will then take the pointer value that you
<admiyo@xxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
I posted a patch that addresses a few of these issues. Here is a topMailbox api does not make assumptions about the format of message
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.
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?
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.
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.
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.
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