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

[Adam Young]

On 10/1/25 07:57, Sudeep Holla wrote:
> On Wed, Oct 01, 2025 at 01:25:42AM -0400, Adam Young wrote:
> > On 9/29/25 20:19, Jassi Brar wrote:
> > > On Mon, Sep 29, 2025 at 12:11 PM Adam Young
> > > <admiyo@xxxxxxxxxxxxxxxxxxxxxxxxxx> 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?
> > >
> > > -jassi
> > 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.
> I’m still not clear on what exactly you are looking for. Let’s look at
> mbox_send_message(). It adds the provided data pointer to the queue, and then
> passes the same pointer to tx_prepare() just before calling send_data(). This
> is what I’ve been pointing out that you can obtain the buffer pointer there and
> use it to update the shared memory in the client driver.

So we have two different use cases in the discussions here, which make 
it a little tricky to separate.  Type 2 uses a Reduced Memory region, 
and type 3/4 use  an extended memory region.  Jassi and I were talking 
about the type 2.  I think we should table that discussion for the moment.

To answer your question, Sudeep, I need to deal with the Type3/4 flags 
for ensuring that the buffer is available to write.  In type 2, this is 
done using a value inside the buffer, and is hard coded  by the spec as 
a field in the statis code.  For Type3/4, the logic is this:

       pcc_chan_reg_read(&pchan->cmd_complete, &val);
       if (!val) {
               pr_info("%s pchan->cmd_complete not set", __func__);
               return -1;
       }
       memcpy_toio(pcc_mbox_chan->shmem,  data, len);

pchan->cmd_complete is a register set in the PCCT, and can vary from 
channel to channel.  This needs to be atomically checked with the 
following write.  Since the mailbox API has a lock here, I want to do 
this inside the send_message code.

The alternative, which you might suggest, is to do this logic in the tx 
Prep. That would require a different change, one that exposes the result of

 pcc_chan_reg_read(&pchan->cmd_complete, &val);

the way that the type 2 drivers do.  Putting this into the drivers 
tx_prepare commits us to that path, as any attempt to move the logic 
into the mailbox would break the driver (or require a rewrite).  This is 
part of the PCC protocol, and the Mailbox is PCC protocol specific.  
Hence I put it into the send_data path.

In my latest change, submitted right before the revert got posted, I 
made a change to only execute this code for Type3 and Typ3 4 Drivers.  I 
think that this is the better option than flagging the channel with 
"managed_writes" and I do regret that the change got merged before that 
change was caught.  That change is titled:  mailbox/pcc: use mailbox-api 
level rx_alloc callback