[PATCH v23 0/2] MCTP Over PCC Transport

[PATCH v23 0/2] MCTP Over PCC Transport

[admiyo]

From: Linux Bot <linuxbot@amperecomputing.com>

This series adds support for the Management Control 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.

The first patch in the series implements a mechanism to allow the driver
to indicate whether an ACK should be sent back to the caller
after processing the interrupt.  This is an optional feature in
the PCC code, but has been made explicitly required in another driver.
The implementation here maintains the backwards compatibility of that
driver.

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.

Previous implementations of the pcc version of the mailbox protocol assumed the
driver was directly managing the shared memory region.  This lead to duplicated
code and missed stpes of the PCC protocol. The first patch in this series makes
it possible for mailbox/pcc to manage the writing of the buffer prior to sending
messages.  It also fixes the notification of message transmission completion.

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

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 (2):
  mailbox/pcc: support mailbox management of the shared buffer
  mctp pcc: Implement MCTP over PCC Transport

 MAINTAINERS                 |   5 +
 drivers/mailbox/pcc.c       | 102 ++++++++++-
 drivers/net/mctp/Kconfig    |  13 ++
 drivers/net/mctp/Makefile   |   1 +
 drivers/net/mctp/mctp-pcc.c | 347 ++++++++++++++++++++++++++++++++++++
 include/acpi/pcc.h          |  29 +++
 6 files changed, 493 insertions(+), 4 deletions(-)
 create mode 100644 drivers/net/mctp/mctp-pcc.c

-- 
2.43.0

[PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[admiyo]

From: Adam Young <admiyo@os.amperecomputing.com>

Define a new, optional, callback that allows the driver to
specify how the return data buffer is allocated.  If that callback
is set,  mailbox/pcc.c is now responsible for reading from and
writing to the PCC shared buffer.

This also allows for proper checks of the Commnand complete flag
between the PCC sender and receiver.

For Type 4 channels, initialize the command complete flag prior
to accepting messages.

Since the mailbox does not know what memory allocation scheme
to use for response messages, the client now has an optional
callback that allows it to allocate the buffer for a response
message.

When an outbound message is written to the buffer, the mailbox
checks for the flag indicating the client wants an tx complete
notification via IRQ.  Upon receipt of the interrupt It will
pair it with the outgoing message. The expected use is to
free the kernel memory buffer for the previous outgoing message.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index f6714c233f5a..0a00719b2482 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -306,6 +306,22 @@ static void pcc_chan_acknowledge(struct pcc_chan_info *pchan)
 		pcc_chan_reg_read_modify_write(&pchan->db);
 }
 
+static void *write_response(struct pcc_chan_info *pchan)
+{
+	struct pcc_header pcc_header;
+	void *buffer;
+	int data_len;
+
+	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);
+	if (buffer != NULL)
+		memcpy_fromio(buffer, pchan->chan.shmem, data_len);
+	return buffer;
+}
+
 /**
  * pcc_mbox_irq - PCC mailbox interrupt handler
  * @irq:	interrupt number
@@ -317,6 +333,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 {
 	struct pcc_chan_info *pchan;
 	struct mbox_chan *chan = p;
+	struct pcc_header *pcc_header = chan->active_req;
+	void *handle = NULL;
 
 	pchan = chan->con_priv;
 
@@ -340,7 +358,17 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	 * required to avoid any possible race in updatation of this flag.
 	 */
 	pchan->chan_in_use = false;
-	mbox_chan_received_data(chan, NULL);
+
+	if (pchan->chan.rx_alloc)
+		handle = write_response(pchan);
+
+	if (chan->active_req) {
+		pcc_header = chan->active_req;
+		if (pcc_header->flags & PCC_CMD_COMPLETION_NOTIFY)
+			mbox_chan_txdone(chan, 0);
+	}
+
+	mbox_chan_received_data(chan, handle);
 
 	pcc_chan_acknowledge(pchan);
 
@@ -384,9 +412,24 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 	pcc_mchan = &pchan->chan;
 	pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
 					   pcc_mchan->shmem_size);
-	if (pcc_mchan->shmem)
-		return pcc_mchan;
+	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
+	 * the mailbox API. That implies that the mailbox client must
+	 * have set the allocate callback function prior to
+	 * sending any messages.
+	 */
+	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
+		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
+
+	return pcc_mchan;
 
+err:
 	mbox_free_channel(chan);
 	return ERR_PTR(-ENXIO);
 }
@@ -417,8 +460,38 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
 }
 EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
 
+static int pcc_write_to_buffer(struct mbox_chan *chan, void *data)
+{
+	struct pcc_chan_info *pchan = chan->con_priv;
+	struct pcc_mbox_chan *pcc_mbox_chan = &pchan->chan;
+	struct pcc_header *pcc_header = data;
+
+	if (!pchan->chan.manage_writes)
+		return 0;
+
+	/* The PCC header length includes the command field
+	 * but not the other values from the header.
+	 */
+	int len = pcc_header->length - sizeof(u32) + sizeof(struct pcc_header);
+	u64 val;
+
+	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);
+	return 0;
+}
+
+
 /**
- * pcc_send_data - Called from Mailbox Controller code. Used
+ * pcc_send_data - Called from Mailbox Controller code. If
+ *		pchan->chan.rx_alloc is set, then the command complete
+ *		flag is checked and the data is written to the shared
+ *		buffer io memory.
+ *
+ *		If pchan->chan.rx_alloc is not set, then it is used
  *		here only to ring the channel doorbell. The PCC client
  *		specific read/write is done in the client driver in
  *		order to maintain atomicity over PCC channel once
@@ -434,17 +507,37 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
 	int ret;
 	struct pcc_chan_info *pchan = chan->con_priv;
 
+	ret = pcc_write_to_buffer(chan, data);
+	if (ret)
+		return ret;
+
 	ret = pcc_chan_reg_read_modify_write(&pchan->cmd_update);
 	if (ret)
 		return ret;
 
 	ret = pcc_chan_reg_read_modify_write(&pchan->db);
+
 	if (!ret && pchan->plat_irq > 0)
 		pchan->chan_in_use = true;
 
 	return ret;
 }
 
+
+static bool pcc_last_tx_done(struct mbox_chan *chan)
+{
+	struct pcc_chan_info *pchan = chan->con_priv;
+	u64 val;
+
+	pcc_chan_reg_read(&pchan->cmd_complete, &val);
+	if (!val)
+		return false;
+	else
+		return true;
+}
+
+
+
 /**
  * pcc_startup - Called from Mailbox Controller code. Used here
  *		to request the interrupt.
@@ -490,6 +583,7 @@ static const struct mbox_chan_ops pcc_chan_ops = {
 	.send_data = pcc_send_data,
 	.startup = pcc_startup,
 	.shutdown = pcc_shutdown,
+	.last_tx_done = pcc_last_tx_done,
 };
 
 /**
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 840bfc95bae3..9af3b502f839 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -17,6 +17,35 @@ 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 {
+	u32 signature;
+	u32 flags;
+	u32 length;
+	u32 command;
 };
 
 /* Generic Communications Channel Shared Memory Region */
-- 
2.43.0

[PATCH v23 2/2] mctp pcc: Implement MCTP over PCC Transport

[admiyo]

From: Adam Young <admiyo@os.amperecomputing.com>

Implementation of network driver for
Management Control 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/SDST 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
is also reponsible 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
outging and incoming sk_buffs to match the data buffers
with the original sk_buffs.

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

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 | 347 ++++++++++++++++++++++++++++++++++++
 4 files changed, 366 insertions(+)
 create mode 100644 drivers/net/mctp/mctp-pcc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3887d5906786..3939d816657d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14469,6 +14469,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..f69d0237f058 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 DST/SDST 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..045711a21395
--- /dev/null
+++ b/drivers/net/mctp/mctp-pcc.c
@@ -0,0 +1,347 @@
+// 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/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>
+
+#include "../../mailbox/mailbox.h"
+
+#define MCTP_PAYLOAD_LENGTH     256
+#define MCTP_CMD_LENGTH         4
+#define MCTP_PCC_VERSION        0x1 /* DSP0292 a single version: 1 */
+#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 {
+	/* spinlock to serialize access to PCC outbox buffer and registers
+	 * Note that what PCC calls registers are memory locations, not CPU
+	 * Registers.  They include the fields used to synchronize access
+	 * between the OS and remote endpoints.
+	 *
+	 * Only the Outbox needs a spinlock, to prevent multiple
+	 * sent packets triggering multiple attempts to over write
+	 * the outbox.  The Inbox buffer is controlled by the remote
+	 * service and a spinlock would have no effect.
+	 */
+	spinlock_t lock;
+	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, int size)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev =
+		container_of(c, struct mctp_pcc_ndev, inbox.client);
+	struct mctp_pcc_mailbox *box = &mctp_pcc_ndev->inbox;
+	struct sk_buff *skb;
+
+	if (size > mctp_pcc_ndev->ndev->mtu)
+		return NULL;
+	skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
+	if (!skb)
+		return NULL;
+	skb_put(skb, size);
+	skb->protocol = htons(ETH_P_MCTP);
+
+	skb_queue_head(&box->packets, skb);
+
+	return skb->data;
+}
+
+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 mctp_skb_cb *cb;
+	struct sk_buff *skb;
+
+	mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, inbox.client);
+	if (!buffer) {
+		dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
+		return;
+	}
+
+	skb_queue_walk(&mctp_pcc_ndev->inbox.packets, skb) {
+		if (skb->data != buffer)
+			continue;
+		skb_unlink(skb, &mctp_pcc_ndev->inbox.packets);
+		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);
+		return;
+	}
+	pr_warn("Unmatched packet in mctp-pcc inbox packet list");
+}
+
+static void mctp_pcc_tx_done(struct mbox_client *c, void *mssg, int r)
+{
+	struct mctp_pcc_mailbox *box;
+	struct sk_buff *skb;
+
+	box = container_of(c, struct mctp_pcc_mailbox, client);
+	skb_queue_walk(&box->packets, skb) {
+		if (skb->data == mssg) {
+			skb_unlink(skb, &box->packets);
+			dev_consume_skb_any(skb);
+			break;
+		}
+	}
+}
+
+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 = cpu_to_le32(PCC_SIGNATURE | mpnd->outbox.index);
+	pcc_header->flags = cpu_to_le32(PCC_CMD_COMPLETION_NOTIFY);
+	memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
+	pcc_header->length = cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);
+	skb_queue_head(&mpnd->outbox.packets, skb);
+
+	rc = mbox_send_message(mpnd->outbox.chan->mchan, skb->data);
+
+	if (rc < 0) {
+		skb_unlink(skb, &mpnd->outbox.packets);
+		return NETDEV_TX_BUSY;
+	}
+
+	dev_dstats_tx_add(ndev, len);
+	return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops mctp_pcc_netdev_ops = {
+	.ndo_start_xmit = mctp_pcc_tx,
+};
+
+static const struct mctp_netdev_ops mctp_netdev_ops = {
+	NULL
+};
+
+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 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 void mctp_cleanup_netdev(void *data)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev;
+	struct net_device *ndev = data;
+
+	mctp_pcc_ndev = netdev_priv(ndev);
+	drain_packets(&mctp_pcc_ndev->outbox.packets);
+	drain_packets(&mctp_pcc_ndev->inbox.packets);
+
+	mctp_unregister_netdev(ndev);
+}
+
+static void mctp_cleanup_channel(void *data)
+{
+	struct pcc_mbox_chan *chan = data;
+
+	pcc_mbox_free_channel(chan);
+}
+
+static int mctp_pcc_initialize_mailbox(struct device *dev,
+				       struct mctp_pcc_mailbox *box, u32 index)
+{
+	box->index = index;
+	skb_queue_head_init(&box->packets);
+	box->chan = pcc_mbox_request_channel(&box->client, index);
+
+	box->client.dev = dev;
+	if (IS_ERR(box->chan))
+		return PTR_ERR(box->chan);
+	return devm_add_action_or_reset(dev, mctp_cleanup_channel, box->chan);
+}
+
+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;
+	int mctp_pcc_mtu;
+	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, "FAILURE to lookup PCC indexes from CRS\n");
+		return -EINVAL;
+	}
+
+	snprintf(name, sizeof(name), "mctpipcc%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);
+	spin_lock_init(&mctp_pcc_ndev->lock);
+
+	/* inbox initialization */
+	rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
+					 context.inbox_index);
+	if (rc)
+		goto free_netdev;
+
+	mctp_pcc_ndev->inbox.chan->rx_alloc = mctp_pcc_rx_alloc;
+	mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
+
+	/* outbox initialization */
+	rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox,
+					 context.outbox_index);
+	if (rc)
+		goto free_netdev;
+
+	mctp_pcc_ndev->outbox.chan->manage_writes = true;
+	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;
+
+	/* There is no clean way to pass the MTU to the callback function
+	 * used for registration, so set the values ahead of time.
+	 */
+	mctp_pcc_mtu = mctp_pcc_ndev->outbox.chan->shmem_size -
+		sizeof(struct pcc_header);
+	ndev->mtu = MCTP_MIN_MTU;
+	ndev->max_mtu = mctp_pcc_mtu;
+	ndev->min_mtu = MCTP_MIN_MTU;
+
+	/* ndev needs to be freed before the iomemory (mapped above) gets
+	 * unmapped,  devm resources get freed in reverse to the order they
+	 * are added.
+	 */
+	rc = mctp_register_netdev(ndev, &mctp_netdev_ops, MCTP_PHYS_BINDING_PCC);
+	if (rc)
+		goto free_netdev;
+
+	return devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
+free_netdev:
+	free_netdev(ndev);
+	return rc;
+}
+
+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 v23 2/2] mctp pcc: Implement MCTP over PCC Transport

[Simon Horman]

On Mon, Jul 14, 2025 at 08:10:08PM -0400, admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@os.amperecomputing.com>
> 
> Implementation of network driver for
> Management Control 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/SDST 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
> is also reponsible for allocating a struct sk_buff that

responsible

> is then passed to the mailbox and used to record the
> data in the shared buffer. It maintains a list of both
> outging and incoming sk_buffs to match the data buffers
> with the original sk_buffs.
> 
> When the Type 3 channel outbox receives a txdone response
> interrupt, it consumes the outgoing sk_buff, allowing
> it to be freed.
> 
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>

...

> diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c

...

> +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 = cpu_to_le32(PCC_SIGNATURE | mpnd->outbox.index);
> +	pcc_header->flags = cpu_to_le32(PCC_CMD_COMPLETION_NOTIFY);
> +	memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
> +	pcc_header->length = cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);

Hi Adam,

There seems to be an endian missmatch here: host order values
are being converted to little endian values and then
assigned to host endian variables.

Sparse says:

  .../mctp-pcc.c:146:31: warning: incorrect type in assignment (different base types)
  .../mctp-pcc.c:146:31:    expected unsigned int [usertype] signature
  .../mctp-pcc.c:146:31:    got restricted __le32 [usertype]
  .../mctp-pcc.c:147:27: warning: incorrect type in assignment (different base types)
  .../mctp-pcc.c:147:27:    expected unsigned int [usertype] flags
  .../mctp-pcc.c:147:27:    got restricted __le32 [usertype]
  .../mctp-pcc.c:149:28: warning: incorrect type in assignment (different base types)
  .../mctp-pcc.c:149:28:    expected unsigned int [usertype] length
  .../mctp-pcc.c:149:28:    got restricted __le32 [usertype]

> +	skb_queue_head(&mpnd->outbox.packets, skb);
> +
> +	rc = mbox_send_message(mpnd->outbox.chan->mchan, skb->data);
> +
> +	if (rc < 0) {
> +		skb_unlink(skb, &mpnd->outbox.packets);
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	dev_dstats_tx_add(ndev, len);
> +	return NETDEV_TX_OK;
> +}

...

> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
> +{

...

> +	/* ndev needs to be freed before the iomemory (mapped above) gets
> +	 * unmapped,  devm resources get freed in reverse to the order they
> +	 * are added.
> +	 */
> +	rc = mctp_register_netdev(ndev, &mctp_netdev_ops, MCTP_PHYS_BINDING_PCC);

nit: please line wrap to 80 columbs wide or less:

	rc = mctp_register_netdev(ndev, &mctp_netdev_ops,
				  MCTP_PHYS_BINDING_PCC);

> +	if (rc)
> +		goto free_netdev;
> +
> +	return devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
> +free_netdev:
> +	free_netdev(ndev);
> +	return rc;
> +}

...

pw-bot: changes-requested

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Simon Horman]

On Mon, Jul 14, 2025 at 08:10:07PM -0400, admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@os.amperecomputing.com>
> 
> Define a new, optional, callback that allows the driver to
> specify how the return data buffer is allocated.  If that callback
> is set,  mailbox/pcc.c is now responsible for reading from and
> writing to the PCC shared buffer.
> 
> This also allows for proper checks of the Commnand complete flag
> between the PCC sender and receiver.

Command

> 
> For Type 4 channels, initialize the command complete flag prior
> to accepting messages.
> 
> Since the mailbox does not know what memory allocation scheme
> to use for response messages, the client now has an optional
> callback that allows it to allocate the buffer for a response
> message.
> 
> When an outbound message is written to the buffer, the mailbox
> checks for the flag indicating the client wants an tx complete
> notification via IRQ.  Upon receipt of the interrupt It will
> pair it with the outgoing message. The expected use is to
> free the kernel memory buffer for the previous outgoing message.
> 
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>

...

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Adam Young]

On 7/14/25 20:10, admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@os.amperecomputing.com>
>
> Define a new, optional, callback that allows the driver to
> specify how the return data buffer is allocated.  If that callback
> is set,  mailbox/pcc.c is now responsible for reading from and
> writing to the PCC shared buffer.
>
> This also allows for proper checks of the Commnand complete flag
> between the PCC sender and receiver.
>
> For Type 4 channels, initialize the command complete flag prior
> to accepting messages.
>
> Since the mailbox does not know what memory allocation scheme
> to use for response messages, the client now has an optional
> callback that allows it to allocate the buffer for a response
> message.
>
> When an outbound message is written to the buffer, the mailbox
> checks for the flag indicating the client wants an tx complete
> notification via IRQ.  Upon receipt of the interrupt It will
> pair it with the outgoing message. The expected use is to
> free the kernel memory buffer for the previous outgoing message.
>
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
>   drivers/mailbox/pcc.c | 102 ++++++++++++++++++++++++++++++++++++++++--
>   include/acpi/pcc.h    |  29 ++++++++++++
>   2 files changed, 127 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index f6714c233f5a..0a00719b2482 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -306,6 +306,22 @@ static void pcc_chan_acknowledge(struct pcc_chan_info *pchan)
>   		pcc_chan_reg_read_modify_write(&pchan->db);
>   }
>   
> +static void *write_response(struct pcc_chan_info *pchan)
> +{
> +	struct pcc_header pcc_header;
> +	void *buffer;
> +	int data_len;
> +
> +	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);
> +	if (buffer != NULL)
> +		memcpy_fromio(buffer, pchan->chan.shmem, data_len);
> +	return buffer;
> +}
> +
>   /**
>    * pcc_mbox_irq - PCC mailbox interrupt handler
>    * @irq:	interrupt number
> @@ -317,6 +333,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   {
>   	struct pcc_chan_info *pchan;
>   	struct mbox_chan *chan = p;
> +	struct pcc_header *pcc_header = chan->active_req;

OK, so it looks a little strange to re-initialize this later. Would it 
be better to not have it initialized?


> +	void *handle = NULL;
>   
>   	pchan = chan->con_priv;
>   
> @@ -340,7 +358,17 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	 * required to avoid any possible race in updatation of this flag.
>   	 */
>   	pchan->chan_in_use = false;
> -	mbox_chan_received_data(chan, NULL);
> +
> +	if (pchan->chan.rx_alloc)
> +		handle = write_response(pchan);
> +
> +	if (chan->active_req) {
> +		pcc_header = chan->active_req;
> +		if (pcc_header->flags & PCC_CMD_COMPLETION_NOTIFY)

Note that this is the counterpoint to my earlier patch that only 
notifies the platform if the platform requests it.  This is part of the 
specification.

> +			mbox_chan_txdone(chan, 0);
> +	}
> +
> +	mbox_chan_received_data(chan, handle);
>   
>   	pcc_chan_acknowledge(pchan);
>   
> @@ -384,9 +412,24 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   	pcc_mchan = &pchan->chan;
>   	pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
>   					   pcc_mchan->shmem_size);
> -	if (pcc_mchan->shmem)
> -		return pcc_mchan;
> +	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
> +	 * the mailbox API. That implies that the mailbox client must
> +	 * have set the allocate callback function prior to
> +	 * sending any messages.
> +	 */
> +	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> +		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
Is there a better  way to do this?  The flag is not accessable from the 
driver.
> +
> +	return pcc_mchan;
>   
> +err:
>   	mbox_free_channel(chan);
>   	return ERR_PTR(-ENXIO);
>   }
> @@ -417,8 +460,38 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>   }
>   EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>   
> +static int pcc_write_to_buffer(struct mbox_chan *chan, void *data)
> +{
> +	struct pcc_chan_info *pchan = chan->con_priv;
> +	struct pcc_mbox_chan *pcc_mbox_chan = &pchan->chan;
> +	struct pcc_header *pcc_header = data;
> +
> +	if (!pchan->chan.manage_writes)
> +		return 0;
> +
> +	/* The PCC header length includes the command field
> +	 * but not the other values from the header.
> +	 */
> +	int len = pcc_header->length - sizeof(u32) + sizeof(struct pcc_header);
> +	u64 val;
> +
> +	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);
> +	return 0;
> +}
> +

I think this is the pattern that we  want all of the PCC mailbox clients 
to migrate to.  Is there any reason it was not implmented this way 
originally?-

> +
>   /**
> - * pcc_send_data - Called from Mailbox Controller code. Used
> + * pcc_send_data - Called from Mailbox Controller code. If
> + *		pchan->chan.rx_alloc is set, then the command complete
> + *		flag is checked and the data is written to the shared
> + *		buffer io memory.
> + *
> + *		If pchan->chan.rx_alloc is not set, then it is used
>    *		here only to ring the channel doorbell. The PCC client
>    *		specific read/write is done in the client driver in
>    *		order to maintain atomicity over PCC channel once
> @@ -434,17 +507,37 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
>   	int ret;
>   	struct pcc_chan_info *pchan = chan->con_priv;
>   
> +	ret = pcc_write_to_buffer(chan, data);
> +	if (ret)
> +		return ret;
> +
>   	ret = pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>   	if (ret)
>   		return ret;
>   
>   	ret = pcc_chan_reg_read_modify_write(&pchan->db);
> +
>   	if (!ret && pchan->plat_irq > 0)
>   		pchan->chan_in_use = true;
>   
>   	return ret;
>   }
>   
> +
> +static bool pcc_last_tx_done(struct mbox_chan *chan)
> +{
> +	struct pcc_chan_info *pchan = chan->con_priv;
> +	u64 val;
> +
> +	pcc_chan_reg_read(&pchan->cmd_complete, &val);
> +	if (!val)
> +		return false;
> +	else
> +		return true;
> +}
> +
> +
> +
>   /**
>    * pcc_startup - Called from Mailbox Controller code. Used here
>    *		to request the interrupt.
> @@ -490,6 +583,7 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>   	.send_data = pcc_send_data,
>   	.startup = pcc_startup,
>   	.shutdown = pcc_shutdown,
> +	.last_tx_done = pcc_last_tx_done,
>   };
>   
>   /**
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 840bfc95bae3..9af3b502f839 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -17,6 +17,35 @@ 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 {
> +	u32 signature;
> +	u32 flags;
> +	u32 length;
> +	u32 command;
>   };

Fairly certain these should not be explicitly little endian IAW the 
spec. It has been a source of discussion in the past.


>   
>   /* Generic Communications Channel Shared Memory Region */

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Adam Young]

Is there some reason that this patch is not showing up in the 
maintainers queues for review?

On 7/22/25 13:10, Adam Young wrote:
>
> On 7/14/25 20:10, admiyo@os.amperecomputing.com wrote:
>> From: Adam Young <admiyo@os.amperecomputing.com>
>>
>> Define a new, optional, callback that allows the driver to
>> specify how the return data buffer is allocated.  If that callback
>> is set,  mailbox/pcc.c is now responsible for reading from and
>> writing to the PCC shared buffer.
>>
>> This also allows for proper checks of the Commnand complete flag
>> between the PCC sender and receiver.
>>
>> For Type 4 channels, initialize the command complete flag prior
>> to accepting messages.
>>
>> Since the mailbox does not know what memory allocation scheme
>> to use for response messages, the client now has an optional
>> callback that allows it to allocate the buffer for a response
>> message.
>>
>> When an outbound message is written to the buffer, the mailbox
>> checks for the flag indicating the client wants an tx complete
>> notification via IRQ.  Upon receipt of the interrupt It will
>> pair it with the outgoing message. The expected use is to
>> free the kernel memory buffer for the previous outgoing message.
>>
>> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
>> ---
>>   drivers/mailbox/pcc.c | 102 ++++++++++++++++++++++++++++++++++++++++--
>>   include/acpi/pcc.h    |  29 ++++++++++++
>>   2 files changed, 127 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index f6714c233f5a..0a00719b2482 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -306,6 +306,22 @@ static void pcc_chan_acknowledge(struct 
>> pcc_chan_info *pchan)
>>           pcc_chan_reg_read_modify_write(&pchan->db);
>>   }
>>   +static void *write_response(struct pcc_chan_info *pchan)
>> +{
>> +    struct pcc_header pcc_header;
>> +    void *buffer;
>> +    int data_len;
>> +
>> +    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);
>> +    if (buffer != NULL)
>> +        memcpy_fromio(buffer, pchan->chan.shmem, data_len);
>> +    return buffer;
>> +}
>> +
>>   /**
>>    * pcc_mbox_irq - PCC mailbox interrupt handler
>>    * @irq:    interrupt number
>> @@ -317,6 +333,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>   {
>>       struct pcc_chan_info *pchan;
>>       struct mbox_chan *chan = p;
>> +    struct pcc_header *pcc_header = chan->active_req;
>
> OK, so it looks a little strange to re-initialize this later. Would it 
> be better to not have it initialized?
>
>
>> +    void *handle = NULL;
>>         pchan = chan->con_priv;
>>   @@ -340,7 +358,17 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>        * required to avoid any possible race in updatation of this flag.
>>        */
>>       pchan->chan_in_use = false;
>> -    mbox_chan_received_data(chan, NULL);
>> +
>> +    if (pchan->chan.rx_alloc)
>> +        handle = write_response(pchan);
>> +
>> +    if (chan->active_req) {
>> +        pcc_header = chan->active_req;
>> +        if (pcc_header->flags & PCC_CMD_COMPLETION_NOTIFY)
>
> Note that this is the counterpoint to my earlier patch that only 
> notifies the platform if the platform requests it.  This is part of 
> the specification.
>
>> +            mbox_chan_txdone(chan, 0);
>> +    }
>> +
>> +    mbox_chan_received_data(chan, handle);
>>         pcc_chan_acknowledge(pchan);
>>   @@ -384,9 +412,24 @@ pcc_mbox_request_channel(struct mbox_client 
>> *cl, int subspace_id)
>>       pcc_mchan = &pchan->chan;
>>       pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
>>                          pcc_mchan->shmem_size);
>> -    if (pcc_mchan->shmem)
>> -        return pcc_mchan;
>> +    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
>> +     * the mailbox API. That implies that the mailbox client must
>> +     * have set the allocate callback function prior to
>> +     * sending any messages.
>> +     */
>> +    if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
>> + pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> Is there a better  way to do this?  The flag is not accessable from 
> the driver.
>> +
>> +    return pcc_mchan;
>>   +err:
>>       mbox_free_channel(chan);
>>       return ERR_PTR(-ENXIO);
>>   }
>> @@ -417,8 +460,38 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan 
>> *pchan)
>>   }
>>   EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>>   +static int pcc_write_to_buffer(struct mbox_chan *chan, void *data)
>> +{
>> +    struct pcc_chan_info *pchan = chan->con_priv;
>> +    struct pcc_mbox_chan *pcc_mbox_chan = &pchan->chan;
>> +    struct pcc_header *pcc_header = data;
>> +
>> +    if (!pchan->chan.manage_writes)
>> +        return 0;
>> +
>> +    /* The PCC header length includes the command field
>> +     * but not the other values from the header.
>> +     */
>> +    int len = pcc_header->length - sizeof(u32) + sizeof(struct 
>> pcc_header);
>> +    u64 val;
>> +
>> +    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);
>> +    return 0;
>> +}
>> +
>
> I think this is the pattern that we  want all of the PCC mailbox 
> clients to migrate to.  Is there any reason it was not implmented this 
> way originally?-
>
>> +
>>   /**
>> - * pcc_send_data - Called from Mailbox Controller code. Used
>> + * pcc_send_data - Called from Mailbox Controller code. If
>> + *        pchan->chan.rx_alloc is set, then the command complete
>> + *        flag is checked and the data is written to the shared
>> + *        buffer io memory.
>> + *
>> + *        If pchan->chan.rx_alloc is not set, then it is used
>>    *        here only to ring the channel doorbell. The PCC client
>>    *        specific read/write is done in the client driver in
>>    *        order to maintain atomicity over PCC channel once
>> @@ -434,17 +507,37 @@ static int pcc_send_data(struct mbox_chan 
>> *chan, void *data)
>>       int ret;
>>       struct pcc_chan_info *pchan = chan->con_priv;
>>   +    ret = pcc_write_to_buffer(chan, data);
>> +    if (ret)
>> +        return ret;
>> +
>>       ret = pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>>       if (ret)
>>           return ret;
>>         ret = pcc_chan_reg_read_modify_write(&pchan->db);
>> +
>>       if (!ret && pchan->plat_irq > 0)
>>           pchan->chan_in_use = true;
>>         return ret;
>>   }
>>   +
>> +static bool pcc_last_tx_done(struct mbox_chan *chan)
>> +{
>> +    struct pcc_chan_info *pchan = chan->con_priv;
>> +    u64 val;
>> +
>> +    pcc_chan_reg_read(&pchan->cmd_complete, &val);
>> +    if (!val)
>> +        return false;
>> +    else
>> +        return true;
>> +}
>> +
>> +
>> +
>>   /**
>>    * pcc_startup - Called from Mailbox Controller code. Used here
>>    *        to request the interrupt.
>> @@ -490,6 +583,7 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>>       .send_data = pcc_send_data,
>>       .startup = pcc_startup,
>>       .shutdown = pcc_shutdown,
>> +    .last_tx_done = pcc_last_tx_done,
>>   };
>>     /**
>> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
>> index 840bfc95bae3..9af3b502f839 100644
>> --- a/include/acpi/pcc.h
>> +++ b/include/acpi/pcc.h
>> @@ -17,6 +17,35 @@ 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 {
>> +    u32 signature;
>> +    u32 flags;
>> +    u32 length;
>> +    u32 command;
>>   };
>
> Fairly certain these should not be explicitly little endian IAW the 
> spec. It has been a source of discussion in the past.
>
>
>>     /* Generic Communications Channel Shared Memory Region */

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Adam Young]

On 7/31/25 16:42, Jassi Brar wrote:
>
>
> On Thu, Jul 31, 2025, 3:35 PM Adam Young 
> <admiyo@amperemail.onmicrosoft.com> wrote:
>
>     Is there some reason that this patch is not showing up in the
>     maintainers queues for review?
>
>
> From your last reply to yourself it seems like you were to make some 
> changes.
> And there wasn't any acked by ( though that can be overridden by 
> enough public exposure)


Thanks, Jassi,

I just know that with the last set of changes on the Mailbox 
implementation there was a bit of back and forth, and I expected the 
same with this one.  I am not making any significant changes to that 
layer, just nits.

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Sudeep Holla]

On Mon, Jul 14, 2025 at 08:10:07PM -0400, admiyo@os.amperecomputing.com wrote:
> From: Adam Young <admiyo@os.amperecomputing.com>
> 
> Define a new, optional, callback that allows the driver to
> specify how the return data buffer is allocated.  If that callback
> is set,  mailbox/pcc.c is now responsible for reading from and
> writing to the PCC shared buffer.
> 
> This also allows for proper checks of the Commnand complete flag
> between the PCC sender and receiver.
> 
> For Type 4 channels, initialize the command complete flag prior
> to accepting messages.
> 
> Since the mailbox does not know what memory allocation scheme
> to use for response messages, the client now has an optional
> callback that allows it to allocate the buffer for a response
> message.
> 
> When an outbound message is written to the buffer, the mailbox
> checks for the flag indicating the client wants an tx complete
> notification via IRQ.  Upon receipt of the interrupt It will
> pair it with the outgoing message. The expected use is to
> free the kernel memory buffer for the previous outgoing message.
>

I know this is merged. Based on the discussions here, I may send a revert
to this as I don't think it is correct.

> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
>  drivers/mailbox/pcc.c | 102 ++++++++++++++++++++++++++++++++++++++++--
>  include/acpi/pcc.h    |  29 ++++++++++++
>  2 files changed, 127 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index f6714c233f5a..0a00719b2482 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -306,6 +306,22 @@ static void pcc_chan_acknowledge(struct pcc_chan_info *pchan)
>  		pcc_chan_reg_read_modify_write(&pchan->db);
>  }
>  
> +static void *write_response(struct pcc_chan_info *pchan)
> +{
> +	struct pcc_header pcc_header;
> +	void *buffer;
> +	int data_len;
> +
> +	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);
> +	if (buffer != NULL)
> +		memcpy_fromio(buffer, pchan->chan.shmem, data_len);
> +	return buffer;
> +}
> +
>  /**
>   * pcc_mbox_irq - PCC mailbox interrupt handler
>   * @irq:	interrupt number
> @@ -317,6 +333,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>  {
>  	struct pcc_chan_info *pchan;
>  	struct mbox_chan *chan = p;
> +	struct pcc_header *pcc_header = chan->active_req;
> +	void *handle = NULL;
>  
>  	pchan = chan->con_priv;
>  
> @@ -340,7 +358,17 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>  	 * required to avoid any possible race in updatation of this flag.
>  	 */
>  	pchan->chan_in_use = false;
> -	mbox_chan_received_data(chan, NULL);
> +
> +	if (pchan->chan.rx_alloc)
> +		handle = write_response(pchan);
> +
> +	if (chan->active_req) {
> +		pcc_header = chan->active_req;
> +		if (pcc_header->flags & PCC_CMD_COMPLETION_NOTIFY)
> +			mbox_chan_txdone(chan, 0);
> +	}
> +
> +	mbox_chan_received_data(chan, handle);
>  
>  	pcc_chan_acknowledge(pchan);
>  
> @@ -384,9 +412,24 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>  	pcc_mchan = &pchan->chan;
>  	pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
>  					   pcc_mchan->shmem_size);
> -	if (pcc_mchan->shmem)
> -		return pcc_mchan;
> +	if (!pcc_mchan->shmem)
> +		goto err;
> +
> +	pcc_mchan->manage_writes = false;
> +

Who will change this value as it is fixed to false always.
That makes the whole pcc_write_to_buffer() reduntant. It must go away.
Also why can't you use tx_prepare callback here. I don't like these changes
at all as I find these redundant. Sorry for not reviewing it in time.
I was totally confused with your versioning and didn't spot the mailbox/pcc
changes in between and assumed it is just MCTP net driver changes. My mistake.

> +	/* 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
> +	 * the mailbox API. That implies that the mailbox client must
> +	 * have set the allocate callback function prior to
> +	 * sending any messages.
> +	 */
> +	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> +		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> +
> +	return pcc_mchan;
>  
> +err:
>  	mbox_free_channel(chan);
>  	return ERR_PTR(-ENXIO);
>  }
> @@ -417,8 +460,38 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>  }
>  EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>  
> +static int pcc_write_to_buffer(struct mbox_chan *chan, void *data)
> +{
> +	struct pcc_chan_info *pchan = chan->con_priv;
> +	struct pcc_mbox_chan *pcc_mbox_chan = &pchan->chan;
> +	struct pcc_header *pcc_header = data;
> +
> +	if (!pchan->chan.manage_writes)
> +		return 0;
> +
> +	/* The PCC header length includes the command field
> +	 * but not the other values from the header.
> +	 */
> +	int len = pcc_header->length - sizeof(u32) + sizeof(struct pcc_header);
> +	u64 val;
> +
> +	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);
> +	return 0;
> +}
> +
> +
>  /**
> - * pcc_send_data - Called from Mailbox Controller code. Used
> + * pcc_send_data - Called from Mailbox Controller code. If
> + *		pchan->chan.rx_alloc is set, then the command complete
> + *		flag is checked and the data is written to the shared
> + *		buffer io memory.
> + *
> + *		If pchan->chan.rx_alloc is not set, then it is used
>   *		here only to ring the channel doorbell. The PCC client
>   *		specific read/write is done in the client driver in
>   *		order to maintain atomicity over PCC channel once
> @@ -434,17 +507,37 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
>  	int ret;
>  	struct pcc_chan_info *pchan = chan->con_priv;
>  
> +	ret = pcc_write_to_buffer(chan, data);
> +	if (ret)
> +		return ret;
> +

Completely null as manages_write is false always.

>  	ret = pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>  	if (ret)
>  		return ret;
>  
>  	ret = pcc_chan_reg_read_modify_write(&pchan->db);
> +
>  	if (!ret && pchan->plat_irq > 0)
>  		pchan->chan_in_use = true;
>  
>  	return ret;
>  }
>  
> +
> +static bool pcc_last_tx_done(struct mbox_chan *chan)
> +{
> +	struct pcc_chan_info *pchan = chan->con_priv;
> +	u64 val;
> +
> +	pcc_chan_reg_read(&pchan->cmd_complete, &val);

Not checking return from pcc_chan_reg_read(). Be consistent with the
other code in the file.

> +	if (!val)
> +		return false;
> +	else
> +		return true;
> +}
> +
> +
> +
>  /**
>   * pcc_startup - Called from Mailbox Controller code. Used here
>   *		to request the interrupt.
> @@ -490,6 +583,7 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>  	.send_data = pcc_send_data,
>  	.startup = pcc_startup,
>  	.shutdown = pcc_shutdown,
> +	.last_tx_done = pcc_last_tx_done,
>  };
>  
>  /**
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 840bfc95bae3..9af3b502f839 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -17,6 +17,35 @@ 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);

Why this can't be in rx_callback ?


I am convinced to send a revert, please respond so that I can understand
the requirements better.

-- 
Regards,
Sudeep

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Adam Young]

Answers inline.

On 9/4/25 07:00, Sudeep Holla wrote:
> On Mon, Jul 14, 2025 at 08:10:07PM -0400, admiyo@os.amperecomputing.com wrote:
>> From: Adam Young <admiyo@os.amperecomputing.com>
>>
>> Define a new, optional, callback that allows the driver to
>> specify how the return data buffer is allocated.  If that callback
>> is set,  mailbox/pcc.c is now responsible for reading from and
>> writing to the PCC shared buffer.
>>
>> This also allows for proper checks of the Commnand complete flag
>> between the PCC sender and receiver.
>>
>> For Type 4 channels, initialize the command complete flag prior
>> to accepting messages.
>>
>> Since the mailbox does not know what memory allocation scheme
>> to use for response messages, the client now has an optional
>> callback that allows it to allocate the buffer for a response
>> message.
>>
>> When an outbound message is written to the buffer, the mailbox
>> checks for the flag indicating the client wants an tx complete
>> notification via IRQ.  Upon receipt of the interrupt It will
>> pair it with the outgoing message. The expected use is to
>> free the kernel memory buffer for the previous outgoing message.
>>
> I know this is merged. Based on the discussions here, I may send a revert
> to this as I don't think it is correct.
>
>> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
>> ---
>>   drivers/mailbox/pcc.c | 102 ++++++++++++++++++++++++++++++++++++++++--
>>   include/acpi/pcc.h    |  29 ++++++++++++
>>   2 files changed, 127 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index f6714c233f5a..0a00719b2482 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -306,6 +306,22 @@ static void pcc_chan_acknowledge(struct pcc_chan_info *pchan)
>>   		pcc_chan_reg_read_modify_write(&pchan->db);
>>   }
>>   
>> +static void *write_response(struct pcc_chan_info *pchan)
>> +{
>> +	struct pcc_header pcc_header;
>> +	void *buffer;
>> +	int data_len;
>> +
>> +	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);
>> +	if (buffer != NULL)
>> +		memcpy_fromio(buffer, pchan->chan.shmem, data_len);
>> +	return buffer;
>> +}
>> +
>>   /**
>>    * pcc_mbox_irq - PCC mailbox interrupt handler
>>    * @irq:	interrupt number
>> @@ -317,6 +333,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>   {
>>   	struct pcc_chan_info *pchan;
>>   	struct mbox_chan *chan = p;
>> +	struct pcc_header *pcc_header = chan->active_req;
>> +	void *handle = NULL;
>>   
>>   	pchan = chan->con_priv;
>>   
>> @@ -340,7 +358,17 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>   	 * required to avoid any possible race in updatation of this flag.
>>   	 */
>>   	pchan->chan_in_use = false;
>> -	mbox_chan_received_data(chan, NULL);
>> +
>> +	if (pchan->chan.rx_alloc)
>> +		handle = write_response(pchan);
>> +
>> +	if (chan->active_req) {
>> +		pcc_header = chan->active_req;
>> +		if (pcc_header->flags & PCC_CMD_COMPLETION_NOTIFY)
>> +			mbox_chan_txdone(chan, 0);
>> +	}
>> +
>> +	mbox_chan_received_data(chan, handle);
>>   
>>   	pcc_chan_acknowledge(pchan);
>>   
>> @@ -384,9 +412,24 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>>   	pcc_mchan = &pchan->chan;
>>   	pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
>>   					   pcc_mchan->shmem_size);
>> -	if (pcc_mchan->shmem)
>> -		return pcc_mchan;
>> +	if (!pcc_mchan->shmem)
>> +		goto err;
>> +
>> +	pcc_mchan->manage_writes = false;
>> +
> Who will change this value as it is fixed to false always.
> That makes the whole pcc_write_to_buffer() reduntant. It must go away.
> Also why can't you use tx_prepare callback here. I don't like these changes
> at all as I find these redundant. Sorry for not reviewing it in time.
> I was totally confused with your versioning and didn't spot the mailbox/pcc
> changes in between and assumed it is just MCTP net driver changes. My mistake.

This was a case of leaving the default as is to not-break the existing 
mailbox clients.

The maibox client can over ride it in its driver setup.



>
>> +	/* 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
>> +	 * the mailbox API. That implies that the mailbox client must
>> +	 * have set the allocate callback function prior to
>> +	 * sending any messages.
>> +	 */
>> +	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
>> +		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>> +
>> +	return pcc_mchan;
>>   
>> +err:
>>   	mbox_free_channel(chan);
>>   	return ERR_PTR(-ENXIO);
>>   }
>> @@ -417,8 +460,38 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>>   }
>>   EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>>   
>> +static int pcc_write_to_buffer(struct mbox_chan *chan, void *data)
>> +{
>> +	struct pcc_chan_info *pchan = chan->con_priv;
>> +	struct pcc_mbox_chan *pcc_mbox_chan = &pchan->chan;
>> +	struct pcc_header *pcc_header = data;
>> +
>> +	if (!pchan->chan.manage_writes)
>> +		return 0;
>> +
>> +	/* The PCC header length includes the command field
>> +	 * but not the other values from the header.
>> +	 */
>> +	int len = pcc_header->length - sizeof(u32) + sizeof(struct pcc_header);
>> +	u64 val;
>> +
>> +	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);
>> +	return 0;
>> +}
>> +
>> +
>>   /**
>> - * pcc_send_data - Called from Mailbox Controller code. Used
>> + * pcc_send_data - Called from Mailbox Controller code. If
>> + *		pchan->chan.rx_alloc is set, then the command complete
>> + *		flag is checked and the data is written to the shared
>> + *		buffer io memory.
>> + *
>> + *		If pchan->chan.rx_alloc is not set, then it is used
>>    *		here only to ring the channel doorbell. The PCC client
>>    *		specific read/write is done in the client driver in
>>    *		order to maintain atomicity over PCC channel once
>> @@ -434,17 +507,37 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
>>   	int ret;
>>   	struct pcc_chan_info *pchan = chan->con_priv;
>>   
>> +	ret = pcc_write_to_buffer(chan, data);
>> +	if (ret)
>> +		return ret;
>> +
> Completely null as manages_write is false always.
Not if re-set by the client.
>
>>   	ret = pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>>   	if (ret)
>>   		return ret;
>>   
>>   	ret = pcc_chan_reg_read_modify_write(&pchan->db);
>> +
>>   	if (!ret && pchan->plat_irq > 0)
>>   		pchan->chan_in_use = true;
>>   
>>   	return ret;
>>   }
>>   
>> +
>> +static bool pcc_last_tx_done(struct mbox_chan *chan)
>> +{
>> +	struct pcc_chan_info *pchan = chan->con_priv;
>> +	u64 val;
>> +
>> +	pcc_chan_reg_read(&pchan->cmd_complete, &val);
> Not checking return from pcc_chan_reg_read(). Be consistent with the
> other code in the file.
OK, this is legit.
>
>> +	if (!val)
>> +		return false;
>> +	else
>> +		return true;
>> +}
>> +
>> +
>> +
>>   /**
>>    * pcc_startup - Called from Mailbox Controller code. Used here
>>    *		to request the interrupt.
>> @@ -490,6 +583,7 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>>   	.send_data = pcc_send_data,
>>   	.startup = pcc_startup,
>>   	.shutdown = pcc_shutdown,
>> +	.last_tx_done = pcc_last_tx_done,
>>   };
>>   
>>   /**
>> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
>> index 840bfc95bae3..9af3b502f839 100644
>> --- a/include/acpi/pcc.h
>> +++ b/include/acpi/pcc.h
>> @@ -17,6 +17,35 @@ 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);
> Why this can't be in rx_callback ?

Because that is too late.

The problem is that the client needs  to allocate the memory that the 
message comes in in order to hand it off.

In the case of a network device, the rx_alloc code is going to return 
the memory are of a struct sk_buff. The Mailbox does not know how to 
allocate this. If the driver just kmallocs memory for the return 
message, we would have a re-copy of the message.

This is really a mailbox-api level issue, but I was trying to limit the 
scope of my changes as much as possible.

The PCC mailbox code really does not match the abstractions of the 
mailbox in general.  The idea that copying into and out of the buffer is 
done by each individual driver leads to a lot of duplicated code.  With 
this change, most of the other drivers could now be re-written to let 
the mailbox manage the copying, while letting the mailbox client specify 
only how to allocate the message buffers.


Much of this change  was driven by the fact that the PCC mailbox does 
not properly check the flags before allowing writes to the rx channel, 
and that code is not exposed to the driver.  Thus, it was impossible to 
write everything in the rx callback regardless. This work was based on 
Huisong's comments on version 21 of the patch series.

>
>
> I am convinced to send a revert, please respond so that I can understand
> the requirements better.

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Sudeep Holla]

On Thu, Sep 04, 2025 at 01:06:09PM -0400, Adam Young wrote:
> Answers inline.
> 
> On 9/4/25 07:00, Sudeep Holla wrote:

[...]

> > Who will change this value as it is fixed to false always.
> > That makes the whole pcc_write_to_buffer() reduntant. It must go away.
> > Also why can't you use tx_prepare callback here. I don't like these changes
> > at all as I find these redundant. Sorry for not reviewing it in time.
> > I was totally confused with your versioning and didn't spot the mailbox/pcc
> > changes in between and assumed it is just MCTP net driver changes. My mistake.
> 
> This was a case of leaving the default as is to not-break the existing
> mailbox clients.
> 
> The maibox client can over ride it in its driver setup.
> 

What if driver changes in the middle of an ongoing transaction ? That
doesn't sound like a good idea to me.

You didn't respond as why tx_prepare callback can be used to do exactly
same thing ?

> > > +	void *(*rx_alloc)(struct mbox_client *cl,  int size);
> > Why this can't be in rx_callback ?
> 
> Because that is too late.
> 
> The problem is that the client needs  to allocate the memory that the
> message comes in in order to hand it off.
> 
> In the case of a network device, the rx_alloc code is going to return the
> memory are of a struct sk_buff. The Mailbox does not know how to allocate
> this. If the driver just kmallocs memory for the return message, we would
> have a re-copy of the message.
>

I still don't understand the requirement. The PCC users has access to shmem
and can do what they want in rx_callback, so I don't see any reason for
this API.

> This is really a mailbox-api level issue, but I was trying to limit the
> scope of my changes as much as possible.
> 

Please explain the issue. Sorry if I have missed, pointer are enough if
already present in some mail thread.

> The PCC mailbox code really does not match the abstractions of the mailbox
> in general.  The idea that copying into and out of the buffer is done by
> each individual driver leads to a lot of duplicated code.  With this change,
> most of the other drivers could now be re-written to let the mailbox manage
> the copying, while letting the mailbox client specify only how to allocate
> the message buffers.
> 

Yes that's because each user have their own requirement. You can do what
you want in rx_callback.

> Much of this change  was driven by the fact that the PCC mailbox does not
> properly check the flags before allowing writes to the rx channel, and that
> code is not exposed to the driver.  Thus, it was impossible to write
> everything in the rx callback regardless. This work was based on Huisong's
> comments on version 21 of the patch series.
> 

Pointers please, sorry again. But I really don't like the merged code and
looking for ways to clean it up as well as address the requirement if it
is not available esp. if we have to revert this change.

-- 
Regards,
Sudeep

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Adam Young]

On 9/5/25 10:37, Sudeep Holla wrote:
> On Thu, Sep 04, 2025 at 01:06:09PM -0400, Adam Young wrote:
>> Answers inline.
>>
>> On 9/4/25 07:00, Sudeep Holla wrote:
> [...]
>
>>> Who will change this value as it is fixed to false always.
>>> That makes the whole pcc_write_to_buffer() reduntant. It must go away.
>>> Also why can't you use tx_prepare callback here. I don't like these changes
>>> at all as I find these redundant. Sorry for not reviewing it in time.
>>> I was totally confused with your versioning and didn't spot the mailbox/pcc
>>> changes in between and assumed it is just MCTP net driver changes. My mistake.
>> This was a case of leaving the default as is to not-break the existing
>> mailbox clients.
>>
>> The maibox client can over ride it in its driver setup.
>>
> What if driver changes in the middle of an ongoing transaction ? That
> doesn't sound like a good idea to me.
It would not be a good idea.  This should be setup only.  Is there a 
cleaner way to pass an initialization value like this in the mailbox API?
>
> You didn't respond as why tx_prepare callback can be used to do exactly
> same thing ?

Note that write_to_buffer checks pcc_chan_reg_read(&pchan->cmd_complete, &val);  This flag comes from struct pcc_chan_info which is defined in the pcc.c, not in a header.  Thus it is not accessible outside of the mailbox driver.  This needs to be done before data is written into the buffer, or there is a chance the far side is still reading it.  Checking it before send_data leads to a race condition.

>
>>>> +	void *(*rx_alloc)(struct mbox_client *cl,  int size);
>>> Why this can't be in rx_callback ?
>> Because that is too late.
>>
>> The problem is that the client needs  to allocate the memory that the
>> message comes in in order to hand it off.
>>
>> In the case of a network device, the rx_alloc code is going to return the
>> memory are of a struct sk_buff. The Mailbox does not know how to allocate
>> this. If the driver just kmallocs memory for the return message, we would
>> have a re-copy of the message.
>>
> I still don't understand the requirement. The PCC users has access to shmem
> and can do what they want in rx_callback, so I don't see any reason for
> this API.

I was not around for the discussion where it was decided to diverge from 
the mailbox abstraction, and provide direct access to the buffer.  I 
assume it was due to the desire not to copy into temporary-allocated 
memory.  However, the pcc mailbox driver should be handling the access 
into and out of the buffer, and not each individual driver.  Looking at 
most of them, there is very little type 4 usage where you are getting 
significant amounts of data back in the buffer, most of them are using 
rx for notification only, not transfer of data into the buffer.  Every 
single driver that will need to copy data from the shmem into a kernel 
memory buffer will have to do the exact same logic, and you can see it 
is non-trivial and worth getting right in the mailbox driver.


>
>> This is really a mailbox-api level issue, but I was trying to limit the
>> scope of my changes as much as possible.
>>
> Please explain the issue. Sorry if I have missed, pointer are enough if
> already present in some mail thread.

I don't think it is in any mail-thread: I expected the conversation to 
happen once I submitted this patch.  I did attempt to clarify my 
reasoning as much as possible in the commit message.  But the patch got 
merged with no discussion, and without a maintainer ACK.

The issue is that the mailbox driver is generic, and the actual use of 
it (MCTP-PCC in my case) is bound to other kernel subsystems, that need 
to manage memory in various ways.  The mailbox driver should handle the 
protocol (PCC) but not the memory management.

Thus, the rights solution would be to provide this callback function at 
the mailbox API level, much like the tx_prepare functions.  However, 
doing that would only change where he pcc mailbox driver finds the 
function, and not the logic.  Thus, I limited the change to only the PCC 
subsystem:  I did not want to have to sell this to the entire mailbox 
community without some prior art to point to.


>
>> The PCC mailbox code really does not match the abstractions of the mailbox
>> in general.  The idea that copying into and out of the buffer is done by
>> each individual driver leads to a lot of duplicated code.  With this change,
>> most of the other drivers could now be re-written to let the mailbox manage
>> the copying, while letting the mailbox client specify only how to allocate
>> the message buffers.
>>
> Yes that's because each user have their own requirement. You can do what
> you want in rx_callback.

Each driver needs to do the same thing: reading the header from 
iomemory, checking the length, allocating a buffer, and then reading the 
whole message from iomemory and setting the flag that says the buffer 
read is complete.  This is a way to get that logic right once, and  
reuse for any PCC driver that receives data in the buffer.


>
>> Much of this change  was driven by the fact that the PCC mailbox does not
>> properly check the flags before allowing writes to the rx channel, and that
>> code is not exposed to the driver.  Thus, it was impossible to write
>> everything in the rx callback regardless. This work was based on Huisong's
>> comments on version 21 of the patch series.
>>
> Pointers please, sorry again. But I really don't like the merged code and
> looking for ways to clean it up as well as address the requirement if it
> is not available esp. if we have to revert this change.

In retrospect, this patch made two changes, one which was completely 
required (access to the flag before writing to the buffer) and one which 
is a don't-repeat-yourself implementation.

If needs be, I can work around the changes to the RX  path. But I cannot 
work around the changes  in the TX path: it will lead to a race condition.

Making the RX change makes both paths equivalent, and will lead to 
cleaner PCC clients in the future.

And I can imagine your shock in seeing this patch post-merge.  I did 
send email out to you directly, but I realize now it must have gotten 
lost in the noise.  I really wish we had this discussion prior to 
merge.  However, I hope you will consider not reverting it.  I wrote it 
to be backwards compatible with existing mailbox clients, and to only 
take the new path upon explicit enabling.  I am more than happy to 
consider better ways to enable the features.

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Adam Young]

On 9/5/25 12:57, Adam Young wrote:
>>>> Who will change this value as it is fixed to false always.
>>>> That makes the whole pcc_write_to_buffer() reduntant. It must go away.
>>>> Also why can't you use tx_prepare callback here. I don't like these 
>>>> changes
>>>> at all as I find these redundant. Sorry for not reviewing it in time.
>>>> I was totally confused with your versioning and didn't spot the 
>>>> mailbox/pcc
>>>> changes in between and assumed it is just MCTP net driver changes. 
>>>> My mistake.
>>> This was a case of leaving the default as is to not-break the existing
>>> mailbox clients.
>>>
>>> The maibox client can over ride it in its driver setup.
>>>
>> What if driver changes in the middle of an ongoing transaction ? That
>> doesn't sound like a good idea to me.
> It would not be a good idea.  This should be setup only.  Is there a 
> cleaner way to pass an initialization value like this in the mailbox API? 

My initial idea was that we should use the mssg pointer to dictate 
whether or not the mailbox should attempt the write.  If the client 
passes in a NULL pointer (and they all should, with the exception of new 
ones) then there is nothing to try to write.

But at lease one of the clients seem to set the message, and I don't 
think there is any really good reason for it.

These are  the drivers that explicitly call pcc_mbox_request_channel.  
There might be other drivers that use the mailbox and request the 
channel through the mailbox API, but lets start with these

drivers/acpi/cppc_acpi.c

drivers/hwmon/xgene-hwmon.c

drivers/i2c/busses/i2c-xgene-slimpro.c

drivers/soc/hisilicon/kunpeng_hccs.c

drivers/devfreq/hisi_uncore_freq.c


For example, the last driver calls:          rc = 
mbox_send_message(pchan->mchan, &cmd);

There is actually no reason to assume that the doorbell will be rung at 
that point:  the cmd object is not null, and thus gets added to the 
ring_buffer.  They then have to poll.  The poll may timeout, but the cmd 
pointer may still be in the ring buffer, and get sent on the next 
message.  But there is no benefit to sending the cmd object here, as the 
ring buffer pointer is not read.

slimpro_i2c_send_msg same thing.  The message is a 32bit value. None of 
these calls actually make use of  the PCC buffer: there is no PCC header 
written etc.  You could argue they don't actually meet the protocol 
definition.

Here is drivers/acpi/cppc_acpi.c

         /* Flip CMD COMPLETE bit */
         writew_relaxed(0, &generic_comm_base->status);

         pcc_ss_data->platform_owns_pcc = true;

         /* Ring doorbell */
         ret = mbox_send_message(pcc_ss_data->pcc_channel->mchan, &cmd);

If, instead, these drivers did

         ret = mbox_send_message(pcc_ss_data->pcc_channel->mchan,  NULL);

You would have proper functioning, and the void * mssg parameter could 
be used for the drivers that actually need it.

All of these drivers would have a simpler code path if they called the 
function pcc_send_data directly, without putting values on the ring 
buffer.  That was how I originally wrote my driver, but I actually want 
to make use of  the mailbox abstraction, and can use the ring buffer as 
rate limiter etc.

So, instead of going an changing the other drivers, I provided a default 
that left the existing behavior alone, and only performs the 
mailbox-assisted-write if the flag is set.

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Adam Young]

On 9/4/25 07:00, Sudeep Holla wrote:
> On Mon, Jul 14, 2025 at 08:10:07PM -0400,admiyo@os.amperecomputing.com wrote:
>> From: Adam Young<admiyo@os.amperecomputing.com>
>>
>> Define a new, optional, callback that allows the driver to
>> specify how the return data buffer is allocated.  If that callback
>> is set,  mailbox/pcc.c is now responsible for reading from and
>> writing to the PCC shared buffer.
>>
>> This also allows for proper checks of the Commnand complete flag
>> between the PCC sender and receiver.
>>
>> For Type 4 channels, initialize the command complete flag prior
>> to accepting messages.
>>
>> Since the mailbox does not know what memory allocation scheme
>> to use for response messages, the client now has an optional
>> callback that allows it to allocate the buffer for a response
>> message.
>>
>> When an outbound message is written to the buffer, the mailbox
>> checks for the flag indicating the client wants an tx complete
>> notification via IRQ.  Upon receipt of the interrupt It will
>> pair it with the outgoing message. The expected use is to
>> free the kernel memory buffer for the previous outgoing message.
>>
> I know this is merged. Based on the discussions here, I may send a revert
> to this as I don't think it is correct.

Have you decided what to do?  The MCTP over PCC driver depends on the 
behavior in this patch. If you do revert, I will need a path forward.

Based on other code review feed back, I need to make an additional 
change:  the rx_alloc callback function needs to be atomically set, and 
thus needs to move to the mailbox API.  There it will pair with the 
prepare transaction function.  It is a small change, but I expect some 
feedback from the mailbox maintainers.

I know all of the other drivers that use the PCC mailbox currently do 
direct management of the shared buffer.  I suspect that is the biggest 
change that is causing you concern.  Are you OK with maintaining a 
mailbox-managed path to buffer management as well?  I think it will be 
beneficial to other drivers in the long run.

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Sudeep Holla]

On Mon, Sep 08, 2025 at 10:58:55AM -0400, Adam Young wrote:
> 
> On 9/4/25 07:00, Sudeep Holla wrote:
> > On Mon, Jul 14, 2025 at 08:10:07PM -0400,admiyo@os.amperecomputing.com wrote:
> > > From: Adam Young<admiyo@os.amperecomputing.com>
> > > 
> > > Define a new, optional, callback that allows the driver to
> > > specify how the return data buffer is allocated.  If that callback
> > > is set,  mailbox/pcc.c is now responsible for reading from and
> > > writing to the PCC shared buffer.
> > > 
> > > This also allows for proper checks of the Commnand complete flag
> > > between the PCC sender and receiver.
> > > 
> > > For Type 4 channels, initialize the command complete flag prior
> > > to accepting messages.
> > > 
> > > Since the mailbox does not know what memory allocation scheme
> > > to use for response messages, the client now has an optional
> > > callback that allows it to allocate the buffer for a response
> > > message.
> > > 
> > > When an outbound message is written to the buffer, the mailbox
> > > checks for the flag indicating the client wants an tx complete
> > > notification via IRQ.  Upon receipt of the interrupt It will
> > > pair it with the outgoing message. The expected use is to
> > > free the kernel memory buffer for the previous outgoing message.
> > > 
> > I know this is merged. Based on the discussions here, I may send a revert
> > to this as I don't think it is correct.
> 
> Have you decided what to do?  The MCTP over PCC driver depends on the
> behavior in this patch. If you do revert, I will need a path forward.
> 

Sorry not yet. I still need to understand and analyse your last reply.

> Based on other code review feed back, I need to make an additional change: 
> the rx_alloc callback function needs to be atomically set, and thus needs to
> move to the mailbox API.  There it will pair with the prepare transaction
> function.  It is a small change, but I expect some feedback from the mailbox
> maintainers.
> 
> I know all of the other drivers that use the PCC mailbox currently do direct
> management of the shared buffer.  I suspect that is the biggest change that
> is causing you concern.  Are you OK with maintaining a mailbox-managed path
> to buffer management as well?  I think it will be beneficial to other
> drivers in the long run.
> 

If you are really interesting to consolidating, then move all the buffer
management into the core. Just don't introduce things that just work on
your platform and for your use case. You need move all the drivers to this
new model of accessing the buffers. Otherwise I see no point as it is just
another churn but in core mailbox PCC driver instead of a client driver
using PCC. So, I am not OK with the change as is and needs to be reworked
or reverted. I need sometime to understand your email and requirements.

-- 
Regards,
Sudeep

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Adam Young]

On 9/8/25 11:20, Sudeep Holla wrote:
> If you are really interesting to consolidating, then move all the buffer
> management into the core. Just don't introduce things that just work on
> your platform and for your use case. You need move all the drivers to this
> new model of accessing the buffers. Otherwise I see no point as it is just
> another churn but in core mailbox PCC driver instead of a client driver
> using PCC. So, I am not OK with the change as is and needs to be reworked
> or reverted. I need sometime to understand your email and requirements.

This is kindof a large request.  I can start working on getting the 
other drivers to use the common mechanisms, but I do not have a way to 
test most of them, and there are numerous drivers.  I don't like making 
changes that I cannot test myself, but if we can get the other driver 
maintainers to test and review, I am happy to participate.

I know we use the CPPC driver, and I can show how that one would be 
consolidated.

Here is a potential path forward:

1. Revert the current patch, specifically to remove the callback function.

2.  I will post a minimal patch for the change to the mailbox api

3.  I will post patches that modify the other drivers to pass NULL in to 
the send_message path.  These will need to be reviewed and tested by the 
other driver maintainers

4. I will post a revised patch that only performs the buffer management 
for the send path.  This is essential for the MCTP over PCC driver, and 
for anything that wants to follow the PCC spec correctly.  This will 
remove pcc_mchan->manage_writes = false; This path will be triggered by 
passing a non-NULL pointer into the send data path. This is roughly half 
to the current patch.

5.  I will post a revised patch that makes use of the mailbox API 
callback defined in step 2 to allocate the memory used for the read stage.

6.I will repost my MCTP over PCC driver  that makes  use  of the updated 
patches.

Any point after step 3, we can start migrating the drivers to use the 
send mechanism.  After step 5 we can migrate the drivers to use the 
receive mechanism.


A shorter path forward would be:

1.   I will post a minimal patch for the change to the mailbox api

2. I will post a revised PCC Mailbox patch that makes use of the 
callback function, as well as an updated MCTP-PCC driver that makes use 
of that callback.  We deprecate the existing rx_alloc callback in the 
PCC Mailbox driver and ignore it in the PCC Mailbox.

3. Convert the other drivers to use the managed send/receive mechanism.

4.  Remove the flag pcc_mchan->manage_writes

I will stay engaged through the entire process, to include providing 
patches for the updated drivers, testing whatever I have access to, and 
reviewing all code that comes along these paths. I obviously prefer the 
shorter path, as it will allow me to get the MCTP-PCC driver merged. My 
team is going to be writing another, similar Mailbox implementation to 
the PCC mailbox. The more common behavior between the two 
implementations, the less code we will have to vary between drivers that 
make use of the mailboxes.

Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

[Adam Young]

On 9/8/25 12:44, Adam Young wrote:
>
> On 9/8/25 11:20, Sudeep Holla wrote:
>> If you are really interesting to consolidating, then move all the buffer
>> management into the core. Just don't introduce things that just work on
>> your platform and for your use case. You need move all the drivers to 
>> this
>> new model of accessing the buffers. Otherwise I see no point as it is 
>> just
>> another churn but in core mailbox PCC driver instead of a client driver
>> using PCC. So, I am not OK with the change as is and needs to be 
>> reworked
>> or reverted. I need sometime to understand your email and requirements.
>
> This is kindof a large request.  I can start working on getting the 
> other drivers to use the common mechanisms, but I do not have a way to 
> test most of them, and there are numerous drivers.  I don't like 
> making changes that I cannot test myself, but if we can get the other 
> driver maintainers to test and review, I am happy to participate.
>
> I know we use the CPPC driver, and I can show how that one would be 
> consolidated.
>
> Here is a potential path forward:
>
> 1. Revert the current patch, specifically to remove the callback 
> function.
>
> 2.  I will post a minimal patch for the change to the mailbox api
>
> 3.  I will post patches that modify the other drivers to pass NULL in 
> to the send_message path.  These will need to be reviewed and tested 
> by the other driver maintainers
>
> 4. I will post a revised patch that only performs the buffer 
> management for the send path.  This is essential for the MCTP over PCC 
> driver, and for anything that wants to follow the PCC spec correctly.  
> This will remove pcc_mchan->manage_writes = false; This path will be 
> triggered by passing a non-NULL pointer into the send data path. This 
> is roughly half to the current patch.
>
> 5.  I will post a revised patch that makes use of the mailbox API 
> callback defined in step 2 to allocate the memory used for the read 
> stage.
>
> 6.I will repost my MCTP over PCC driver  that makes  use  of the 
> updated patches.
>
> Any point after step 3, we can start migrating the drivers to use the 
> send mechanism.  After step 5 we can migrate the drivers to use the 
> receive mechanism.
>
>
> A shorter path forward would be:
>
> 1.   I will post a minimal patch for the change to the mailbox api
>
> 2. I will post a revised PCC Mailbox patch that makes use of the 
> callback function, as well as an updated MCTP-PCC driver that makes 
> use of that callback.  We deprecate the existing rx_alloc callback in 
> the PCC Mailbox driver and ignore it in the PCC Mailbox.
>
> 3. Convert the other drivers to use the managed send/receive mechanism.
>
> 4.  Remove the flag pcc_mchan->manage_writes
>
> I will stay engaged through the entire process, to include providing 
> patches for the updated drivers, testing whatever I have access to, 
> and reviewing all code that comes along these paths. I obviously 
> prefer the shorter path, as it will allow me to get the MCTP-PCC 
> driver merged. My team is going to be writing another, similar Mailbox 
> implementation to the PCC mailbox. The more common behavior between 
> the two implementations, the less code we will have to vary between 
> drivers that make use of the mailboxes.
>
>
I will post an updated version of my patch series that starts to follows 
this second path.  I will not include the CPPC patch in there yet, as I 
need to get my head around what they are doing inside that driver.

Is there a forum in which I should cross post the Mailbox changes?  I 
plan on creating a new, mailbox API level  call back and I assume there 
will be a larger audience interested in reviewing that.


>
>