Allows the mailbox client to specify how to allocate the memory
that the mailbox controller uses to send the message to the client.
In the case of a network driver, the message should be allocated as
a struct sk_buff allocated and managed by the network subsystem. The
two parameters passed back from the callback represent the sk_buff
itself and the data section inside the skbuff where the message gets
written.
For simpler cases where the client kmallocs a buffer or returns
static memory, both pointers should point to the same value.
Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
---
include/linux/mailbox_client.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index c6eea9afb943..901184d0515e 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -21,6 +21,12 @@ struct mbox_chan;
* @knows_txdone: If the client could run the TX state machine. Usually
* if the client receives some ACK packet for transmission.
* Unused if the controller already has TX_Done/RTR IRQ.
+ * @rx_alloc Optional callback that allows the driver
+ * to allocate the memory used for receiving
+ * messages. The handle parameter is the value to return
+ * to the client,buffer is the location the mailbox should
+ * write to, and size it the size of the buffer to allocate.
+ * inside the buffer where the mailbox should write the data.
* @rx_callback: Atomic callback to provide client the data received
* @tx_prepare: Atomic callback to ask client to prepare the payload
* before initiating the transmission if required.
@@ -32,6 +38,7 @@ struct mbox_client {
unsigned long tx_tout;
bool knows_txdone;
+ void (*rx_alloc)(struct mbox_client *cl, void **handle, void **buffer, int size);
void (*rx_callback)(struct mbox_client *cl, void *mssg);
void (*tx_prepare)(struct mbox_client *cl, void *mssg);
void (*tx_done)(struct mbox_client *cl, void *mssg, int r);
--
2.43.0Jassi, this one needs your attention specifically.
Do you have an issue with adding this callback? I think it will add an
important ability to the receive path for the mailbox API: letting the
client driver specify how to allocate the memory that the message is
coming in. For general purpose mechanisms like PCC, this is essential:
the mailbox cannot know all of the different formats that the drivers
are going to require. For example, the same system might have MPAM
(Memory Protection) and MCTP (Network Protocol) driven by the same PCC
Mailbox.
On 9/25/25 15:00, Adam Young wrote:
> Allows the mailbox client to specify how to allocate the memory
> that the mailbox controller uses to send the message to the client.
>
> In the case of a network driver, the message should be allocated as
> a struct sk_buff allocated and managed by the network subsystem. The
> two parameters passed back from the callback represent the sk_buff
> itself and the data section inside the skbuff where the message gets
> written.
>
> For simpler cases where the client kmallocs a buffer or returns
> static memory, both pointers should point to the same value.
>
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
> include/linux/mailbox_client.h | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
> index c6eea9afb943..901184d0515e 100644
> --- a/include/linux/mailbox_client.h
> +++ b/include/linux/mailbox_client.h
> @@ -21,6 +21,12 @@ struct mbox_chan;
> * @knows_txdone: If the client could run the TX state machine. Usually
> * if the client receives some ACK packet for transmission.
> * Unused if the controller already has TX_Done/RTR IRQ.
> + * @rx_alloc Optional callback that allows the driver
> + * to allocate the memory used for receiving
> + * messages. The handle parameter is the value to return
> + * to the client,buffer is the location the mailbox should
> + * write to, and size it the size of the buffer to allocate.
> + * inside the buffer where the mailbox should write the data.
> * @rx_callback: Atomic callback to provide client the data received
> * @tx_prepare: Atomic callback to ask client to prepare the payload
> * before initiating the transmission if required.
> @@ -32,6 +38,7 @@ struct mbox_client {
> unsigned long tx_tout;
> bool knows_txdone;
>
> + void (*rx_alloc)(struct mbox_client *cl, void **handle, void **buffer, int size);
> void (*rx_callback)(struct mbox_client *cl, void *mssg);
> void (*tx_prepare)(struct mbox_client *cl, void *mssg);
> void (*tx_done)(struct mbox_client *cl, void *mssg, int r);
On Sun, Oct 5, 2025 at 12:13 AM Adam Young
<admiyo@amperemail.onmicrosoft.com> wrote:
>
> Jassi, this one needs your attention specifically.
>
> Do you have an issue with adding this callback? I think it will add an
> important ability to the receive path for the mailbox API: letting the
> client driver specify how to allocate the memory that the message is
> coming in. For general purpose mechanisms like PCC, this is essential:
> the mailbox cannot know all of the different formats that the drivers
> are going to require. For example, the same system might have MPAM
> (Memory Protection) and MCTP (Network Protocol) driven by the same PCC
> Mailbox.
>
Looking at the existing code, I am not even sure if rx_alloc() is needed at all.
Let me explain...
1) write_response, via rx_alloc, is basically asking the client to
allocate a buffer of length parsed from the pcc header in shmem.
2) write_response is called from isr and even before the
mbox_chan_received_data() call.
Why can't you get rid of write_response() and simply call
mbox_chan_received_data(chan, pchan->chan.shmem)
for the client to allocate and memcpy_fromio itself?
Ideally, the client should have the buffer pre-allocated and only have
to copy the data into it, but even if not it will still not be worse
than what you currently have.
-jassi
On 10/5/25 19:34, Jassi Brar wrote: > On Sun, Oct 5, 2025 at 12:13 AM Adam Young > <admiyo@amperemail.onmicrosoft.com> wrote: >> Jassi, this one needs your attention specifically. >> >> Do you have an issue with adding this callback? I think it will add an >> important ability to the receive path for the mailbox API: letting the >> client driver specify how to allocate the memory that the message is >> coming in. For general purpose mechanisms like PCC, this is essential: >> the mailbox cannot know all of the different formats that the drivers >> are going to require. For example, the same system might have MPAM >> (Memory Protection) and MCTP (Network Protocol) driven by the same PCC >> Mailbox. >> > Looking at the existing code, I am not even sure if rx_alloc() is needed at all. > > Let me explain... > 1) write_response, via rx_alloc, is basically asking the client to > allocate a buffer of length parsed from the pcc header in shmem. Yes, that is exactly what it is doing. Write response encapsulates the PCC specific logic for extracting the message length from the shared buffer. Anything using an extended memory (type 3 or 4) PCC channel is going to have to do this logic. > 2) write_response is called from isr and even before the > mbox_chan_received_data() call. Yes. Specifically, it is marshalling the data from the shared buffer into kernel space. This is logic that every single PCC driver needs to do. It should be put in common code. > > Why can't you get rid of write_response() and simply call > mbox_chan_received_data(chan, pchan->chan.shmem) > for the client to allocate and memcpy_fromio itself? Moving write_response into the client and out of the mailbox means that it has to be implemented properly on every driver, leading to cut-and-paste errors. So, yes, I can do that, but then every single driver that needs to use the pcc mailbox has to do the exact same code. This is the first Type 3/4 PCC driver to use extended memory, and thus it needs to implement new logic. That logic is to make sure we have proper serialized access to the shared buffer. It is this kind of access that the mailbox API is well designed to provide: if both sides follow the protocol, it will only deliver a single message at a time. If we move the logic out of the mailbox, we end up duplicating the serialization code in the client driver. I could make a helper function for it, but we are getting to the point, then, where the mailbox API is not very useful. If we are going to have an abstraction like this (and I think we should) then we should use it. We have more drivers like this coming. There is code that is going to be non-PCC, but really PCC like that will need an MCTP driver. That driver will also need to allocate an sk_buff for receiving the data. There is also MPAM code that will use the PCC driver and a type3 (extended memory) channel. The mailbox API, in order to be more generally useful, should allow for swapping the memory allocation scheme between different clients of the same mailbox. Then the mailbox layer is responsible for handling the mailboxes, and the clients are domain-specific code. > Ideally, the client should have the buffer pre-allocated and only have > to copy the data into it, but even if not it will still not be worse > than what you currently have. > > -jassi
On Mon, Oct 06, 2025 at 11:24:23AM -0400, Adam Young wrote: > > On 10/5/25 19:34, Jassi Brar wrote: > > On Sun, Oct 5, 2025 at 12:13 AM Adam Young > > <admiyo@amperemail.onmicrosoft.com> wrote: > > > Jassi, this one needs your attention specifically. > > > > > > Do you have an issue with adding this callback? I think it will add an > > > important ability to the receive path for the mailbox API: letting the > > > client driver specify how to allocate the memory that the message is > > > coming in. For general purpose mechanisms like PCC, this is essential: > > > the mailbox cannot know all of the different formats that the drivers > > > are going to require. For example, the same system might have MPAM > > > (Memory Protection) and MCTP (Network Protocol) driven by the same PCC > > > Mailbox. > > > > > Looking at the existing code, I am not even sure if rx_alloc() is needed at all. > > > > Let me explain... > > 1) write_response, via rx_alloc, is basically asking the client to > > allocate a buffer of length parsed from the pcc header in shmem. > Yes, that is exactly what it is doing. Write response encapsulates the PCC > specific logic for extracting the message length from the shared buffer. > Anything using an extended memory (type 3 or 4) PCC channel is going to have > to do this logic. > > 2) write_response is called from isr and even before the > > mbox_chan_received_data() call. > Yes. Specifically, it is marshalling the data from the shared buffer into > kernel space. This is logic that every single PCC driver needs to do. It > should be put in common code. > > > > Why can't you get rid of write_response() and simply call > > mbox_chan_received_data(chan, pchan->chan.shmem) > > for the client to allocate and memcpy_fromio itself? > > Moving write_response into the client and out of the mailbox means that it > has to be implemented properly on every driver, leading to cut-and-paste > errors. > Agreed, but I don’t think we need to add a new API to the mailbox framework solely to handle duplication across PCC client drivers. As I’ve mentioned a few times already, we can introduce common helpers later if a second driver ends up replicating this logic. That alone isn’t a good reason to add generic APIs to the mailbox framework right now. We can revisit this idea in the future if there’s a clear benefit, but in my opinion, a multi-step approach is more appropriate: 1. First, add the initial driver with all the required client handling. 2. Then, if a second driver needs similar functionality, we can introduce shared helpers. 3. Finally, if it still makes sense at that point, we can discuss defining a new mailbox API - with the benefit of having concrete examples already in the upstream tree. > So, yes, I can do that, but then every single driver that needs to use the > pcc mailbox has to do the exact same code. This is the first Type 3/4 PCC > driver to use extended memory, and thus it needs to implement new logic. > That logic is to make sure we have proper serialized access to the shared > buffer. It is this kind of access that the mailbox API is well designed to > provide: if both sides follow the protocol, it will only deliver a single > message at a time. If we move the logic out of the mailbox, we end up > duplicating the serialization code in the client driver. I could make a > helper function for it, but we are getting to the point, then, where the > mailbox API is not very useful. If we are going to have an abstraction > like this (and I think we should) then we should use it. > Sorry but you haven't demonstrated this with example. First try the above mentioned step and lets talk later if you still have issues. > We have more drivers like this coming. There is code that is going to be > non-PCC, but really PCC like that will need an MCTP driver. That driver > will also need to allocate an sk_buff for receiving the data. There is > also MPAM code that will use the PCC driver and a type3 (extended memory) > channel. The mailbox API, in order to be more generally useful, should > allow for swapping the memory allocation scheme between different clients of > the same mailbox. Then the mailbox layer is responsible for handling the > mailboxes, and the clients are domain-specific code. > You can always improve the code later. We don't know what will the discussions lead to when you submit that driver later. We can keep those discussion for later and just concentrate on getting the first step done here. Hi Jassi, If you don't have any objections, can we first get the revert in place and let Adam attempt adding code to the client and take the discussions from there. -- Regards, Sudeep
On Sun, Oct 05, 2025 at 06:34:51PM -0500, Jassi Brar wrote: > On Sun, Oct 5, 2025 at 12:13 AM Adam Young > <admiyo@amperemail.onmicrosoft.com> wrote: > > > > Jassi, this one needs your attention specifically. > > > > Do you have an issue with adding this callback? I think it will add an > > important ability to the receive path for the mailbox API: letting the > > client driver specify how to allocate the memory that the message is > > coming in. For general purpose mechanisms like PCC, this is essential: > > the mailbox cannot know all of the different formats that the drivers > > are going to require. For example, the same system might have MPAM > > (Memory Protection) and MCTP (Network Protocol) driven by the same PCC > > Mailbox. > > > Looking at the existing code, I am not even sure if rx_alloc() is needed at all. > > Let me explain... > 1) write_response, via rx_alloc, is basically asking the client to > allocate a buffer of length parsed from the pcc header in shmem. > 2) write_response is called from isr and even before the > mbox_chan_received_data() call. > > Why can't you get rid of write_response() and simply call > mbox_chan_received_data(chan, pchan->chan.shmem) > for the client to allocate and memcpy_fromio itself? > Ideally, the client should have the buffer pre-allocated and only have > to copy the data into it, but even if not it will still not be worse > than what you currently have. > Exactly, this is what I have been telling. Adam, Please share the code that you have attempted with this approach and the problems you have faced. -- Regards, Sudeep
Hi, On Thu, Sep 25, 2025 at 03:00:24PM -0400, Adam Young wrote: > Allows the mailbox client to specify how to allocate the memory > that the mailbox controller uses to send the message to the client. > > In the case of a network driver, the message should be allocated as > a struct sk_buff allocated and managed by the network subsystem. The > two parameters passed back from the callback represent the sk_buff > itself and the data section inside the skbuff where the message gets > written. > > For simpler cases where the client kmallocs a buffer or returns > static memory, both pointers should point to the same value. > I have posted a revert[1] of a patch that you got merged recently and this change builds on top of that. Please start fresh with that patch/revert applied and explain why you can't use tx_prepare and rx_callback as suggested and then we can take it from there. It makes no sense to add these optional callbacks the way it is done at the least. If you are worried about duplication of shmem handling, IIUC, it is done intentionally to give that flexibility to the clients. If you think it can be consolidated, I would rather have them as a standard helper which can be assigned to tx_prepare and rx_callback and convert few users to demonstrate the same. So I definitely don't like this addition and NACK until all those details are looked at and you have convinced it can't be achieved via those and need more changes. On the other hand, the consolidation is a good thought, but it can't be addition without any other drivers except you new yet to be merged driver as the sole user of that. If there are no users needing copying to/from the shared memory like you driver, then I am happy to consider this but it needs more thinking and reviewing for sure. I will also have a look. Sorry for missing the review of the original patch that got merged, my bad. -- Regards, Sudeep [1] https://lore.kernel.org/all/20250926153311.2202648-1-sudeep.holla@arm.com
On Thu, Sep 25, 2025 at 03:00:24PM -0400, Adam Young wrote:
> Allows the mailbox client to specify how to allocate the memory
> that the mailbox controller uses to send the message to the client.
>
> In the case of a network driver, the message should be allocated as
> a struct sk_buff allocated and managed by the network subsystem. The
> two parameters passed back from the callback represent the sk_buff
> itself and the data section inside the skbuff where the message gets
> written.
>
> For simpler cases where the client kmallocs a buffer or returns
> static memory, both pointers should point to the same value.
>
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
> include/linux/mailbox_client.h | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
> index c6eea9afb943..901184d0515e 100644
> --- a/include/linux/mailbox_client.h
> +++ b/include/linux/mailbox_client.h
> @@ -21,6 +21,12 @@ struct mbox_chan;
> * @knows_txdone: If the client could run the TX state machine. Usually
> * if the client receives some ACK packet for transmission.
> * Unused if the controller already has TX_Done/RTR IRQ.
> + * @rx_alloc Optional callback that allows the driver
@rx_alloc:
^^^
Flagged by ./scripts/kernel-doc -none -Wall
> + * to allocate the memory used for receiving
> + * messages. The handle parameter is the value to return
> + * to the client,buffer is the location the mailbox should
> + * write to, and size it the size of the buffer to allocate.
> + * inside the buffer where the mailbox should write the data.
> * @rx_callback: Atomic callback to provide client the data received
> * @tx_prepare: Atomic callback to ask client to prepare the payload
> * before initiating the transmission if required.
> @@ -32,6 +38,7 @@ struct mbox_client {
> unsigned long tx_tout;
> bool knows_txdone;
>
> + void (*rx_alloc)(struct mbox_client *cl, void **handle, void **buffer, int size);
> void (*rx_callback)(struct mbox_client *cl, void *mssg);
> void (*tx_prepare)(struct mbox_client *cl, void *mssg);
> void (*tx_done)(struct mbox_client *cl, void *mssg, int r);
> --
> 2.43.0
>
>
© 2016 - 2026 Red Hat, Inc.