[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <54aa6e28-65cc-425f-a124-372175a6e1c2@amperemail.onmicrosoft.com>
Date: Thu, 2 Oct 2025 19:00:23 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: Sudeep Holla <sudeep.holla@....com>
Cc: Jassi Brar <jassisinghbrar@...il.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 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@...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?
>>>
>>> -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
Powered by blists - more mailing lists