Message-ID: <CABb+yY2-CQj=S6FYaOq=78EuQCnpKFUqFSJV+NHdLBjS-txnAw@mail.gmail.com>
Date: Sun, 5 Oct 2025 16:29:08 -0500
From: Jassi Brar <jassisinghbrar@...il.com>
To: Adam Young <admiyo@...eremail.onmicrosoft.com>
Cc: Sudeep Holla <sudeep.holla@....com>, Adam Young <admiyo@...amperecomputing.com>, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Revert "mailbox/pcc: support mailbox management of the
 shared buffer"

On Thu, Oct 2, 2025 at 6:17 PM Adam Young
<admiyo@...eremail.onmicrosoft.com> wrote:
>
>
> On 10/1/25 16:32, Jassi Brar wrote:
> > On Wed, Oct 1, 2025 at 12:25 AM Adam Young
> > <admiyo@...eremail.onmicrosoft.com> wrote:
> >>
> >> On 9/29/25 20:19, Jassi Brar wrote:
> >>> On Mon, Sep 29, 2025 at 12:11 PM Adam Young
> >>> <admiyo@...eremail.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

Powered by blists - more mailing lists