[PATCH net-next v29 0/3] MCTP Over PCC Transport

[PATCH net-next v29 0/3] MCTP Over PCC Transport

[Adam Young]

This series adds support for the
Management Component Transport Protocol (MCTP)
over the Platform Communication Channel (PCC) mechanism.

DMTF DSP:0292
https://www.dmtf.org/sites/default/files/standards/documents/\
DSP0292_1.0.0WIP50.pdf

MCTP defines a communication model intended to facilitate communication
between Management controllers and other management controllers, and
between Management controllers and management devices.

PCC is a mechanism for communication between components within the
Platform. It is a composed of shared memory regions, interrupt registers,
and status registers.

The MCTP over PCC driver makes use of two PCC channels. For sending
messages, it uses a Type 3 channel, and for receiving messages it uses
the paired Type 4 channel.  The device and corresponding channels are
specified via ACPI.

MCTP is a general purpose  protocol so it would  be impossible to
enumerate all the use cases, but some of the ones that are most topical
are attestation and RAS support.  There are a handful of protocols built
on top of MCTP, to include PLDM and SPDM, both specified by the DMTF.

https://www.dmtf.org/sites/default/files/standards/documents/DSP0240_1.0.0.pdf
https://www.dmtf.org/sites/default/files/standards/documents/DSP0274_1.3.0.pd

SPDM entails various usages, including device identity collection, device
authentication, measurement collection, and device secure session
establishment.

PLDM is more likely to be used for hardware support: temperature, voltage,
or fan sensor control.

At least two companies have devices that can make use of the mechanism.
One is Ampere Computing, my employer.

The mechanism it uses is called Platform Communication Channels is part of
the ACPI spec:
https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html

Since it is a socket interface, the system administrator also has the
ability to ignore an MCTP link that they do not want to enable. This link
would be visible to the end user, but would not be usable.

If MCTP support is disabled in the Kernel, this driver would also be disabled.

PCC is based on a shared buffer and a set of I/O mapped memory locations
that the Spec calls registers.  This mechanism exists regardless of the
existence of the driver. Thus, if the user has the ability to map these
physical location to virtual locations, they have the ability to drive the
hardware.  Thus, there is a security aspect to this mechanism that extends
beyond the responsibilities of the operating system.

If the hardware does not expose the PCC in the ACPI table, this device
will never be enabled. Thus it is only an issue on hard that does support
PCC. In that case, it is up to the remote controller to sanitize
communication; MCTP will be exposed as a socket interface, and userland
can send any crafted packet it wants. It would thus also be incumbent on
the hardware manufacturer to allow the end user to disable MCTP over PCC
communication if they did not want to expose it.

A Previous version of this patch series had a pre-requisite patch that
allows the PCC Mailbox to managed the PCC shared buffer.  That patch has
been merged. This patch series has a fix for a race condition in that
implementation. This code is only executed for type3 and type 4 buffers:
there are currently no other drivers in the Linux kernel that are type 3
or type 4.

Previous Version:
https://lore.kernel.org/lkml/20250904040544.598469-1-admiyo@os.amperecomputing.com/

Changed in V29:
- Added a callback function for the mailbox API to allocate the rx_buffer
- The PCC mailbox to uses the Mailbox API callback instead of the PCC specific one
- The MCTP-PCC driver uses the Mailbox API callback instead of the PCC specific one
- Code review fixes for language in comments
- Removed PCC specific callback

Changes in V28:
- ndo open and ndo start create and free channels
- Max MTU is set in create
- Reverse XMass tree rules complied with
- Driver no longer has any auto-cleanup on registration functions
- Tested with KASAN

Changes in V27:
- Stop and restart packet Queues to deal with a full ring buffer
- drop the 'i' from the middle of the link name
- restore the allocation and freeing of the channel to the driver add/remove functions
  leaving only the queue draining in the ndo stop function

Changes in V26:
-  Remove the addition net-device spinlock and use the spinlock already present in skb lists
-  Use temporary variables to check for success finding the skb in the lists
-  Remove comment that is no longer relevant

Changes in V25:
- Use spin lock to control access to queues of sk_buffs
- removed unused constants
- added ndo_open and ndo_stop functions.  These two functions do
  channel creation and cleanup, to remove packets from the mailbox.
  They do queue cleanup as well.
- No longer cleans up the channel from the device.

Changes in V24:
- Removed endianess for PCC header values
- Kept Column width to under 80 chars
- Typo in commit message
- Prereqisite patch for PCC buffer management was merged late in 6.17.
  See "mailbox/pcc: support mailbox management of the shared buffer"

Changes in V23:
- Trigger for direct management of shared buffer based on flag in pcc channel
- Only initialize rx_alloc for inbox, not outbox.
- Read value for requested IRQ flag out of channel's current_req
- unqueue an sk_buff that failed to send
- Move error handling for skb resize error inline instead of goto

Changes in V22:
- Direct management of the shared buffer in the mailbox layer.
- Proper checking of command complete flag prior to writing to the buffer.

Changes in V21:
- Use existing constants PCC_SIGNATURE and PCC_CMD_COMPLETION_NOTIFY
- Check return code on call to send_data and drop packet if failed
- use sizeof(*mctp_pcc_header) etc,  instead of structs for resizing buffers
- simplify check for ares->type != PCC_DWORD_TYPE
- simply return result devm_add_action_or_reset
- reduce initializer for  mctp_pcc_lookup_context context = {};
- move initialization of mbox dev into mctp_pcc_initialize_mailbox
- minor spacing changes

Changes in V20:
- corrected typo in RFC version
- removed spurious space
- tx spin lock only controls access to shared memory buffer
- tx spin lock not eheld on error condition
- tx returns OK if skb can't be expanded

Changes in V19:
- Rebased on changes to PCC mailbox handling
- checks for cloned SKB prior to transmission
- converted doulbe slash comments to C comments

Changes in V18:
- Added Acked-By
- Fix minor spacing issue

Changes in V17:
- No new changes. Rebased on net-next post 6.13 release.

Changes in V16:
- do not duplicate cleanup after devm_add_action_or_reset calls

Changes in V15:
- corrected indentation formatting error
- Corrected TABS issue in MAINTAINER entry

Changes in V14:
- Do not attempt to unregister a netdev that is never registered
- Added MAINTAINER entry

Changes in V13:
- Explicitly Convert PCC header from little endian to machine native

Changes in V12:
- Explicitly use little endian conversion for PCC header signature
- Builds clean with make C=1

Changes in V11:
- Explicitly use little endian types for PCC header

Changes in V11:
- Switch Big Endian data types to machine local for PCC header
- use mctp specific function for registering netdev

Changes in V10:
- sync with net-next branch
- use dstats helper functions
- remove duplicate drop stat
- remove more double spaces

Changes in V9:
- Prerequisite patch for PCC mailbox has been merged
- Stats collection now use helper functions
- many double spaces reduced to single

Changes in V8:
- change 0 to NULL for pointer check of shmem
- add semi for static version of pcc_mbox_ioremap
- convert pcc_mbox_ioremap function to static inline when client code is not being built
- remove shmem comment from struct pcc_chan_info descriptor
- copy rx_dropped in mctp_pcc_net_stats
- removed trailing newline on error message
- removed double space in dev_dbg string
- use big endian for header members
- Fix use full spec ID in description
- Fix typo in file description
- Form the complete outbound message in the sk_buff

Changes in V7:
- Removed the Hardware address as specification is not published.
- Map the shared buffer in the mailbox and share the mapped region with the driver
- Use the sk_buff memory to prepare the message before copying to shared region

Changes in V6:
- Removed patch for ACPICA code that has merged
- Includes the hardware address in the network device
- Converted all device resources to devm resources
- Removed mctp_pcc_driver_remove function
- uses acpi_driver_module for initialization
- created helper structure for in and out mailboxes
- Consolidated code for initializing mailboxes in the add_device function
- Added specification references
- Removed duplicate constant PCC_ACK_FLAG_MASK
- Use the MCTP_SIGNATURE_LENGTH define
- made naming of header structs consistent
- use sizeof local variables for offset calculations
- prefix structure name to avoid potential clash
- removed unnecessary null initialization from acpi_device_id

Changes in V5
- Removed Owner field from ACPI module declaration
- removed unused next field from struct mctp_pcc_ndev
- Corrected logic reading  RX ACK flag.
- Added comment for struct pcc_chan_info field shmem_base_addr
- check against current mtu instead of max mtu for packet length\
- removed unnecessary lookups of pnd->mdev.dev

Changes in V4
- Read flags out of shared buffer to trigger ACK for Type 4 RX
- Remove list of netdevs and cleanup from devices only
- tag PCCT protocol headers as little endian
- Remove unused constants

Changes in V3
- removed unused header
- removed spurious space
- removed spurious semis after functiomns
- removed null assignment for init
- remove redundant set of device on skb
- tabify constant declarations
- added  rtnl_link_stats64 function
- set MTU to minimum to start
- clean up logic on driver removal
- remove cast on void * assignment
- call cleanup function directly
- check received length before allocating skb
- introduce symbolic constatn for ACK FLAG MASK
- symbolic constant for PCC header flag.
- Add namespace ID to PCC magic
- replaced readls with copy from io of PCC header
- replaced custom modules init and cleanup with ACPI version

Changes in V2

- All Variable Declarations are in reverse Xmass Tree Format
- All Checkpatch Warnings Are Fixed
- Removed Dead code
- Added packet tx/rx stats
- Removed network physical address.  This is still in
  disucssion in the spec, and will be added once there
  is consensus. The protocol can be used with out it.
  This also lead to the removal of the Big Endian
  conversions.
- Avoided using non volatile pointers in copy to and from io space
- Reorderd the patches to put the ACK check for the PCC Mailbox
  as a pre-requisite.  The corresponding change for the MCTP
  driver has been inlined in the main patch.
- Replaced magic numbers with constants, fixed typos, and other
  minor changes from code review.

Adam Young (3):
  mailbox: add callback function for rx buffer allocation
  mailbox/pcc: use mailbox-api level rx_alloc callback
  mctp pcc: Implement MCTP over PCC Transport

 MAINTAINERS                    |   5 +
 drivers/mailbox/pcc.c          |  16 +-
 drivers/net/mctp/Kconfig       |  13 ++
 drivers/net/mctp/Makefile      |   1 +
 drivers/net/mctp/mctp-pcc.c    | 341 +++++++++++++++++++++++++++++++++
 include/acpi/pcc.h             |  22 ---
 include/linux/mailbox_client.h |   7 +
 7 files changed, 377 insertions(+), 28 deletions(-)
 create mode 100644 drivers/net/mctp/mctp-pcc.c

-- 
2.43.0

[PATCH net-next v29 1/3] mailbox: add callback function for rx buffer allocation

[Adam Young]

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.0

[PATCH net-next v29 2/3] mailbox/pcc: use mailbox-api level rx_alloc callback

[Adam Young]

Uses the newly introduced mailbox_api rx_alloc callback. Since this
callback is registered in the call to request a channel, it prevents a
race condition.  Without it, it is impossible to assign the callback before
activating the mailbox delivery of messages.

This patch also removes the flag pcc_mchan->manage_writes which
is not necessary: only type 3 and type 4 subspaces will have their
buffers managed by the mailbox.  It is not required for the driver
to explicitly specify.  If a future type 3 or type 4 drivers wishes
to manage the buffer directly, they can do so by passing NULL in
to mbox_send_message.

Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
---
 drivers/mailbox/pcc.c | 16 ++++++++++------
 include/acpi/pcc.h    | 22 ----------------------
 2 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 0a00719b2482..4535cd208b9e 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -309,17 +309,20 @@ static void pcc_chan_acknowledge(struct pcc_chan_info *pchan)
 static void *write_response(struct pcc_chan_info *pchan)
 {
 	struct pcc_header pcc_header;
+	struct mbox_client *cl;
+	void *handle;
 	void *buffer;
 	int data_len;
 
+	cl = pchan->chan.mchan->cl;
 	memcpy_fromio(&pcc_header, pchan->chan.shmem,
 		      sizeof(pcc_header));
 	data_len = pcc_header.length - sizeof(u32) + sizeof(struct pcc_header);
 
-	buffer = pchan->chan.rx_alloc(pchan->chan.mchan->cl, data_len);
+	cl->rx_alloc(cl, &handle, &buffer, data_len);
 	if (buffer != NULL)
 		memcpy_fromio(buffer, pchan->chan.shmem, data_len);
-	return buffer;
+	return handle;
 }
 
 /**
@@ -359,7 +362,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	 */
 	pchan->chan_in_use = false;
 
-	if (pchan->chan.rx_alloc)
+	if (pchan->chan.mchan->cl->rx_alloc)
 		handle = write_response(pchan);
 
 	if (chan->active_req) {
@@ -415,8 +418,6 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 	if (!pcc_mchan->shmem)
 		goto err;
 
-	pcc_mchan->manage_writes = false;
-
 	/* This indicates that the channel is ready to accept messages.
 	 * This needs to happen after the channel has registered
 	 * its callback. There is no access point to do that in
@@ -466,7 +467,10 @@ static int pcc_write_to_buffer(struct mbox_chan *chan, void *data)
 	struct pcc_mbox_chan *pcc_mbox_chan = &pchan->chan;
 	struct pcc_header *pcc_header = data;
 
-	if (!pchan->chan.manage_writes)
+	if (data == NULL)
+		return 0;
+	if (pchan->type < ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE ||
+	    pchan->type > ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
 		return 0;
 
 	/* The PCC header length includes the command field
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 9af3b502f839..5506490e628c 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -17,28 +17,6 @@ struct pcc_mbox_chan {
 	u32 latency;
 	u32 max_access_rate;
 	u16 min_turnaround_time;
-
-	/* Set to true to indicate that the mailbox should manage
-	 * writing the dat to the shared buffer. This differs from
-	 * the case where the drivesr are writing to the buffer and
-	 * using send_data only to  ring the doorbell.  If this flag
-	 * is set, then the void * data parameter of send_data must
-	 * point to a kernel-memory buffer formatted in accordance with
-	 * the PCC specification.
-	 *
-	 * The active buffer management will include reading the
-	 * notify_on_completion flag, and will then
-	 * call mbox_chan_txdone when the acknowledgment interrupt is
-	 * received.
-	 */
-	bool manage_writes;
-
-	/* Optional callback that allows the driver
-	 * to allocate the memory used for receiving
-	 * messages.  The return value is the location
-	 * inside the buffer where the mailbox should write the data.
-	 */
-	void *(*rx_alloc)(struct mbox_client *cl,  int size);
 };
 
 struct pcc_header {
-- 
2.43.0

[PATCH net-next v29 3/3] mctp pcc: Implement MCTP over PCC Transport

[Adam Young]

Implementation of network driver for
Management Component Transport Protocol(MCTP)
over Platform Communication Channel(PCC)

DMTF DSP:0292
https://www.dmtf.org/sites/default/files/standards/documents/\
DSP0292_1.0.0WIP50.pdf

MCTP devices are specified via ACPI by entries in DSDT/SSDT and reference
channels specified in the PCCT.  Messages are sent on a type 3 and
received on a type 4 channel.  Communication with other devices use the
PCC based doorbell mechanism; a shared memory segment with a corresponding
interrupt and a memory register used to trigger remote interrupts.

This driver takes advantage of PCC mailbox buffer management. The data
section of the struct sk_buff that contains the outgoing packet is sent to
the mailbox, already properly formatted  as a PCC message.

The driver makes use of the mailbox-api rx_alloc callback.  This is
responsible for allocating a struct sk_buff that is then passed to the
mailbox and used to record the data in the shared buffer. It maintains a
list of both outgoing and incoming sk_buffs to match the data buffers.

If the mailbox ring buffer is full, the driver stops the incoming packet
queues until a message has been sent, freeing space in the ring buffer.

When the Type 3 channel outbox receives a txdone response interrupt, it
consumes the outgoing sk_buff, allowing it to be freed.

Bringing up an interface creates the channel between the network driver and
the mailbox driver.  This enables communication with the remote endpoint,
to include the receipt of new messages. Bringing down an interface removes
the channel, and no new messages can be delivered.  Stopping the interface
also frees any packets that are cached in the mailbox ringbuffer.

Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
---
 MAINTAINERS                 |   5 +
 drivers/net/mctp/Kconfig    |  13 ++
 drivers/net/mctp/Makefile   |   1 +
 drivers/net/mctp/mctp-pcc.c | 341 ++++++++++++++++++++++++++++++++++++
 4 files changed, 360 insertions(+)
 create mode 100644 drivers/net/mctp/mctp-pcc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a8a770714101..f7b9e935543b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14677,6 +14677,11 @@ F:	include/net/mctpdevice.h
 F:	include/net/netns/mctp.h
 F:	net/mctp/
 
+MANAGEMENT COMPONENT TRANSPORT PROTOCOL (MCTP) over PCC (MCTP-PCC) Driver
+M:	Adam Young <admiyo@os.amperecomputing.com>
+S:	Maintained
+F:	drivers/net/mctp/mctp-pcc.c
+
 MAPLE TREE
 M:	Liam R. Howlett <Liam.Howlett@oracle.com>
 L:	maple-tree@lists.infradead.org
diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
index cf325ab0b1ef..01855846491b 100644
--- a/drivers/net/mctp/Kconfig
+++ b/drivers/net/mctp/Kconfig
@@ -57,6 +57,19 @@ config MCTP_TRANSPORT_USB
 	  MCTP-over-USB interfaces are peer-to-peer, so each interface
 	  represents a physical connection to one remote MCTP endpoint.
 
+config MCTP_TRANSPORT_PCC
+	tristate "MCTP PCC transport"
+	depends on ACPI
+	help
+	  Provides a driver to access MCTP devices over PCC transport,
+	  A MCTP protocol network device is created via ACPI for each
+	  entry in the DSDT/SSDT that matches the identifier. The Platform
+	  communication channels are selected from the corresponding
+	  entries in the PCCT.
+
+	  Say y here if you need to connect to MCTP endpoints over PCC. To
+	  compile as a module, use m; the module will be called mctp-pcc.
+
 endmenu
 
 endif
diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile
index c36006849a1e..2276f148df7c 100644
--- a/drivers/net/mctp/Makefile
+++ b/drivers/net/mctp/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_MCTP_TRANSPORT_PCC) += mctp-pcc.o
 obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o
 obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o
 obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.o
diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
new file mode 100644
index 000000000000..b525df86e459
--- /dev/null
+++ b/drivers/net/mctp/mctp-pcc.c
@@ -0,0 +1,341 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * mctp-pcc.c - Driver for MCTP over PCC.
+ * Copyright (c) 2024-2025, Ampere Computing LLC
+ *
+ */
+
+/* Implementation of MCTP over PCC DMTF Specification DSP0256
+ * https://www.dmtf.org/sites/default/files/standards/documents/DSP0292_1.0.0WIP50.pdf
+ */
+
+#include <linux/acpi.h>
+#include <linux/if_arp.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+#include <linux/skbuff.h>
+#include <linux/hrtimer.h>
+
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+#include <acpi/acrestyp.h>
+#include <acpi/actbl.h>
+#include <net/mctp.h>
+#include <net/mctpdevice.h>
+#include <acpi/pcc.h>
+
+#define MCTP_SIGNATURE          "MCTP"
+#define MCTP_SIGNATURE_LENGTH   (sizeof(MCTP_SIGNATURE) - 1)
+#define MCTP_MIN_MTU            68
+#define PCC_DWORD_TYPE          0x0c
+
+struct mctp_pcc_mailbox {
+	u32 index;
+	struct pcc_mbox_chan *chan;
+	struct mbox_client client;
+	struct sk_buff_head packets;
+};
+
+/* The netdev structure. One of these per PCC adapter. */
+struct mctp_pcc_ndev {
+	struct net_device *ndev;
+	struct acpi_device *acpi_device;
+	struct mctp_pcc_mailbox inbox;
+	struct mctp_pcc_mailbox outbox;
+};
+
+static  void mctp_pcc_rx_alloc(struct mbox_client *c, void **handle, void **buffer, int size)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev;
+	struct sk_buff *skb;
+
+	mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, inbox.client);
+
+	*handle = NULL;
+	*buffer = NULL;
+
+	if (size > mctp_pcc_ndev->ndev->mtu)
+		return;
+	skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
+	if (!skb)
+		return;
+	skb_put(skb, size);
+	skb->protocol = htons(ETH_P_MCTP);
+
+	*buffer = skb->data;
+	*handle = skb;
+}
+
+static void mctp_pcc_client_rx_callback(struct mbox_client *c, void *buffer)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev;
+	struct pcc_header pcc_header;
+	struct sk_buff *skb = buffer;
+	struct mctp_skb_cb *cb;
+
+	mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, inbox.client);
+	if (!buffer) {
+		dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
+		return;
+	}
+
+	if (skb) {
+		dev_dstats_rx_add(mctp_pcc_ndev->ndev, skb->len);
+		skb_reset_mac_header(skb);
+		skb_pull(skb, sizeof(pcc_header));
+		skb_reset_network_header(skb);
+		cb = __mctp_cb(skb);
+		cb->halen = 0;
+		netif_rx(skb);
+	}
+}
+
+static void mctp_pcc_tx_done(struct mbox_client *c, void *mssg, int r)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev;
+	struct mctp_pcc_mailbox *outbox;
+	struct sk_buff *skb = NULL;
+	struct sk_buff *curr_skb;
+
+	mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, outbox.client);
+	outbox = container_of(c, struct mctp_pcc_mailbox, client);
+	spin_lock(&outbox->packets.lock);
+	skb_queue_walk(&outbox->packets, curr_skb) {
+		if (curr_skb->data == mssg) {
+			skb = curr_skb;
+			__skb_unlink(skb, &outbox->packets);
+			break;
+		}
+	}
+	spin_unlock(&outbox->packets.lock);
+	if (skb)
+		dev_consume_skb_any(skb);
+	netif_wake_queue(mctp_pcc_ndev->ndev);
+}
+
+static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
+	struct pcc_header *pcc_header;
+	int len = skb->len;
+	int rc;
+
+	rc = skb_cow_head(skb, sizeof(*pcc_header));
+	if (rc) {
+		dev_dstats_tx_dropped(ndev);
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
+	}
+
+	pcc_header = skb_push(skb, sizeof(*pcc_header));
+	pcc_header->signature = PCC_SIGNATURE | mpnd->outbox.index;
+	pcc_header->flags = PCC_CMD_COMPLETION_NOTIFY;
+	memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
+	pcc_header->length = len + MCTP_SIGNATURE_LENGTH;
+
+	skb_queue_head(&mpnd->outbox.packets, skb);
+
+	rc = mbox_send_message(mpnd->outbox.chan->mchan, skb->data);
+
+	if (rc < 0) {
+		netif_stop_queue(ndev);
+		skb_unlink(skb, &mpnd->outbox.packets);
+		return NETDEV_TX_BUSY;
+	}
+
+	dev_dstats_tx_add(ndev, len);
+	return NETDEV_TX_OK;
+}
+
+static void drain_packets(struct sk_buff_head *list)
+{
+	struct sk_buff *skb;
+
+	while (!skb_queue_empty(list)) {
+		skb = skb_dequeue(list);
+		dev_consume_skb_any(skb);
+	}
+}
+
+static int mctp_pcc_ndo_open(struct net_device *ndev)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
+	struct mctp_pcc_mailbox *out = &mctp_pcc_ndev->outbox;
+	struct mctp_pcc_mailbox *in = &mctp_pcc_ndev->inbox;
+
+	out->chan = pcc_mbox_request_channel(&out->client, out->index);
+	if (IS_ERR(out->chan))
+		return PTR_ERR(out->chan);
+
+	in->client.rx_alloc = mctp_pcc_rx_alloc;
+	in->client.rx_callback = mctp_pcc_client_rx_callback;
+	in->chan = pcc_mbox_request_channel(&in->client, in->index);
+	if (IS_ERR(in->chan)) {
+		pcc_mbox_free_channel(out->chan);
+		return PTR_ERR(in->chan);
+	}
+
+	return 0;
+}
+
+static int mctp_pcc_ndo_stop(struct net_device *ndev)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
+
+	pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
+	pcc_mbox_free_channel(mctp_pcc_ndev->inbox.chan);
+	drain_packets(&mctp_pcc_ndev->outbox.packets);
+	return 0;
+}
+
+static const struct net_device_ops mctp_pcc_netdev_ops = {
+	.ndo_open = mctp_pcc_ndo_open,
+	.ndo_stop = mctp_pcc_ndo_stop,
+	.ndo_start_xmit = mctp_pcc_tx,
+};
+
+static void mctp_pcc_setup(struct net_device *ndev)
+{
+	ndev->type = ARPHRD_MCTP;
+	ndev->hard_header_len = 0;
+	ndev->tx_queue_len = 0;
+	ndev->flags = IFF_NOARP;
+	ndev->netdev_ops = &mctp_pcc_netdev_ops;
+	ndev->needs_free_netdev = true;
+	ndev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
+}
+
+struct mctp_pcc_lookup_context {
+	int index;
+	u32 inbox_index;
+	u32 outbox_index;
+};
+
+static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
+				       void *context)
+{
+	struct mctp_pcc_lookup_context *luc = context;
+	struct acpi_resource_address32 *addr;
+
+	if (ares->type != PCC_DWORD_TYPE)
+		return AE_OK;
+
+	addr = ACPI_CAST_PTR(struct acpi_resource_address32, &ares->data);
+	switch (luc->index) {
+	case 0:
+		luc->outbox_index = addr[0].address.minimum;
+		break;
+	case 1:
+		luc->inbox_index = addr[0].address.minimum;
+		break;
+	}
+	luc->index++;
+	return AE_OK;
+}
+
+static void mctp_cleanup_netdev(void *data)
+{
+	struct net_device *ndev = data;
+
+	mctp_unregister_netdev(ndev);
+}
+
+static int initialize_MTU(struct net_device *ndev)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
+	struct mctp_pcc_mailbox *outbox;
+	int mctp_pcc_mtu;
+
+	outbox = &mctp_pcc_ndev->outbox;
+	outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
+	mctp_pcc_mtu = outbox->chan->shmem_size - sizeof(struct pcc_header);
+	if (IS_ERR(outbox->chan))
+		return PTR_ERR(outbox->chan);
+
+	pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
+
+	mctp_pcc_ndev = netdev_priv(ndev);
+	ndev->mtu = MCTP_MIN_MTU;
+	ndev->max_mtu = mctp_pcc_mtu;
+	ndev->min_mtu = MCTP_MIN_MTU;
+
+	return 0;
+}
+
+static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
+{
+	struct mctp_pcc_lookup_context context = {0};
+	struct mctp_pcc_ndev *mctp_pcc_ndev;
+	struct device *dev = &acpi_dev->dev;
+	struct net_device *ndev;
+	acpi_handle dev_handle;
+	acpi_status status;
+	char name[32];
+	int rc;
+
+	dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
+		acpi_device_hid(acpi_dev));
+	dev_handle = acpi_device_handle(acpi_dev);
+	status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
+				     &context);
+	if (!ACPI_SUCCESS(status)) {
+		dev_err(dev, "FAILED to lookup PCC indexes from CRS\n");
+		return -EINVAL;
+	}
+
+	snprintf(name, sizeof(name), "mctppcc%d", context.inbox_index);
+	ndev = alloc_netdev(sizeof(*mctp_pcc_ndev), name, NET_NAME_PREDICTABLE,
+			    mctp_pcc_setup);
+	if (!ndev)
+		return -ENOMEM;
+
+	mctp_pcc_ndev = netdev_priv(ndev);
+
+	mctp_pcc_ndev->inbox.index = context.inbox_index;
+	mctp_pcc_ndev->inbox.client.dev = dev;
+
+	mctp_pcc_ndev->outbox.index = context.outbox_index;
+	skb_queue_head_init(&mctp_pcc_ndev->outbox.packets);
+	mctp_pcc_ndev->outbox.client.dev = dev;
+
+	mctp_pcc_ndev->outbox.client.tx_done = mctp_pcc_tx_done;
+	mctp_pcc_ndev->acpi_device = acpi_dev;
+	mctp_pcc_ndev->ndev = ndev;
+	acpi_dev->driver_data = mctp_pcc_ndev;
+
+	initialize_MTU(ndev);
+
+	rc = mctp_register_netdev(ndev, NULL, MCTP_PHYS_BINDING_PCC);
+	if (rc) {
+		free_netdev(ndev);
+		return rc;
+	}
+	return devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
+}
+
+static const struct acpi_device_id mctp_pcc_device_ids[] = {
+	{ "DMT0001" },
+	{}
+};
+
+static struct acpi_driver mctp_pcc_driver = {
+	.name = "mctp_pcc",
+	.class = "Unknown",
+	.ids = mctp_pcc_device_ids,
+	.ops = {
+		.add = mctp_pcc_driver_add,
+	},
+};
+
+module_acpi_driver(mctp_pcc_driver);
+
+MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids);
+
+MODULE_DESCRIPTION("MCTP PCC ACPI device");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>");
-- 
2.43.0

Re: [PATCH net-next v29 1/3] mailbox: add callback function for rx buffer allocation

[Simon Horman]

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
> 
> 

Re: [PATCH net-next v29 3/3] mctp pcc: Implement MCTP over PCC Transport

[Simon Horman]

On Thu, Sep 25, 2025 at 03:00:26PM -0400, Adam Young wrote:

...

> +static int initialize_MTU(struct net_device *ndev)
> +{
> +	struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
> +	struct mctp_pcc_mailbox *outbox;
> +	int mctp_pcc_mtu;
> +
> +	outbox = &mctp_pcc_ndev->outbox;
> +	outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
> +	mctp_pcc_mtu = outbox->chan->shmem_size - sizeof(struct pcc_header);

Hi Adam,

On the line below it is expected that outbox->chan may be an error value
rather than a valid pointer. But on the line above outbox->chan is
dereferenced.

This does not seem consistent.

Flagged by Smatch.

> +	if (IS_ERR(outbox->chan))
> +		return PTR_ERR(outbox->chan);
> +
> +	pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
> +
> +	mctp_pcc_ndev = netdev_priv(ndev);
> +	ndev->mtu = MCTP_MIN_MTU;
> +	ndev->max_mtu = mctp_pcc_mtu;
> +	ndev->min_mtu = MCTP_MIN_MTU;
> +
> +	return 0;
> +}

...

-- 
pw-bot: changes-requested

Re: [PATCH net-next v29 1/3] mailbox: add callback function for rx buffer allocation

[Sudeep Holla]

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

Re: [External] : [PATCH net-next v29 3/3] mctp pcc: Implement MCTP over PCC Transport

[ALOK TIWARI]

On 9/26/2025 12:30 AM, Adam Young wrote:
> +static int initialize_MTU(struct net_device *ndev)
> +{
> +	struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
> +	struct mctp_pcc_mailbox *outbox;
> +	int mctp_pcc_mtu;
> +
> +	outbox = &mctp_pcc_ndev->outbox;
> +	outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
> +	mctp_pcc_mtu = outbox->chan->shmem_size - sizeof(struct pcc_header);

de-reference outbox->chan->shmem_size before checking IS_ERR(outbox->chan)

> +	if (IS_ERR(outbox->chan))
> +		return PTR_ERR(outbox->chan);
> +
> +	pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
> +
> +	mctp_pcc_ndev = netdev_priv(ndev);
> +	ndev->mtu = MCTP_MIN_MTU;
> +	ndev->max_mtu = mctp_pcc_mtu;
> +	ndev->min_mtu = MCTP_MIN_MTU;
> +
> +	return 0;
> +}


Thanks,
Alok

Re: [PATCH net-next v29 1/3] mailbox: add callback function for rx buffer allocation

[Adam Young]

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.


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);

Re: [PATCH net-next v29 1/3] mailbox: add callback function for rx buffer allocation

[Jassi Brar]

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

Re: [PATCH net-next v29 1/3] mailbox: add callback function for rx buffer allocation

[Sudeep Holla]

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

Re: [PATCH net-next v29 1/3] mailbox: add callback function for rx buffer allocation

[Adam Young]

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

Re: [PATCH net-next v29 1/3] mailbox: add callback function for rx buffer allocation

[Sudeep Holla]

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