[PATCH v30 0/3] MCTP Over PCC Transport

[PATCH v30 0/3] MCTP Over PCC Transport

[Adam Young]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

A Previous version of this patch series had a pre-requisite patch that
allows the PCC Mailbox to managed the PCC shared buffer.  That patch has
been reverted. Instead, the functionaliity was moved to helper functions
which are explicitly called by the MCTP driver.

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

Changed in V30:
- rebased on revert of mailbox/pcc.c code
-  Explicit patch for dealing with PCC Type 3 ACK Interrupts
- PCC buffer management moved to helper functions
- PCC helper functions are explicitly called from Network Driver
- Removal of sk_buff queues
-

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Changes in V2

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

Adam Young (3):
  mailbox: pcc: Type3 Buffer handles ACK IRQ
  mailbox: pcc: functions for reading and writing PCC extended data
  mctp pcc: Implement MCTP over PCC Transport

 MAINTAINERS                 |   5 +
 drivers/mailbox/pcc.c       | 150 +++++++++++++++++
 drivers/net/mctp/Kconfig    |  13 ++
 drivers/net/mctp/Makefile   |   1 +
 drivers/net/mctp/mctp-pcc.c | 319 ++++++++++++++++++++++++++++++++++++
 include/acpi/pcc.h          |  38 +++++
 6 files changed, 526 insertions(+)
 create mode 100644 drivers/net/mctp/mctp-pcc.c

-- 
2.43.0

[PATCH v30 1/3] mailbox: pcc: Type3 Buffer handles ACK IRQ

[Adam Young]

The PCC protocol type 3 requests include a field that indicates that the
recipient should trigger an interrupt once the message has been read
from the buffer. The sender uses this interrupt to know that a
transmission is complete, and it is safe to send additional messages.

Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>

mailbox/pcc extended memory helper functions
---
 drivers/mailbox/pcc.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index f6714c233f5a..978a7b674946 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -306,6 +306,18 @@ static void pcc_chan_acknowledge(struct pcc_chan_info *pchan)
 		pcc_chan_reg_read_modify_write(&pchan->db);
 }
 
+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_mbox_irq - PCC mailbox interrupt handler
  * @irq:	interrupt number
@@ -340,6 +352,14 @@ 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;
+
+	/**
+	 * The remote side sent an ack.
+	 */
+	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE &&
+	    chan->active_req)
+		mbox_chan_txdone(chan, 0);
+
 	mbox_chan_received_data(chan, NULL);
 
 	pcc_chan_acknowledge(pchan);
@@ -490,6 +510,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,
 };
 
 /**
-- 
2.43.0

[PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data

[Adam Young]

Adds functions that aid in compliance with the PCC protocol by
checking the command complete flag status.

Adds a function that exposes the size of the shared buffer without
activating the channel.

Adds a function that allows a client to query the number of bytes
avaialbel to read in order to preallocate buffers for reading.

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

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 978a7b674946..653897d61db5 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -367,6 +367,46 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static
+struct pcc_chan_info *lookup_channel_info(int subspace_id)
+{
+	struct pcc_chan_info *pchan;
+	struct mbox_chan *chan;
+
+	if (subspace_id < 0 || subspace_id >= pcc_chan_count)
+		return ERR_PTR(-ENOENT);
+
+	pchan = chan_info + subspace_id;
+	chan = pchan->chan.mchan;
+	if (IS_ERR(chan) || chan->cl) {
+		pr_err("Channel not found for idx: %d\n", subspace_id);
+		return ERR_PTR(-EBUSY);
+	}
+	return pchan;
+}
+
+/**
+ * pcc_mbox_buffer_size - PCC clients call this function to
+ *		request the size of the shared buffer in cases
+ *              where requesting the channel would prematurely
+ *              trigger channel activation and message delivery.
+ * @subspace_id: The PCC Subspace index as parsed in the PCC client
+ *		ACPI package. This is used to lookup the array of PCC
+ *		subspaces as parsed by the PCC Mailbox controller.
+ *
+ * Return: The size of the shared buffer.
+ */
+int pcc_mbox_buffer_size(int index)
+{
+	struct pcc_chan_info *pchan = lookup_channel_info(index);
+
+	if (IS_ERR(pchan))
+		return -1;
+	return pchan->chan.shmem_size;
+}
+EXPORT_SYMBOL_GPL(pcc_mbox_buffer_size);
+
+
 /**
  * pcc_mbox_request_channel - PCC clients call this function to
  *		request a pointer to their PCC subspace, from which they
@@ -437,6 +477,95 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
 }
 EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
 
+/**
+ * pcc_mbox_query_bytes_available
+ *
+ * @pchan pointer to channel associated with buffer
+ * Return: the number of bytes available to read from the shared buffer
+ */
+int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan)
+{
+	struct pcc_extended_header pcc_header;
+	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
+	int data_len;
+	u64 val;
+
+	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
+	if (val) {
+		pr_info("%s Buffer not enabled for reading", __func__);
+		return -1;
+	}
+	memcpy_fromio(&pcc_header, pchan->shmem,
+		      sizeof(pcc_header));
+	data_len = pcc_header.length - sizeof(u32) + sizeof(pcc_header);
+	return data_len;
+}
+EXPORT_SYMBOL_GPL(pcc_mbox_query_bytes_available);
+
+/**
+ * pcc_mbox_read_from_buffer - Copy bytes from shared buffer into data
+ *
+ * @pchan - channel associated with the shared buffer
+ * @len - number of bytes to read
+ * @data - pointer to memory in which to write the data from the
+ *         shared buffer
+ *
+ * Return: number of bytes read and written into daa
+ */
+int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
+{
+	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
+	int data_len;
+	u64 val;
+
+	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
+	if (val) {
+		pr_info("%s buffer not enabled for reading", __func__);
+		return -1;
+	}
+	data_len  = pcc_mbox_query_bytes_available(pchan);
+	if (len < data_len)
+		data_len = len;
+	memcpy_fromio(data, pchan->shmem, len);
+	return len;
+}
+EXPORT_SYMBOL_GPL(pcc_mbox_read_from_buffer);
+
+/**
+ * pcc_mbox_write_to_buffer, copy the contents of the data
+ * pointer to the shared buffer.  Confirms that the command
+ * flag has been set prior to writing.  Data should be a
+ * properly formatted extended data buffer.
+ * pcc_mbox_write_to_buffer
+ * @pchan: channel
+ * @len: Length of the overall buffer passed in, including the
+ *       Entire header. The length value in the shared buffer header
+ *       Will be calculated from len.
+ * @data: Client specific data to be written to the shared buffer.
+ * Return: number of bytes written to the buffer.
+ */
+int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
+{
+	struct pcc_extended_header *pcc_header = data;
+	struct mbox_chan *mbox_chan = pchan->mchan;
+
+	/*
+	 * The PCC header length includes the command field
+	 * but not the other values from the header.
+	 */
+	pcc_header->length = len - sizeof(struct pcc_extended_header) + sizeof(u32);
+
+	if (!pcc_last_tx_done(mbox_chan)) {
+		pr_info("%s pchan->cmd_complete not set.", __func__);
+		return 0;
+	}
+	memcpy_toio(pchan->shmem,  data, len);
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(pcc_mbox_write_to_buffer);
+
+
 /**
  * pcc_send_data - Called from Mailbox Controller code. Used
  *		here only to ring the channel doorbell. The PCC client
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 840bfc95bae3..96a6f85fc1ba 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -19,6 +19,13 @@ struct pcc_mbox_chan {
 	u16 min_turnaround_time;
 };
 
+struct pcc_extended_header {
+	u32 signature;
+	u32 flags;
+	u32 length;
+	u32 command;
+};
+
 /* Generic Communications Channel Shared Memory Region */
 #define PCC_SIGNATURE			0x50434300
 /* Generic Communications Channel Command Field */
@@ -37,6 +44,17 @@ struct pcc_mbox_chan {
 extern struct pcc_mbox_chan *
 pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id);
 extern void pcc_mbox_free_channel(struct pcc_mbox_chan *chan);
+extern
+int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data);
+extern
+int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan);
+extern
+int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len,
+			      void *data);
+extern
+int pcc_mbox_buffer_size(int index);
+
+
 #else
 static inline struct pcc_mbox_chan *
 pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
@@ -44,6 +62,26 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
 	return ERR_PTR(-ENODEV);
 }
 static inline void pcc_mbox_free_channel(struct pcc_mbox_chan *chan) { }
+static inline
+int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
+{
+	return 0;
+}
+static inline int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan);
+{
+	return 0;
+}
+static inline
+int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
+{
+	return 0;
+}
+static inline
+int pcc_mbox_buffer_size(int index)
+{
+	return -1;
+}
+
 #endif
 
 #endif /* _PCC_H */
-- 
2.43.0

[PATCH v30 3/3] mctp pcc: Implement MCTP over PCC Transport

[Adam Young]

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

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

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

This driver takes advantage of PCC mailbox shared functions
management. Unlike the existing Type 2 drivers, the mssg
parameter is actively used. The data section of the struct sk_buff
that contains the outgoing packet is sent to the mailbox,
already properly formatted as a PCC exctended message.

The driver makes use of the pcc mailbox buffer management helpers.
These allow the network driver to use common code for the reading and
wrting from the shared memory buffer to the mailbox driver,
attempting to get a single implementation of the PCC protocol for
Type3/4.

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

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

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

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 46126ce2f968..e1497608a05d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14915,6 +14915,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>
 R:	Alice Ryhl <aliceryhl@google.com>
diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
index cf325ab0b1ef..77cd4091050c 100644
--- a/drivers/net/mctp/Kconfig
+++ b/drivers/net/mctp/Kconfig
@@ -47,6 +47,19 @@ config MCTP_TRANSPORT_I3C
 	  A MCTP protocol network device is created for each I3C bus
 	  having a "mctp-controller" devicetree property.
 
+config MCTP_TRANSPORT_PCC
+	tristate "MCTP PCC transport"
+	depends on ACPI
+	help
+	  Provides a driver to access MCTP devices over PCC transport,
+	  A MCTP protocol network device is created via ACPI for each
+	  entry in the DSDT/SSDT that matches the identifier. The Platform
+	  communication channels are selected from the corresponding
+	  entries in the PCCT.
+
+	  Say y here if you need to connect to MCTP endpoints over PCC. To
+	  compile as a module, use m; the module will be called mctp-pcc.
+
 config MCTP_TRANSPORT_USB
 	tristate "MCTP USB transport"
 	depends on USB
diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile
index c36006849a1e..0a591299ffa9 100644
--- a/drivers/net/mctp/Makefile
+++ b/drivers/net/mctp/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o
 obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o
 obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.o
+obj-$(CONFIG_MCTP_TRANSPORT_PCC) += mctp-pcc.o
 obj-$(CONFIG_MCTP_TRANSPORT_USB) += mctp-usb.o
diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
new file mode 100644
index 000000000000..927a525c1121
--- /dev/null
+++ b/drivers/net/mctp/mctp-pcc.c
@@ -0,0 +1,319 @@
+// 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/hrtimer.h>
+#include <linux/if_arp.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>
+#include <linux/skbuff.h>
+#include <linux/string.h>
+
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+#include <acpi/acrestyp.h>
+#include <acpi/actbl.h>
+#include <acpi/pcc.h>
+#include <net/mctp.h>
+#include <net/mctpdevice.h>
+
+#define MCTP_SIGNATURE          "MCTP"
+#define MCTP_SIGNATURE_LENGTH   (sizeof(MCTP_SIGNATURE) - 1)
+#define MCTP_MIN_MTU            68
+#define PCC_DWORD_TYPE          0x0c
+
+struct mctp_pcc_mailbox {
+	u32 index;
+	struct pcc_mbox_chan *chan;
+	struct mbox_client client;
+};
+
+/* The netdev structure. One of these per PCC adapter. */
+struct mctp_pcc_ndev {
+	struct net_device *ndev;
+	struct acpi_device *acpi_device;
+	struct mctp_pcc_mailbox inbox;
+	struct mctp_pcc_mailbox outbox;
+};
+
+static void mctp_pcc_client_rx_callback(struct mbox_client *cl, void *mssg)
+{
+	struct pcc_extended_header pcc_header;
+	struct mctp_pcc_ndev *mctp_pcc_ndev;
+	struct mctp_pcc_mailbox *inbox;
+	struct mctp_skb_cb *cb;
+	struct sk_buff *skb;
+	int size;
+
+	mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, inbox.client);
+	inbox = &mctp_pcc_ndev->inbox;
+	size = pcc_mbox_query_bytes_available(inbox->chan);
+	if (size == 0)
+		return;
+	skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
+	if (!skb) {
+		dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
+		return;
+	}
+	skb_put(skb, size);
+	skb->protocol = htons(ETH_P_MCTP);
+	pcc_mbox_read_from_buffer(inbox->chan, size, skb->data);
+	dev_dstats_rx_add(mctp_pcc_ndev->ndev, skb->len);
+	skb_reset_mac_header(skb);
+	skb_pull(skb, sizeof(pcc_header));
+	skb_reset_network_header(skb);
+	cb = __mctp_cb(skb);
+	cb->halen = 0;
+	netif_rx(skb);
+}
+
+static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
+	struct pcc_extended_header *pcc_header;
+	int len = skb->len;
+	int rc;
+
+	rc = skb_cow_head(skb, sizeof(*pcc_header));
+	if (rc) {
+		dev_dstats_tx_dropped(ndev);
+		kfree_skb(skb);
+		return NETDEV_TX_OK;
+	}
+
+	pcc_header = skb_push(skb, sizeof(*pcc_header));
+	pcc_header->signature = PCC_SIGNATURE | mpnd->outbox.index;
+	pcc_header->flags = PCC_CMD_COMPLETION_NOTIFY;
+	memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
+	pcc_header->length = len + MCTP_SIGNATURE_LENGTH;
+	rc = mbox_send_message(mpnd->outbox.chan->mchan, skb);
+
+	if (rc < 0) {
+		netif_stop_queue(ndev);
+		return NETDEV_TX_BUSY;
+	}
+
+	dev_dstats_tx_add(ndev, len);
+	return NETDEV_TX_OK;
+}
+
+static void mctp_pcc_tx_prepare(struct mbox_client *cl, void *mssg)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev;
+	struct mctp_pcc_mailbox *outbox;
+	struct sk_buff *skb = mssg;
+	int len_sent;
+
+	mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, outbox.client);
+	outbox = &mctp_pcc_ndev->outbox;
+
+	if (!skb)
+		return;
+
+	len_sent = pcc_mbox_write_to_buffer(outbox->chan, skb->len, skb->data);
+	if (len_sent == 0)
+		pr_info("packet dropped");
+}
+
+static void mctp_pcc_tx_done(struct mbox_client *c, void *mssg, int r)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev;
+	struct mctp_pcc_mailbox *outbox;
+	struct sk_buff *skb = mssg;
+
+	mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, outbox.client);
+	outbox = container_of(c, struct mctp_pcc_mailbox, client);
+	if (skb)
+		dev_consume_skb_any(skb);
+	netif_wake_queue(mctp_pcc_ndev->ndev);
+}
+
+static int mctp_pcc_ndo_open(struct net_device *ndev)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
+	struct mctp_pcc_mailbox *outbox, *inbox;
+
+	outbox = &mctp_pcc_ndev->outbox;
+	inbox = &mctp_pcc_ndev->inbox;
+
+	outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
+	if (IS_ERR(outbox->chan))
+		return PTR_ERR(outbox->chan);
+
+	inbox->client.rx_callback = mctp_pcc_client_rx_callback;
+	inbox->chan = pcc_mbox_request_channel(&inbox->client, inbox->index);
+	if (IS_ERR(inbox->chan)) {
+		pcc_mbox_free_channel(outbox->chan);
+		return PTR_ERR(inbox->chan);
+	}
+	return 0;
+}
+
+static int mctp_pcc_ndo_stop(struct net_device *ndev)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
+
+	pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
+	pcc_mbox_free_channel(mctp_pcc_ndev->inbox.chan);
+	return 0;
+}
+
+static const struct net_device_ops mctp_pcc_netdev_ops = {
+	.ndo_open = mctp_pcc_ndo_open,
+	.ndo_stop = mctp_pcc_ndo_stop,
+	.ndo_start_xmit = mctp_pcc_tx,
+};
+
+static void mctp_pcc_setup(struct net_device *ndev)
+{
+	ndev->type = ARPHRD_MCTP;
+	ndev->hard_header_len = 0;
+	ndev->tx_queue_len = 0;
+	ndev->flags = IFF_NOARP;
+	ndev->netdev_ops = &mctp_pcc_netdev_ops;
+	ndev->needs_free_netdev = true;
+	ndev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
+}
+
+struct mctp_pcc_lookup_context {
+	int index;
+	u32 inbox_index;
+	u32 outbox_index;
+};
+
+static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
+				       void *context)
+{
+	struct mctp_pcc_lookup_context *luc = context;
+	struct acpi_resource_address32 *addr;
+
+	if (ares->type != PCC_DWORD_TYPE)
+		return AE_OK;
+
+	addr = ACPI_CAST_PTR(struct acpi_resource_address32, &ares->data);
+	switch (luc->index) {
+	case 0:
+		luc->outbox_index = addr[0].address.minimum;
+		break;
+	case 1:
+		luc->inbox_index = addr[0].address.minimum;
+		break;
+	}
+	luc->index++;
+	return AE_OK;
+}
+
+static void mctp_cleanup_netdev(void *data)
+{
+	struct net_device *ndev = data;
+
+	mctp_unregister_netdev(ndev);
+}
+
+static int initialize_MTU(struct net_device *ndev)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
+	struct mctp_pcc_mailbox *outbox;
+	int mctp_pcc_mtu;
+
+	outbox = &mctp_pcc_ndev->outbox;
+	mctp_pcc_mtu = pcc_mbox_buffer_size(outbox->index);
+	if (mctp_pcc_mtu == -1)
+		return -1;
+
+	mctp_pcc_mtu = mctp_pcc_mtu - sizeof(struct pcc_extended_header);
+	mctp_pcc_ndev = netdev_priv(ndev);
+	ndev->mtu = MCTP_MIN_MTU;
+	ndev->max_mtu = mctp_pcc_mtu;
+	ndev->min_mtu = MCTP_MIN_MTU;
+
+	return 0;
+}
+
+static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
+{
+	struct mctp_pcc_lookup_context context = {0};
+	struct mctp_pcc_ndev *mctp_pcc_ndev;
+	struct device *dev = &acpi_dev->dev;
+	struct net_device *ndev;
+	acpi_handle dev_handle;
+	acpi_status status;
+	char name[32];
+	int rc;
+
+	dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
+		acpi_device_hid(acpi_dev));
+	dev_handle = acpi_device_handle(acpi_dev);
+	status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
+				     &context);
+	if (!ACPI_SUCCESS(status)) {
+		dev_err(dev, "FAILED to lookup PCC indexes from CRS\n");
+		return -EINVAL;
+	}
+
+	snprintf(name, sizeof(name), "mctppcc%d", context.inbox_index);
+	ndev = alloc_netdev(sizeof(*mctp_pcc_ndev), name, NET_NAME_PREDICTABLE,
+			    mctp_pcc_setup);
+	if (!ndev)
+		return -ENOMEM;
+
+	mctp_pcc_ndev = netdev_priv(ndev);
+
+	mctp_pcc_ndev->inbox.index = context.inbox_index;
+	mctp_pcc_ndev->inbox.client.dev = dev;
+	mctp_pcc_ndev->outbox.index = context.outbox_index;
+	mctp_pcc_ndev->outbox.client.dev = dev;
+
+	mctp_pcc_ndev->outbox.client.tx_prepare = mctp_pcc_tx_prepare;
+	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;
+
+	rc = initialize_MTU(ndev);
+	if (rc)
+		goto free_netdev;
+
+	rc = mctp_register_netdev(ndev, NULL, 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 v30 3/3] mctp pcc: Implement MCTP over PCC Transport

[Jeremy Kerr]

Hi Adam,

Looking pretty good, a few things inline:

> diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
> new file mode 100644
> index 000000000000..927a525c1121
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c
> @@ -0,0 +1,319 @@
> +// 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/hrtimer.h>
> +#include <linux/if_arp.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/netdevice.h>
> +#include <linux/platform_device.h>
> +#include <linux/skbuff.h>
> +#include <linux/string.h>
> +
> +#include <acpi/acpi_bus.h>
> +#include <acpi/acpi_drivers.h>
> +#include <acpi/acrestyp.h>
> +#include <acpi/actbl.h>
> +#include <acpi/pcc.h>
> +#include <net/mctp.h>
> +#include <net/mctpdevice.h>
> +
> +#define MCTP_SIGNATURE          "MCTP"
> +#define MCTP_SIGNATURE_LENGTH   (sizeof(MCTP_SIGNATURE) - 1)
> +#define MCTP_MIN_MTU            68
> +#define PCC_DWORD_TYPE          0x0c
> +
> +struct mctp_pcc_mailbox {
> +       u32 index;
> +       struct pcc_mbox_chan *chan;
> +       struct mbox_client client;
> +};
> +
> +/* The netdev structure. One of these per PCC adapter. */
> +struct mctp_pcc_ndev {
> +       struct net_device *ndev;
> +       struct acpi_device *acpi_device;
> +       struct mctp_pcc_mailbox inbox;
> +       struct mctp_pcc_mailbox outbox;
> +};
> +
> +static void mctp_pcc_client_rx_callback(struct mbox_client *cl, void *mssg)
> +{
> +       struct pcc_extended_header pcc_header;
> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> +       struct mctp_pcc_mailbox *inbox;
> +       struct mctp_skb_cb *cb;
> +       struct sk_buff *skb;
> +       int size;
> +
> +       mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, inbox.client);
> +       inbox = &mctp_pcc_ndev->inbox;
> +       size = pcc_mbox_query_bytes_available(inbox->chan);
> +       if (size == 0)
> +               return;
> +       skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
> +       if (!skb) {
> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +               return;
> +       }
> +       skb_put(skb, size);
> +       skb->protocol = htons(ETH_P_MCTP);
> +       pcc_mbox_read_from_buffer(inbox->chan, size, skb->data);
> +       dev_dstats_rx_add(mctp_pcc_ndev->ndev, skb->len);
> +       skb_reset_mac_header(skb);
> +       skb_pull(skb, sizeof(pcc_header));
> +       skb_reset_network_header(skb);
> +       cb = __mctp_cb(skb);
> +       cb->halen = 0;
> +       netif_rx(skb);
> +}
> +
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +       struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
> +       struct pcc_extended_header *pcc_header;
> +       int len = skb->len;
> +       int rc;
> +
> +       rc = skb_cow_head(skb, sizeof(*pcc_header));
> +       if (rc) {
> +               dev_dstats_tx_dropped(ndev);
> +               kfree_skb(skb);
> +               return NETDEV_TX_OK;
> +       }
> +
> +       pcc_header = skb_push(skb, sizeof(*pcc_header));
> +       pcc_header->signature = PCC_SIGNATURE | mpnd->outbox.index;
> +       pcc_header->flags = PCC_CMD_COMPLETION_NOTIFY;
> +       memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
> +       pcc_header->length = len + MCTP_SIGNATURE_LENGTH;
> +       rc = mbox_send_message(mpnd->outbox.chan->mchan, skb);
> +
> +       if (rc < 0) {
> +               netif_stop_queue(ndev);
> +               return NETDEV_TX_BUSY;
> +       }

super minor: the grouping here (and a couple of other places) is a bit
strange; try and keep the rc handler together with where rc is set, and
the setup and send distinct. I'd typically do this as:

       pcc_header = skb_push(skb, sizeof(*pcc_header));
       pcc_header->signature = PCC_SIGNATURE | mpnd->outbox.index;
       pcc_header->flags = PCC_CMD_COMPLETION_NOTIFY;
       memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
       pcc_header->length = len + MCTP_SIGNATURE_LENGTH;

       rc = mbox_send_message(mpnd->outbox.chan->mchan, skb);
       if (rc < 0) {
               netif_stop_queue(ndev);
               return NETDEV_TX_BUSY;
       }

> +
> +       dev_dstats_tx_add(ndev, len);
> +       return NETDEV_TX_OK;
> +}
> +
> +static void mctp_pcc_tx_prepare(struct mbox_client *cl, void *mssg)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> +       struct mctp_pcc_mailbox *outbox;
> +       struct sk_buff *skb = mssg;
> +       int len_sent;
> +
> +       mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, outbox.client);
> +       outbox = &mctp_pcc_ndev->outbox;
> +
> +       if (!skb)
> +               return;
> +
> +       len_sent = pcc_mbox_write_to_buffer(outbox->chan, skb->len, skb->data);
> +       if (len_sent == 0)
> +               pr_info("packet dropped");

You probably don't want a bare pr_info() in the failure path here;
either something debug level, or ratelimited.

and probably use a netdev-specific log function (netdev_dbg() perhaps),
so you get the device information too.

I'm not super clear on this failure mode, do you need to update the
tx statistics on the drop here? In which case, you may not need the
_dbg/_info message at all.

> +}
> +
> +static void mctp_pcc_tx_done(struct mbox_client *c, void *mssg, int r)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> +       struct mctp_pcc_mailbox *outbox;
> +       struct sk_buff *skb = mssg;

Nice, no more list lookups.

> +
> +       mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, outbox.client);
> +       outbox = container_of(c, struct mctp_pcc_mailbox, client);
> +       if (skb)
> +               dev_consume_skb_any(skb);

minor: dev_consume_skb_any() will tolerate a null argument, you can
leave out the conditional here.

> +       netif_wake_queue(mctp_pcc_ndev->ndev);
> +}
> +
> +static int mctp_pcc_ndo_open(struct net_device *ndev)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
> +       struct mctp_pcc_mailbox *outbox, *inbox;
> +
> +       outbox = &mctp_pcc_ndev->outbox;
> +       inbox = &mctp_pcc_ndev->inbox;
> +
> +       outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
> +       if (IS_ERR(outbox->chan))
> +               return PTR_ERR(outbox->chan);
> +
> +       inbox->client.rx_callback = mctp_pcc_client_rx_callback;
> +       inbox->chan = pcc_mbox_request_channel(&inbox->client, inbox->index);
> +       if (IS_ERR(inbox->chan)) {
> +               pcc_mbox_free_channel(outbox->chan);
> +               return PTR_ERR(inbox->chan);
> +       }
> +       return 0;
> +}

Having the channels fully set-up before calling request_channel looks
much safer now :)

> +
> +static int mctp_pcc_ndo_stop(struct net_device *ndev)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
> +
> +       pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
> +       pcc_mbox_free_channel(mctp_pcc_ndev->inbox.chan);
> +       return 0;
> +}
> +
> +static const struct net_device_ops mctp_pcc_netdev_ops = {
> +       .ndo_open = mctp_pcc_ndo_open,
> +       .ndo_stop = mctp_pcc_ndo_stop,
> +       .ndo_start_xmit = mctp_pcc_tx,
> +};
> +
> +static void mctp_pcc_setup(struct net_device *ndev)
> +{
> +       ndev->type = ARPHRD_MCTP;
> +       ndev->hard_header_len = 0;

Isn't this sizeof(struct pcc_extended_header) ?

> +       ndev->tx_queue_len = 0;
> +       ndev->flags = IFF_NOARP;
> +       ndev->netdev_ops = &mctp_pcc_netdev_ops;
> +       ndev->needs_free_netdev = true;
> +       ndev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
> +}
> +
> +struct mctp_pcc_lookup_context {
> +       int index;
> +       u32 inbox_index;
> +       u32 outbox_index;
> +};
> +
> +static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
> +                                      void *context)
> +{
> +       struct mctp_pcc_lookup_context *luc = context;
> +       struct acpi_resource_address32 *addr;
> +
> +       if (ares->type != PCC_DWORD_TYPE)
> +               return AE_OK;
> +
> +       addr = ACPI_CAST_PTR(struct acpi_resource_address32, &ares->data);
> +       switch (luc->index) {
> +       case 0:
> +               luc->outbox_index = addr[0].address.minimum;
> +               break;
> +       case 1:
> +               luc->inbox_index = addr[0].address.minimum;
> +               break;
> +       }
> +       luc->index++;
> +       return AE_OK;
> +}
> +
> +static void mctp_cleanup_netdev(void *data)
> +{
> +       struct net_device *ndev = data;
> +
> +       mctp_unregister_netdev(ndev);
> +}
> +
> +static int initialize_MTU(struct net_device *ndev)
> +{
> +       struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
> +       struct mctp_pcc_mailbox *outbox;
> +       int mctp_pcc_mtu;
> +
> +       outbox = &mctp_pcc_ndev->outbox;
> +       mctp_pcc_mtu = pcc_mbox_buffer_size(outbox->index);
> +       if (mctp_pcc_mtu == -1)
> +               return -1;

You may want to check that this is at least
sizeof(struct pcc_extended_header), to avoid an underflow below, and
possibly at least MCTP_MIN_MTU after subtracting the header size (as
this would be less than the MCTP BTU)

> +
> +       mctp_pcc_mtu = mctp_pcc_mtu - sizeof(struct pcc_extended_header);
> +       mctp_pcc_ndev = netdev_priv(ndev);

unneeded?

> +       ndev->mtu = MCTP_MIN_MTU;
> +       ndev->max_mtu = mctp_pcc_mtu;
> +       ndev->min_mtu = MCTP_MIN_MTU;
> +
> +       return 0;
> +}

Now that the initialize_MTU implementation is simpler, you may want to
reinstate it inline in driver_add(), but also fine as-is.

> +
> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
> +{
> +       struct mctp_pcc_lookup_context context = {0};
> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
> +       struct device *dev = &acpi_dev->dev;
> +       struct net_device *ndev;
> +       acpi_handle dev_handle;
> +       acpi_status status;
> +       char name[32];
> +       int rc;
> +
> +       dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
> +               acpi_device_hid(acpi_dev));
> +       dev_handle = acpi_device_handle(acpi_dev);
> +       status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
> +                                    &context);
> +       if (!ACPI_SUCCESS(status)) {
> +               dev_err(dev, "FAILED to lookup PCC indexes from CRS\n");
> +               return -EINVAL;
> +       }
> +
> +       snprintf(name, sizeof(name), "mctppcc%d", context.inbox_index);
> +       ndev = alloc_netdev(sizeof(*mctp_pcc_ndev), name, NET_NAME_PREDICTABLE,
> +                           mctp_pcc_setup);
> +       if (!ndev)
> +               return -ENOMEM;
> +
> +       mctp_pcc_ndev = netdev_priv(ndev);
> +
> +       mctp_pcc_ndev->inbox.index = context.inbox_index;
> +       mctp_pcc_ndev->inbox.client.dev = dev;
> +       mctp_pcc_ndev->outbox.index = context.outbox_index;
> +       mctp_pcc_ndev->outbox.client.dev = dev;
> +
> +       mctp_pcc_ndev->outbox.client.tx_prepare = mctp_pcc_tx_prepare;
> +       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;
> +
> +       rc = initialize_MTU(ndev);
> +       if (rc)
> +               goto free_netdev;
> +
> +       rc = mctp_register_netdev(ndev, NULL, 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>");

Cheers,


Jeremy

Re: [External] : [PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data

[ALOK TIWARI]

On 10/17/2025 2:32 AM, Adam Young wrote:
> Adds functions that aid in compliance with the PCC protocol by
> checking the command complete flag status.
> 
> Adds a function that exposes the size of the shared buffer without
> activating the channel.
> 
> Adds a function that allows a client to query the number of bytes
> avaialbel to read in order to preallocate buffers for reading.

/s available

> 
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
>   drivers/mailbox/pcc.c | 129 ++++++++++++++++++++++++++++++++++++++++++
>   include/acpi/pcc.h    |  38 +++++++++++++
>   2 files changed, 167 insertions(+)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 978a7b674946..653897d61db5 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -367,6 +367,46 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	return IRQ_HANDLED;
>   }
>   
> +static
> +struct pcc_chan_info *lookup_channel_info(int subspace_id)
> +{
> +	struct pcc_chan_info *pchan;
> +	struct mbox_chan *chan;
> +
> +	if (subspace_id < 0 || subspace_id >= pcc_chan_count)
> +		return ERR_PTR(-ENOENT);
> +
> +	pchan = chan_info + subspace_id;
> +	chan = pchan->chan.mchan;
> +	if (IS_ERR(chan) || chan->cl) {
> +		pr_err("Channel not found for idx: %d\n", subspace_id);
> +		return ERR_PTR(-EBUSY);
> +	}
> +	return pchan;
> +}
> +
> +/**
> + * pcc_mbox_buffer_size - PCC clients call this function to
> + *		request the size of the shared buffer in cases
> + *              where requesting the channel would prematurely
> + *              trigger channel activation and message delivery.
> + * @subspace_id: The PCC Subspace index as parsed in the PCC client
> + *		ACPI package. This is used to lookup the array of PCC
> + *		subspaces as parsed by the PCC Mailbox controller.
> + *
> + * Return: The size of the shared buffer.
> + */
> +int pcc_mbox_buffer_size(int index)

use subspace_id for consistent name and use in header

> +{
> +	struct pcc_chan_info *pchan = lookup_channel_info(index);
> +
> +	if (IS_ERR(pchan))
> +		return -1;
> +	return pchan->chan.shmem_size;
> +}
> +EXPORT_SYMBOL_GPL(pcc_mbox_buffer_size);
> +
> +
>   /**
>    * pcc_mbox_request_channel - PCC clients call this function to
>    *		request a pointer to their PCC subspace, from which they
> @@ -437,6 +477,95 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>   }
>   EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>   
> +/**
> + * pcc_mbox_query_bytes_available
> + *
> + * @pchan pointer to channel associated with buffer
> + * Return: the number of bytes available to read from the shared buffer
> + */
> +int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan)
> +{
> +	struct pcc_extended_header pcc_header;
> +	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
> +	int data_len;
> +	u64 val;
> +
> +	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
> +	if (val) {
> +		pr_info("%s Buffer not enabled for reading", __func__);
> +		return -1;
> +	}
> +	memcpy_fromio(&pcc_header, pchan->shmem,
> +		      sizeof(pcc_header));
> +	data_len = pcc_header.length - sizeof(u32) + sizeof(pcc_header);
> +	return data_len;
> +}
> +EXPORT_SYMBOL_GPL(pcc_mbox_query_bytes_available);
> +
> +/**
> + * pcc_mbox_read_from_buffer - Copy bytes from shared buffer into data
> + *
> + * @pchan - channel associated with the shared buffer
> + * @len - number of bytes to read
> + * @data - pointer to memory in which to write the data from the
> + *         shared buffer
> + *
> + * Return: number of bytes read and written into daa

typo daa -> data

> + */
> +int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
> +{
> +	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
> +	int data_len;
> +	u64 val;
> +
> +	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
> +	if (val) {
> +		pr_info("%s buffer not enabled for reading", __func__);
> +		return -1;
> +	}
> +	data_len  = pcc_mbox_query_bytes_available(pchan);
> +	if (len < data_len)
> +		data_len = len;
> +	memcpy_fromio(data, pchan->shmem, len);
> +	return len;
> +}
> +EXPORT_SYMBOL_GPL(pcc_mbox_read_from_buffer);
> +
> +/**
> + * pcc_mbox_write_to_buffer, copy the contents of the data
> + * pointer to the shared buffer.  Confirms that the command
> + * flag has been set prior to writing.  Data should be a
> + * properly formatted extended data buffer.
> + * pcc_mbox_write_to_buffer
> + * @pchan: channel
> + * @len: Length of the overall buffer passed in, including the
> + *       Entire header. The length value in the shared buffer header
> + *       Will be calculated from len.
> + * @data: Client specific data to be written to the shared buffer.
> + * Return: number of bytes written to the buffer.
> + */
> +int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
> +{
> +	struct pcc_extended_header *pcc_header = data;
> +	struct mbox_chan *mbox_chan = pchan->mchan;
> +
> +	/*
> +	 * The PCC header length includes the command field
> +	 * but not the other values from the header.
> +	 */
> +	pcc_header->length = len - sizeof(struct pcc_extended_header) + sizeof(u32);
> +
> +	if (!pcc_last_tx_done(mbox_chan)) {
> +		pr_info("%s pchan->cmd_complete not set.", __func__);
> +		return 0;
> +	}
> +	memcpy_toio(pchan->shmem,  data, len);
> +
> +	return len;
> +}
> +EXPORT_SYMBOL_GPL(pcc_mbox_write_to_buffer);
> +
> +
>   /**
>    * pcc_send_data - Called from Mailbox Controller code. Used
>    *		here only to ring the channel doorbell. The PCC client
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 840bfc95bae3..96a6f85fc1ba 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -19,6 +19,13 @@ struct pcc_mbox_chan {
>   	u16 min_turnaround_time;
>   };
>   
> +struct pcc_extended_header {
> +	u32 signature;
> +	u32 flags;
> +	u32 length;
> +	u32 command;
> +};
> +
>   /* Generic Communications Channel Shared Memory Region */
>   #define PCC_SIGNATURE			0x50434300
>   /* Generic Communications Channel Command Field */
> @@ -37,6 +44,17 @@ struct pcc_mbox_chan {
>   extern struct pcc_mbox_chan *
>   pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id);
>   extern void pcc_mbox_free_channel(struct pcc_mbox_chan *chan);
> +extern
> +int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data);
> +extern
> +int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan);
> +extern
> +int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len,
> +			      void *data);
> +extern
> +int pcc_mbox_buffer_size(int index);
> +
> +
>   #else
>   static inline struct pcc_mbox_chan *
>   pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
> @@ -44,6 +62,26 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   	return ERR_PTR(-ENODEV);
>   }
>   static inline void pcc_mbox_free_channel(struct pcc_mbox_chan *chan) { }
> +static inline
> +int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
> +{
> +	return 0;
> +}
> +static inline int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan);

remove ;

> +{
> +	return 0;
> +}
> +static inline
> +int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
> +{
> +	return 0;
> +}
> +static inline
> +int pcc_mbox_buffer_size(int index)
> +{
> +	return -1;
> +}
> +
>   #endif
>   
>   #endif /* _PCC_H */


Thanks,
Alok

Re: [PATCH v30 1/3] mailbox: pcc: Type3 Buffer handles ACK IRQ

[Adam Young]

This is obsoleted/duplicated by Sudeep's patch.  I am going to rebase on 
his patch series.

On 10/16/25 17:02, Adam Young wrote:
> The PCC protocol type 3 requests include a field that indicates that the
> recipient should trigger an interrupt once the message has been read
> from the buffer. The sender uses this interrupt to know that a
> transmission is complete, and it is safe to send additional messages.
>
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
>
> mailbox/pcc extended memory helper functions
> ---
>   drivers/mailbox/pcc.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index f6714c233f5a..978a7b674946 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -306,6 +306,18 @@ static void pcc_chan_acknowledge(struct pcc_chan_info *pchan)
>   		pcc_chan_reg_read_modify_write(&pchan->db);
>   }
>   
> +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_mbox_irq - PCC mailbox interrupt handler
>    * @irq:	interrupt number
> @@ -340,6 +352,14 @@ 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;
> +
> +	/**
> +	 * The remote side sent an ack.
> +	 */
> +	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE &&
> +	    chan->active_req)
> +		mbox_chan_txdone(chan, 0);
> +
>   	mbox_chan_received_data(chan, NULL);
>   
>   	pcc_chan_acknowledge(pchan);
> @@ -490,6 +510,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,
>   };
>   
>   /**

Re: [PATCH v30 1/3] mailbox: pcc: Type3 Buffer handles ACK IRQ

[Sudeep Holla]

On Fri, Oct 17, 2025 at 12:08:07PM -0400, Adam Young wrote:
> This is obsoleted/duplicated by Sudeep's patch.  I am going to rebase on his
> patch series.
> 

Thanks for looking at them. Sorry we just cross posted each other last night.

-- 
Regards,
Sudeep

Re: [PATCH v30 1/3] mailbox: pcc: Type3 Buffer handles ACK IRQ

[Adam Young]

No need to apologize.  Thank you for writing them. And you caught 
several things I had overlooked.

I have tested the whole patch series with both my MCTP driver and the 
CPPC drivers.  I will post an updated driver (with fixes) in the next 
few days.

On 10/17/25 13:42, Sudeep Holla wrote:
> On Fri, Oct 17, 2025 at 12:08:07PM -0400, Adam Young wrote:
>> This is obsoleted/duplicated by Sudeep's patch.  I am going to rebase on his
>> patch series.
>>
> Thanks for looking at them. Sorry we just cross posted each other last night.
>

Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data

[Sudeep Holla]

On Thu, Oct 16, 2025 at 05:02:20PM -0400, Adam Young wrote:
> Adds functions that aid in compliance with the PCC protocol by
> checking the command complete flag status.
> 
> Adds a function that exposes the size of the shared buffer without
> activating the channel.
> 
> Adds a function that allows a client to query the number of bytes
> avaialbel to read in order to preallocate buffers for reading.
> 
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> ---
>  drivers/mailbox/pcc.c | 129 ++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/pcc.h    |  38 +++++++++++++
>  2 files changed, 167 insertions(+)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 978a7b674946..653897d61db5 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -367,6 +367,46 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static
> +struct pcc_chan_info *lookup_channel_info(int subspace_id)
> +{
> +	struct pcc_chan_info *pchan;
> +	struct mbox_chan *chan;
> +
> +	if (subspace_id < 0 || subspace_id >= pcc_chan_count)
> +		return ERR_PTR(-ENOENT);
> +
> +	pchan = chan_info + subspace_id;
> +	chan = pchan->chan.mchan;
> +	if (IS_ERR(chan) || chan->cl) {
> +		pr_err("Channel not found for idx: %d\n", subspace_id);
> +		return ERR_PTR(-EBUSY);
> +	}
> +	return pchan;
> +}
> +
> +/**
> + * pcc_mbox_buffer_size - PCC clients call this function to
> + *		request the size of the shared buffer in cases
> + *              where requesting the channel would prematurely
> + *              trigger channel activation and message delivery.
> + * @subspace_id: The PCC Subspace index as parsed in the PCC client
> + *		ACPI package. This is used to lookup the array of PCC
> + *		subspaces as parsed by the PCC Mailbox controller.
> + *
> + * Return: The size of the shared buffer.
> + */
> +int pcc_mbox_buffer_size(int index)
> +{
> +	struct pcc_chan_info *pchan = lookup_channel_info(index);
> +
> +	if (IS_ERR(pchan))
> +		return -1;
> +	return pchan->chan.shmem_size;
> +}
> +EXPORT_SYMBOL_GPL(pcc_mbox_buffer_size);
> +

Why do you need to export this when you can grab this from
struct pcc_mbox_chan which is returned from pcc_mbox_request_channel().

Please drop the above 2 functions completely.

> +
>  /**
>   * pcc_mbox_request_channel - PCC clients call this function to
>   *		request a pointer to their PCC subspace, from which they
> @@ -437,6 +477,95 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>  }
>  EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>  
> +/**
> + * pcc_mbox_query_bytes_available
> + *
> + * @pchan pointer to channel associated with buffer
> + * Return: the number of bytes available to read from the shared buffer
> + */
> +int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan)
> +{
> +	struct pcc_extended_header pcc_header;
> +	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
> +	int data_len;
> +	u64 val;
> +
> +	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
> +	if (val) {
> +		pr_info("%s Buffer not enabled for reading", __func__);
> +		return -1;
> +	}

Why would you call pcc_mbox_query_bytes_available() if the transfer is
not complete ?

> +	memcpy_fromio(&pcc_header, pchan->shmem,
> +		      sizeof(pcc_header));
> +	data_len = pcc_header.length - sizeof(u32) + sizeof(pcc_header);

Why are you adding the header size to the length above ?

> +	return data_len;
> +}
> +EXPORT_SYMBOL_GPL(pcc_mbox_query_bytes_available);
> +
> +/**
> + * pcc_mbox_read_from_buffer - Copy bytes from shared buffer into data
> + *
> + * @pchan - channel associated with the shared buffer
> + * @len - number of bytes to read
> + * @data - pointer to memory in which to write the data from the
> + *         shared buffer
> + *
> + * Return: number of bytes read and written into daa
> + */
> +int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
> +{
> +	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
> +	int data_len;
> +	u64 val;
> +
> +	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
> +	if (val) {
> +		pr_info("%s buffer not enabled for reading", __func__);
> +		return -1;
> +	}

Ditto as above, why is this check necessary ?

> +	data_len  = pcc_mbox_query_bytes_available(pchan);
> +	if (len < data_len)
> +		data_len = len;
> +	memcpy_fromio(data, pchan->shmem, len);
> +	return len;
> +}
> +EXPORT_SYMBOL_GPL(pcc_mbox_read_from_buffer);
> +
> +/**
> + * pcc_mbox_write_to_buffer, copy the contents of the data
> + * pointer to the shared buffer.  Confirms that the command
> + * flag has been set prior to writing.  Data should be a
> + * properly formatted extended data buffer.
> + * pcc_mbox_write_to_buffer
> + * @pchan: channel
> + * @len: Length of the overall buffer passed in, including the
> + *       Entire header. The length value in the shared buffer header
> + *       Will be calculated from len.
> + * @data: Client specific data to be written to the shared buffer.
> + * Return: number of bytes written to the buffer.
> + */
> +int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
> +{
> +	struct pcc_extended_header *pcc_header = data;
> +	struct mbox_chan *mbox_chan = pchan->mchan;
> +
> +	/*
> +	 * The PCC header length includes the command field
> +	 * but not the other values from the header.
> +	 */
> +	pcc_header->length = len - sizeof(struct pcc_extended_header) + sizeof(u32);
> +
> +	if (!pcc_last_tx_done(mbox_chan)) {
> +		pr_info("%s pchan->cmd_complete not set.", __func__);
> +		return 0;
> +	}

The mailbox moves to next message only if the last tx is done. Why is
this check necessary ?

> +	memcpy_toio(pchan->shmem,  data, len);
> +
> +	return len;
> +}
> +EXPORT_SYMBOL_GPL(pcc_mbox_write_to_buffer);
> +
> 

I am thinking if reading and writing to shmem can be made inline helper.
Let me try to hack up something add see how that would look like.

>  /**
>   * pcc_send_data - Called from Mailbox Controller code. Used
>   *		here only to ring the channel doorbell. The PCC client
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 840bfc95bae3..96a6f85fc1ba 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -19,6 +19,13 @@ struct pcc_mbox_chan {
>  	u16 min_turnaround_time;
>  };
>  
> +struct pcc_extended_header {
> +	u32 signature;
> +	u32 flags;
> +	u32 length;
> +	u32 command;
> +};
> +

This again is a duplicate of struct acpi_pcct_ext_pcc_shared_memory.
It can be dropped.

-- 
Regards,
Sudeep

Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data

[Adam Young]

Answers inline.  Thanks for the review.

On 10/20/25 08:52, Sudeep Holla wrote:
> On Thu, Oct 16, 2025 at 05:02:20PM -0400, Adam Young wrote:
>> Adds functions that aid in compliance with the PCC protocol by
>> checking the command complete flag status.
>>
>> Adds a function that exposes the size of the shared buffer without
>> activating the channel.
>>
>> Adds a function that allows a client to query the number of bytes
>> avaialbel to read in order to preallocate buffers for reading.
>>
>> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
>> ---
>>   drivers/mailbox/pcc.c | 129 ++++++++++++++++++++++++++++++++++++++++++
>>   include/acpi/pcc.h    |  38 +++++++++++++
>>   2 files changed, 167 insertions(+)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 978a7b674946..653897d61db5 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -367,6 +367,46 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> +static
>> +struct pcc_chan_info *lookup_channel_info(int subspace_id)
>> +{
>> +	struct pcc_chan_info *pchan;
>> +	struct mbox_chan *chan;
>> +
>> +	if (subspace_id < 0 || subspace_id >= pcc_chan_count)
>> +		return ERR_PTR(-ENOENT);
>> +
>> +	pchan = chan_info + subspace_id;
>> +	chan = pchan->chan.mchan;
>> +	if (IS_ERR(chan) || chan->cl) {
>> +		pr_err("Channel not found for idx: %d\n", subspace_id);
>> +		return ERR_PTR(-EBUSY);
>> +	}
>> +	return pchan;
>> +}
>> +
>> +/**
>> + * pcc_mbox_buffer_size - PCC clients call this function to
>> + *		request the size of the shared buffer in cases
>> + *              where requesting the channel would prematurely
>> + *              trigger channel activation and message delivery.
>> + * @subspace_id: The PCC Subspace index as parsed in the PCC client
>> + *		ACPI package. This is used to lookup the array of PCC
>> + *		subspaces as parsed by the PCC Mailbox controller.
>> + *
>> + * Return: The size of the shared buffer.
>> + */
>> +int pcc_mbox_buffer_size(int index)
>> +{
>> +	struct pcc_chan_info *pchan = lookup_channel_info(index);
>> +
>> +	if (IS_ERR(pchan))
>> +		return -1;
>> +	return pchan->chan.shmem_size;
>> +}
>> +EXPORT_SYMBOL_GPL(pcc_mbox_buffer_size);
>> +
> Why do you need to export this when you can grab this from
> struct pcc_mbox_chan which is returned from pcc_mbox_request_channel().
>
> Please drop the above 2 functions completely.\

This is required by the Network driver. Specifically, the network driver 
needs to tell the OS what the Max MTU size  is before the network is 
active.  If I have to call pcc_mbox_request_channel I then activate the 
channel for message delivery, and we have a race condition.

One alternative I did consider was to return all of the data that you 
get from  request channel is a non-active format.  For the type 2 
drivers, this information is available outside of  the mailbox 
interface.  The key effect is that the size of the shared message buffer 
be available without activating the channel.


>
>> +
>>   /**
>>    * pcc_mbox_request_channel - PCC clients call this function to
>>    *		request a pointer to their PCC subspace, from which they
>> @@ -437,6 +477,95 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>>   }
>>   EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>>   
>> +/**
>> + * pcc_mbox_query_bytes_available
>> + *
>> + * @pchan pointer to channel associated with buffer
>> + * Return: the number of bytes available to read from the shared buffer
>> + */
>> +int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan)
>> +{
>> +	struct pcc_extended_header pcc_header;
>> +	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
>> +	int data_len;
>> +	u64 val;
>> +
>> +	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
>> +	if (val) {
>> +		pr_info("%s Buffer not enabled for reading", __func__);
>> +		return -1;
>> +	}
> Why would you call pcc_mbox_query_bytes_available() if the transfer is
> not complete ?

Because I need to  allocate a buffer to read the bytes in to.  In the 
driver, it is called this way.

+       size = pcc_mbox_query_bytes_available(inbox->chan);
+       if (size == 0)
+               return;
+       skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
+       if (!skb) {
+               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
+               return;
+       }
+       skb_put(skb, size);
+       skb->protocol = htons(ETH_P_MCTP);
+       pcc_mbox_read_from_buffer(inbox->chan, size, skb->data);

While we could pre-allocate a sk_buff that is MTU size, that is likely 
to be wasteful for many messages.


>
>> +	memcpy_fromio(&pcc_header, pchan->shmem,
>> +		      sizeof(pcc_header));
>> +	data_len = pcc_header.length - sizeof(u32) + sizeof(pcc_header);
> Why are you adding the header size to the length above ?

Because the PCC spec is wonky.
https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#extended-pcc-subspace-shared-memory-region

"Length of payload being transmitted including command field."  Thus in 
order to copy all of the data, including  the PCC header, I need to drop 
the length (- sizeof(u32) ) and then add the entire header. Having all 
the PCC data in the buffer allows us to see it in networking tools. It 
is also parallel with how the messages are sent, where the PCC header is 
written by the driver and then the whole message is mem-copies in one 
io/read or write.

>
>> +	return data_len;
>> +}
>> +EXPORT_SYMBOL_GPL(pcc_mbox_query_bytes_available);
>> +
>> +/**
>> + * pcc_mbox_read_from_buffer - Copy bytes from shared buffer into data
>> + *
>> + * @pchan - channel associated with the shared buffer
>> + * @len - number of bytes to read
>> + * @data - pointer to memory in which to write the data from the
>> + *         shared buffer
>> + *
>> + * Return: number of bytes read and written into daa
>> + */
>> +int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
>> +{
>> +	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
>> +	int data_len;
>> +	u64 val;
>> +
>> +	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
>> +	if (val) {
>> +		pr_info("%s buffer not enabled for reading", __func__);
>> +		return -1;
>> +	}
> Ditto as above, why is this check necessary ?

Possibly just paranoia. I think this is vestige of older code that did 
polling instead of getting an interrupt.  But it seems correct in 
keeping with the letter of the PCC protocol.


>
>> +	data_len  = pcc_mbox_query_bytes_available(pchan);
>> +	if (len < data_len)
>> +		data_len = len;
>> +	memcpy_fromio(data, pchan->shmem, len);
>> +	return len;
>> +}
>> +EXPORT_SYMBOL_GPL(pcc_mbox_read_from_buffer);
>> +
>> +/**
>> + * pcc_mbox_write_to_buffer, copy the contents of the data
>> + * pointer to the shared buffer.  Confirms that the command
>> + * flag has been set prior to writing.  Data should be a
>> + * properly formatted extended data buffer.
>> + * pcc_mbox_write_to_buffer
>> + * @pchan: channel
>> + * @len: Length of the overall buffer passed in, including the
>> + *       Entire header. The length value in the shared buffer header
>> + *       Will be calculated from len.
>> + * @data: Client specific data to be written to the shared buffer.
>> + * Return: number of bytes written to the buffer.
>> + */
>> +int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
>> +{
>> +	struct pcc_extended_header *pcc_header = data;
>> +	struct mbox_chan *mbox_chan = pchan->mchan;
>> +
>> +	/*
>> +	 * The PCC header length includes the command field
>> +	 * but not the other values from the header.
>> +	 */
>> +	pcc_header->length = len - sizeof(struct pcc_extended_header) + sizeof(u32);
>> +
>> +	if (!pcc_last_tx_done(mbox_chan)) {
>> +		pr_info("%s pchan->cmd_complete not set.", __func__);
>> +		return 0;
>> +	}
> The mailbox moves to next message only if the last tx is done. Why is
> this check necessary ?

I think you are  right, and  these three checks are redundant now.


>
>> +	memcpy_toio(pchan->shmem,  data, len);
>> +
>> +	return len;
>> +}
>> +EXPORT_SYMBOL_GPL(pcc_mbox_write_to_buffer);
>> +
>>
> I am thinking if reading and writing to shmem can be made inline helper.
> Let me try to hack up something add see how that would look like.

That would be a good optimization.


>
>>   /**
>>    * pcc_send_data - Called from Mailbox Controller code. Used
>>    *		here only to ring the channel doorbell. The PCC client
>> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
>> index 840bfc95bae3..96a6f85fc1ba 100644
>> --- a/include/acpi/pcc.h
>> +++ b/include/acpi/pcc.h
>> @@ -19,6 +19,13 @@ struct pcc_mbox_chan {
>>   	u16 min_turnaround_time;
>>   };
>>   
>> +struct pcc_extended_header {
>> +	u32 signature;
>> +	u32 flags;
>> +	u32 length;
>> +	u32 command;
>> +};
>> +
> This again is a duplicate of struct acpi_pcct_ext_pcc_shared_memory.
> It can be dropped.

Will do.



>

Re: [PATCH v30 3/3] mctp pcc: Implement MCTP over PCC Transport

[Adam Young]

On 10/16/25 23:01, Jeremy Kerr wrote:
> Hi Adam,
>
> Looking pretty good, a few things inline:
>
>> diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
>> new file mode 100644
>> index 000000000000..927a525c1121
>> --- /dev/null
>> +++ b/drivers/net/mctp/mctp-pcc.c
>> @@ -0,0 +1,319 @@
>> +// 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/hrtimer.h>
>> +#include <linux/if_arp.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mailbox_client.h>
>> +#include <linux/module.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/skbuff.h>
>> +#include <linux/string.h>
>> +
>> +#include <acpi/acpi_bus.h>
>> +#include <acpi/acpi_drivers.h>
>> +#include <acpi/acrestyp.h>
>> +#include <acpi/actbl.h>
>> +#include <acpi/pcc.h>
>> +#include <net/mctp.h>
>> +#include <net/mctpdevice.h>
>> +
>> +#define MCTP_SIGNATURE          "MCTP"
>> +#define MCTP_SIGNATURE_LENGTH   (sizeof(MCTP_SIGNATURE) - 1)
>> +#define MCTP_MIN_MTU            68
>> +#define PCC_DWORD_TYPE          0x0c
>> +
>> +struct mctp_pcc_mailbox {
>> +       u32 index;
>> +       struct pcc_mbox_chan *chan;
>> +       struct mbox_client client;
>> +};
>> +
>> +/* The netdev structure. One of these per PCC adapter. */
>> +struct mctp_pcc_ndev {
>> +       struct net_device *ndev;
>> +       struct acpi_device *acpi_device;
>> +       struct mctp_pcc_mailbox inbox;
>> +       struct mctp_pcc_mailbox outbox;
>> +};
>> +
>> +static void mctp_pcc_client_rx_callback(struct mbox_client *cl, void *mssg)
>> +{
>> +       struct pcc_extended_header pcc_header;
>> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
>> +       struct mctp_pcc_mailbox *inbox;
>> +       struct mctp_skb_cb *cb;
>> +       struct sk_buff *skb;
>> +       int size;
>> +
>> +       mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, inbox.client);
>> +       inbox = &mctp_pcc_ndev->inbox;
>> +       size = pcc_mbox_query_bytes_available(inbox->chan);
>> +       if (size == 0)
>> +               return;
>> +       skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
>> +       if (!skb) {
>> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
>> +               return;
>> +       }
>> +       skb_put(skb, size);
>> +       skb->protocol = htons(ETH_P_MCTP);
>> +       pcc_mbox_read_from_buffer(inbox->chan, size, skb->data);
>> +       dev_dstats_rx_add(mctp_pcc_ndev->ndev, skb->len);
>> +       skb_reset_mac_header(skb);
>> +       skb_pull(skb, sizeof(pcc_header));
>> +       skb_reset_network_header(skb);
>> +       cb = __mctp_cb(skb);
>> +       cb->halen = 0;
>> +       netif_rx(skb);
>> +}
>> +
>> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> +       struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
>> +       struct pcc_extended_header *pcc_header;
>> +       int len = skb->len;
>> +       int rc;
>> +
>> +       rc = skb_cow_head(skb, sizeof(*pcc_header));
>> +       if (rc) {
>> +               dev_dstats_tx_dropped(ndev);
>> +               kfree_skb(skb);
>> +               return NETDEV_TX_OK;
>> +       }
>> +
>> +       pcc_header = skb_push(skb, sizeof(*pcc_header));
>> +       pcc_header->signature = PCC_SIGNATURE | mpnd->outbox.index;
>> +       pcc_header->flags = PCC_CMD_COMPLETION_NOTIFY;
>> +       memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
>> +       pcc_header->length = len + MCTP_SIGNATURE_LENGTH;
>> +       rc = mbox_send_message(mpnd->outbox.chan->mchan, skb);
>> +
>> +       if (rc < 0) {
>> +               netif_stop_queue(ndev);
>> +               return NETDEV_TX_BUSY;
>> +       }
> super minor: the grouping here (and a couple of other places) is a bit
> strange; try and keep the rc handler together with where rc is set, and
> the setup and send distinct. I'd typically do this as:
>
>         pcc_header = skb_push(skb, sizeof(*pcc_header));
>         pcc_header->signature = PCC_SIGNATURE | mpnd->outbox.index;
>         pcc_header->flags = PCC_CMD_COMPLETION_NOTIFY;
>         memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
>         pcc_header->length = len + MCTP_SIGNATURE_LENGTH;
>
>         rc = mbox_send_message(mpnd->outbox.chan->mchan, skb);
>         if (rc < 0) {
>                 netif_stop_queue(ndev);
>                 return NETDEV_TX_BUSY;
>         }

OK I can see  the rationale.  I do find that easier to read.


>
>> +
>> +       dev_dstats_tx_add(ndev, len);
>> +       return NETDEV_TX_OK;
>> +}
>> +
>> +static void mctp_pcc_tx_prepare(struct mbox_client *cl, void *mssg)
>> +{
>> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
>> +       struct mctp_pcc_mailbox *outbox;
>> +       struct sk_buff *skb = mssg;
>> +       int len_sent;
>> +
>> +       mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, outbox.client);
>> +       outbox = &mctp_pcc_ndev->outbox;
>> +
>> +       if (!skb)
>> +               return;
>> +
>> +       len_sent = pcc_mbox_write_to_buffer(outbox->chan, skb->len, skb->data);
>> +       if (len_sent == 0)
>> +               pr_info("packet dropped");
> You probably don't want a bare pr_info() in the failure path here;
> either something debug level, or ratelimited.
>
> and probably use a netdev-specific log function (netdev_dbg() perhaps),
> so you get the device information too.
>
> I'm not super clear on this failure mode, do you need to update the
> tx statistics on the drop here? In which case, you may not need the
> _dbg/_info message at all.

Honestly, this is a pretty bad case, unlike, say, a UDP packet drop, 
this implies that something in the system is very much not aligned with 
the users expectations.  I can go with a netdev specific, but I think 
this should be info level at least.


>
>> +}
>> +
>> +static void mctp_pcc_tx_done(struct mbox_client *c, void *mssg, int r)
>> +{
>> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
>> +       struct mctp_pcc_mailbox *outbox;
>> +       struct sk_buff *skb = mssg;
> Nice, no more list lookups.
Yep.
>
>> +
>> +       mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, outbox.client);
>> +       outbox = container_of(c, struct mctp_pcc_mailbox, client);
>> +       if (skb)
>> +               dev_consume_skb_any(skb);
> minor: dev_consume_skb_any() will tolerate a null argument, you can
> leave out the conditional here.
OK
>
>> +       netif_wake_queue(mctp_pcc_ndev->ndev);
>> +}
>> +
>> +static int mctp_pcc_ndo_open(struct net_device *ndev)
>> +{
>> +       struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
>> +       struct mctp_pcc_mailbox *outbox, *inbox;
>> +
>> +       outbox = &mctp_pcc_ndev->outbox;
>> +       inbox = &mctp_pcc_ndev->inbox;
>> +
>> +       outbox->chan = pcc_mbox_request_channel(&outbox->client, outbox->index);
>> +       if (IS_ERR(outbox->chan))
>> +               return PTR_ERR(outbox->chan);
>> +
>> +       inbox->client.rx_callback = mctp_pcc_client_rx_callback;
>> +       inbox->chan = pcc_mbox_request_channel(&inbox->client, inbox->index);
>> +       if (IS_ERR(inbox->chan)) {
>> +               pcc_mbox_free_channel(outbox->chan);
>> +               return PTR_ERR(inbox->chan);
>> +       }
>> +       return 0;
>> +}
> Having the channels fully set-up before calling request_channel looks
> much safer now :)

This actually showed that the same problem would have happened in 
setting MTU.  It is why we need and accessor to the shared buffer size.


>
>> +
>> +static int mctp_pcc_ndo_stop(struct net_device *ndev)
>> +{
>> +       struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
>> +
>> +       pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
>> +       pcc_mbox_free_channel(mctp_pcc_ndev->inbox.chan);
>> +       return 0;
>> +}
>> +
>> +static const struct net_device_ops mctp_pcc_netdev_ops = {
>> +       .ndo_open = mctp_pcc_ndo_open,
>> +       .ndo_stop = mctp_pcc_ndo_stop,
>> +       .ndo_start_xmit = mctp_pcc_tx,
>> +};
>> +
>> +static void mctp_pcc_setup(struct net_device *ndev)
>> +{
>> +       ndev->type = ARPHRD_MCTP;
>> +       ndev->hard_header_len = 0;
> Isn't this sizeof(struct pcc_extended_header) ?
Yes it is.
>
>> +       ndev->tx_queue_len = 0;
>> +       ndev->flags = IFF_NOARP;
>> +       ndev->netdev_ops = &mctp_pcc_netdev_ops;
>> +       ndev->needs_free_netdev = true;
>> +       ndev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
>> +}
>> +
>> +struct mctp_pcc_lookup_context {
>> +       int index;
>> +       u32 inbox_index;
>> +       u32 outbox_index;
>> +};
>> +
>> +static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
>> +                                      void *context)
>> +{
>> +       struct mctp_pcc_lookup_context *luc = context;
>> +       struct acpi_resource_address32 *addr;
>> +
>> +       if (ares->type != PCC_DWORD_TYPE)
>> +               return AE_OK;
>> +
>> +       addr = ACPI_CAST_PTR(struct acpi_resource_address32, &ares->data);
>> +       switch (luc->index) {
>> +       case 0:
>> +               luc->outbox_index = addr[0].address.minimum;
>> +               break;
>> +       case 1:
>> +               luc->inbox_index = addr[0].address.minimum;
>> +               break;
>> +       }
>> +       luc->index++;
>> +       return AE_OK;
>> +}
>> +
>> +static void mctp_cleanup_netdev(void *data)
>> +{
>> +       struct net_device *ndev = data;
>> +
>> +       mctp_unregister_netdev(ndev);
>> +}
>> +
>> +static int initialize_MTU(struct net_device *ndev)
>> +{
>> +       struct mctp_pcc_ndev *mctp_pcc_ndev = netdev_priv(ndev);
>> +       struct mctp_pcc_mailbox *outbox;
>> +       int mctp_pcc_mtu;
>> +
>> +       outbox = &mctp_pcc_ndev->outbox;
>> +       mctp_pcc_mtu = pcc_mbox_buffer_size(outbox->index);
>> +       if (mctp_pcc_mtu == -1)
>> +               return -1;
> You may want to check that this is at least
> sizeof(struct pcc_extended_header), to avoid an underflow below, and
> possibly at least MCTP_MIN_MTU after subtracting the header size (as
> this would be less than the MCTP BTU)

And if it is?  I think that is a sign of a hardware problem, beyond the 
scope of what we can fix here.  No matter what, the interface will be 
unusable.



>
>> +
>> +       mctp_pcc_mtu = mctp_pcc_mtu - sizeof(struct pcc_extended_header);
>> +       mctp_pcc_ndev = netdev_priv(ndev);
> unneeded?
Yep, and gone....
>
>> +       ndev->mtu = MCTP_MIN_MTU;
>> +       ndev->max_mtu = mctp_pcc_mtu;
>> +       ndev->min_mtu = MCTP_MIN_MTU;
>> +
>> +       return 0;
>> +}
> Now that the initialize_MTU implementation is simpler, you may want to
> reinstate it inline in driver_add(), but also fine as-is.
>
>> +
>> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
>> +{
>> +       struct mctp_pcc_lookup_context context = {0};
>> +       struct mctp_pcc_ndev *mctp_pcc_ndev;
>> +       struct device *dev = &acpi_dev->dev;
>> +       struct net_device *ndev;
>> +       acpi_handle dev_handle;
>> +       acpi_status status;
>> +       char name[32];
>> +       int rc;
>> +
>> +       dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
>> +               acpi_device_hid(acpi_dev));
>> +       dev_handle = acpi_device_handle(acpi_dev);
>> +       status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
>> +                                    &context);
>> +       if (!ACPI_SUCCESS(status)) {
>> +               dev_err(dev, "FAILED to lookup PCC indexes from CRS\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       snprintf(name, sizeof(name), "mctppcc%d", context.inbox_index);
>> +       ndev = alloc_netdev(sizeof(*mctp_pcc_ndev), name, NET_NAME_PREDICTABLE,
>> +                           mctp_pcc_setup);
>> +       if (!ndev)
>> +               return -ENOMEM;
>> +
>> +       mctp_pcc_ndev = netdev_priv(ndev);
>> +
>> +       mctp_pcc_ndev->inbox.index = context.inbox_index;
>> +       mctp_pcc_ndev->inbox.client.dev = dev;
>> +       mctp_pcc_ndev->outbox.index = context.outbox_index;
>> +       mctp_pcc_ndev->outbox.client.dev = dev;
>> +
>> +       mctp_pcc_ndev->outbox.client.tx_prepare = mctp_pcc_tx_prepare;
>> +       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;
>> +
>> +       rc = initialize_MTU(ndev);
>> +       if (rc)
>> +               goto free_netdev;
>> +
>> +       rc = mctp_register_netdev(ndev, NULL, 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>");
> Cheers,
>
>
> Jeremy

Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data

[Sudeep Holla]

On Mon, Oct 20, 2025 at 01:22:23PM -0400, Adam Young wrote:
> Answers inline.  Thanks for the review.
> 
> On 10/20/25 08:52, Sudeep Holla wrote:
> > On Thu, Oct 16, 2025 at 05:02:20PM -0400, Adam Young wrote:
> > > Adds functions that aid in compliance with the PCC protocol by
> > > checking the command complete flag status.
> > > 
> > > Adds a function that exposes the size of the shared buffer without
> > > activating the channel.
> > > 
> > > Adds a function that allows a client to query the number of bytes
> > > avaialbel to read in order to preallocate buffers for reading.
> > > 
> > > Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
> > > ---
> > >   drivers/mailbox/pcc.c | 129 ++++++++++++++++++++++++++++++++++++++++++
> > >   include/acpi/pcc.h    |  38 +++++++++++++
> > >   2 files changed, 167 insertions(+)
> > > 
> > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> > > index 978a7b674946..653897d61db5 100644
> > > --- a/drivers/mailbox/pcc.c
> > > +++ b/drivers/mailbox/pcc.c
> > > @@ -367,6 +367,46 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
> > >   	return IRQ_HANDLED;
> > >   }
> > > +static
> > > +struct pcc_chan_info *lookup_channel_info(int subspace_id)
> > > +{
> > > +	struct pcc_chan_info *pchan;
> > > +	struct mbox_chan *chan;
> > > +
> > > +	if (subspace_id < 0 || subspace_id >= pcc_chan_count)
> > > +		return ERR_PTR(-ENOENT);
> > > +
> > > +	pchan = chan_info + subspace_id;
> > > +	chan = pchan->chan.mchan;
> > > +	if (IS_ERR(chan) || chan->cl) {
> > > +		pr_err("Channel not found for idx: %d\n", subspace_id);
> > > +		return ERR_PTR(-EBUSY);
> > > +	}
> > > +	return pchan;
> > > +}
> > > +
> > > +/**
> > > + * pcc_mbox_buffer_size - PCC clients call this function to
> > > + *		request the size of the shared buffer in cases
> > > + *              where requesting the channel would prematurely
> > > + *              trigger channel activation and message delivery.
> > > + * @subspace_id: The PCC Subspace index as parsed in the PCC client
> > > + *		ACPI package. This is used to lookup the array of PCC
> > > + *		subspaces as parsed by the PCC Mailbox controller.
> > > + *
> > > + * Return: The size of the shared buffer.
> > > + */
> > > +int pcc_mbox_buffer_size(int index)
> > > +{
> > > +	struct pcc_chan_info *pchan = lookup_channel_info(index);
> > > +
> > > +	if (IS_ERR(pchan))
> > > +		return -1;
> > > +	return pchan->chan.shmem_size;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pcc_mbox_buffer_size);
> > > +
> > Why do you need to export this when you can grab this from
> > struct pcc_mbox_chan which is returned from pcc_mbox_request_channel().
> > 
> > Please drop the above 2 functions completely.\
> 
> This is required by the Network driver. Specifically, the network driver
> needs to tell the OS what the Max MTU size  is before the network is
> active.  If I have to call pcc_mbox_request_channel I then activate the
> channel for message delivery, and we have a race condition.
>

No you just need to establish the channel by calling pcc_mbox_request_channel()
from probe or init routines. After that the shmem size should be available.
No need to send any message or activating anything.

> One alternative I did consider was to return all of the data that you get
> from  request channel is a non-active format.  For the type 2 drivers, this
> information is available outside of  the mailbox interface.  The key effect
> is that the size of the shared message buffer be available without
> activating the channel.
> 

Not sure if that is needed.

> 
> > 
> > > +
> > >   /**
> > >    * pcc_mbox_request_channel - PCC clients call this function to
> > >    *		request a pointer to their PCC subspace, from which they
> > > @@ -437,6 +477,95 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
> > >   }
> > >   EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
> > > +/**
> > > + * pcc_mbox_query_bytes_available
> > > + *
> > > + * @pchan pointer to channel associated with buffer
> > > + * Return: the number of bytes available to read from the shared buffer
> > > + */
> > > +int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan)
> > > +{
> > > +	struct pcc_extended_header pcc_header;
> > > +	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
> > > +	int data_len;
> > > +	u64 val;
> > > +
> > > +	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
> > > +	if (val) {
> > > +		pr_info("%s Buffer not enabled for reading", __func__);
> > > +		return -1;
> > > +	}
> > Why would you call pcc_mbox_query_bytes_available() if the transfer is
> > not complete ?
> 
> Because I need to  allocate a buffer to read the bytes in to.  In the
> driver, it is called this way.
> 

Yes I thought so, I think we must be able to manage this with helper as well.
I will try out some things and share.

> +       size = pcc_mbox_query_bytes_available(inbox->chan);
> +       if (size == 0)
> +               return;
> +       skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
> +       if (!skb) {
> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +               return;
> +       }
> +       skb_put(skb, size);
> +       skb->protocol = htons(ETH_P_MCTP);
> +       pcc_mbox_read_from_buffer(inbox->chan, size, skb->data);
> 
> While we could pre-allocate a sk_buff that is MTU size, that is likely to be
> wasteful for many messages.
> 

Fair enough.

> > 
> > > +	memcpy_fromio(&pcc_header, pchan->shmem,
> > > +		      sizeof(pcc_header));
> > > +	data_len = pcc_header.length - sizeof(u32) + sizeof(pcc_header);
> > Why are you adding the header size to the length above ?
> 
> Because the PCC spec is wonky.
> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#extended-pcc-subspace-shared-memory-region
> 
> "Length of payload being transmitted including command field."  Thus in
> order to copy all of the data, including  the PCC header, I need to drop the
> length (- sizeof(u32) ) and then add the entire header. Having all the PCC
> data in the buffer allows us to see it in networking tools. It is also
> parallel with how the messages are sent, where the PCC header is written by
> the driver and then the whole message is mem-copies in one io/read or write.
> 

No you have misread this part.
Communication subspace(only part and last entry in shared memory at offset of
16 bytes) - "Memory region for reading/writing PCC data. The maximum size of
this region is 16 bytes smaller than the size of the shared memory region
(specified in the Master slave Communications Subspace structure). When a
command is sent to or received from the platform, the size of the data in
this space will be Length (expressed above) minus the 4 bytes taken up by
the command."

The keyword is "this space/region" which refers to only the communication
subspace which is at offset 16 bytes in the shmem.

It should be just length - sizeof(command) i.e. length - 4

> > 
> > > +	return data_len;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pcc_mbox_query_bytes_available);
> > > +
> > > +/**
> > > + * pcc_mbox_read_from_buffer - Copy bytes from shared buffer into data
> > > + *
> > > + * @pchan - channel associated with the shared buffer
> > > + * @len - number of bytes to read
> > > + * @data - pointer to memory in which to write the data from the
> > > + *         shared buffer
> > > + *
> > > + * Return: number of bytes read and written into daa
> > > + */
> > > +int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
> > > +{
> > > +	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
> > > +	int data_len;
> > > +	u64 val;
> > > +
> > > +	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
> > > +	if (val) {
> > > +		pr_info("%s buffer not enabled for reading", __func__);
> > > +		return -1;
> > > +	}
> > Ditto as above, why is this check necessary ?
> 
> Possibly just paranoia. I think this is vestige of older code that did
> polling instead of getting an interrupt.  But it seems correct in keeping
> with the letter of the PCC protocol.

Not needed IMO, lets add when we find the need for it, not for paranoia
reasons please.

> 
> > 
> > > +	data_len  = pcc_mbox_query_bytes_available(pchan);
> > > +	if (len < data_len)
> > > +		data_len = len;
> > > +	memcpy_fromio(data, pchan->shmem, len);
> > > +	return len;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pcc_mbox_read_from_buffer);
> > > +
> > > +/**
> > > + * pcc_mbox_write_to_buffer, copy the contents of the data
> > > + * pointer to the shared buffer.  Confirms that the command
> > > + * flag has been set prior to writing.  Data should be a
> > > + * properly formatted extended data buffer.
> > > + * pcc_mbox_write_to_buffer
> > > + * @pchan: channel
> > > + * @len: Length of the overall buffer passed in, including the
> > > + *       Entire header. The length value in the shared buffer header
> > > + *       Will be calculated from len.
> > > + * @data: Client specific data to be written to the shared buffer.
> > > + * Return: number of bytes written to the buffer.
> > > + */
> > > +int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
> > > +{
> > > +	struct pcc_extended_header *pcc_header = data;
> > > +	struct mbox_chan *mbox_chan = pchan->mchan;
> > > +
> > > +	/*
> > > +	 * The PCC header length includes the command field
> > > +	 * but not the other values from the header.
> > > +	 */
> > > +	pcc_header->length = len - sizeof(struct pcc_extended_header) + sizeof(u32);
> > > +
> > > +	if (!pcc_last_tx_done(mbox_chan)) {
> > > +		pr_info("%s pchan->cmd_complete not set.", __func__);
> > > +		return 0;
> > > +	}
> > The mailbox moves to next message only if the last tx is done. Why is
> > this check necessary ?
> 
> I think you are  right, and  these three checks are redundant now.
> 

Thanks for confirming my understanding, was just worried if there is
anything that I am not considering.

> 
> > 
> > > +	memcpy_toio(pchan->shmem,  data, len);
> > > +
> > > +	return len;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pcc_mbox_write_to_buffer);
> > > +
> > > 
> > I am thinking if reading and writing to shmem can be made inline helper.
> > Let me try to hack up something add see how that would look like.
> 
> That would be a good optimization.
> 

Thanks, I did try to write to buffer part but I am still not decided on
the exact formating yet to share it. I will try to share something in
next couple of days if possible.

-- 
Regards,
Sudeep

Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data

[Adam Young]

On 10/21/25 10:02, Sudeep Holla wrote:
> On Mon, Oct 20, 2025 at 01:22:23PM -0400, Adam Young wrote:
>> Answers inline.  Thanks for the review.
>>
>> On 10/20/25 08:52, Sudeep Holla wrote:
>>> On Thu, Oct 16, 2025 at 05:02:20PM -0400, Adam Young wrote:
>>>> Adds functions that aid in compliance with the PCC protocol by
>>>> checking the command complete flag status.
>>>>
>>>> Adds a function that exposes the size of the shared buffer without
>>>> activating the channel.
>>>>
>>>> Adds a function that allows a client to query the number of bytes
>>>> avaialbel to read in order to preallocate buffers for reading.
>>>>
>>>> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
>>>> ---
>>>>    drivers/mailbox/pcc.c | 129 ++++++++++++++++++++++++++++++++++++++++++
>>>>    include/acpi/pcc.h    |  38 +++++++++++++
>>>>    2 files changed, 167 insertions(+)
>>>>
>>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>>> index 978a7b674946..653897d61db5 100644
>>>> --- a/drivers/mailbox/pcc.c
>>>> +++ b/drivers/mailbox/pcc.c
>>>> @@ -367,6 +367,46 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>>>    	return IRQ_HANDLED;
>>>>    }
>>>> +static
>>>> +struct pcc_chan_info *lookup_channel_info(int subspace_id)
>>>> +{
>>>> +	struct pcc_chan_info *pchan;
>>>> +	struct mbox_chan *chan;
>>>> +
>>>> +	if (subspace_id < 0 || subspace_id >= pcc_chan_count)
>>>> +		return ERR_PTR(-ENOENT);
>>>> +
>>>> +	pchan = chan_info + subspace_id;
>>>> +	chan = pchan->chan.mchan;
>>>> +	if (IS_ERR(chan) || chan->cl) {
>>>> +		pr_err("Channel not found for idx: %d\n", subspace_id);
>>>> +		return ERR_PTR(-EBUSY);
>>>> +	}
>>>> +	return pchan;
>>>> +}
>>>> +
>>>> +/**
>>>> + * pcc_mbox_buffer_size - PCC clients call this function to
>>>> + *		request the size of the shared buffer in cases
>>>> + *              where requesting the channel would prematurely
>>>> + *              trigger channel activation and message delivery.
>>>> + * @subspace_id: The PCC Subspace index as parsed in the PCC client
>>>> + *		ACPI package. This is used to lookup the array of PCC
>>>> + *		subspaces as parsed by the PCC Mailbox controller.
>>>> + *
>>>> + * Return: The size of the shared buffer.
>>>> + */
>>>> +int pcc_mbox_buffer_size(int index)
>>>> +{
>>>> +	struct pcc_chan_info *pchan = lookup_channel_info(index);
>>>> +
>>>> +	if (IS_ERR(pchan))
>>>> +		return -1;
>>>> +	return pchan->chan.shmem_size;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pcc_mbox_buffer_size);
>>>> +
>>> Why do you need to export this when you can grab this from
>>> struct pcc_mbox_chan which is returned from pcc_mbox_request_channel().
>>>
>>> Please drop the above 2 functions completely.\
>> This is required by the Network driver. Specifically, the network driver
>> needs to tell the OS what the Max MTU size  is before the network is
>> active.  If I have to call pcc_mbox_request_channel I then activate the
>> channel for message delivery, and we have a race condition.
>>
> No you just need to establish the channel by calling pcc_mbox_request_channel()
> from probe or init routines. After that the shmem size should be available.
> No need to send any message or activating anything.

I guess I can get away with that if I only do it for the type 3...that 
should not immediately send an interrupt.  I was thinking that the type 
4 could have messages queued up already, and when I request the channel, 
I get a flood that I am not ready for.

Ok, I think I can remove the function.



>
>> One alternative I did consider was to return all of the data that you get
>> from  request channel is a non-active format.  For the type 2 drivers, this
>> information is available outside of  the mailbox interface.  The key effect
>> is that the size of the shared message buffer be available without
>> activating the channel.
>>
> Not sure if that is needed.

Not needed.


>
>>>> +
>>>>    /**
>>>>     * pcc_mbox_request_channel - PCC clients call this function to
>>>>     *		request a pointer to their PCC subspace, from which they
>>>> @@ -437,6 +477,95 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>>>> +/**
>>>> + * pcc_mbox_query_bytes_available
>>>> + *
>>>> + * @pchan pointer to channel associated with buffer
>>>> + * Return: the number of bytes available to read from the shared buffer
>>>> + */
>>>> +int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan)
>>>> +{
>>>> +	struct pcc_extended_header pcc_header;
>>>> +	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
>>>> +	int data_len;
>>>> +	u64 val;
>>>> +
>>>> +	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
>>>> +	if (val) {
>>>> +		pr_info("%s Buffer not enabled for reading", __func__);
>>>> +		return -1;
>>>> +	}
>>> Why would you call pcc_mbox_query_bytes_available() if the transfer is
>>> not complete ?
>> Because I need to  allocate a buffer to read the bytes in to.  In the
>> driver, it is called this way.
>>
> Yes I thought so, I think we must be able to manage this with helper as well.
> I will try out some things and share.
>
>> +       size = pcc_mbox_query_bytes_available(inbox->chan);
>> +       if (size == 0)
>> +               return;
>> +       skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
>> +       if (!skb) {
>> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
>> +               return;
>> +       }
>> +       skb_put(skb, size);
>> +       skb->protocol = htons(ETH_P_MCTP);
>> +       pcc_mbox_read_from_buffer(inbox->chan, size, skb->data);
>>
>> While we could pre-allocate a sk_buff that is MTU size, that is likely to be
>> wasteful for many messages.
>>
> Fair enough.
>
>>>> +	memcpy_fromio(&pcc_header, pchan->shmem,
>>>> +		      sizeof(pcc_header));
>>>> +	data_len = pcc_header.length - sizeof(u32) + sizeof(pcc_header);
>>> Why are you adding the header size to the length above ?
>> Because the PCC spec is wonky.
>> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#extended-pcc-subspace-shared-memory-region
>>
>> "Length of payload being transmitted including command field."  Thus in
>> order to copy all of the data, including  the PCC header, I need to drop the
>> length (- sizeof(u32) ) and then add the entire header. Having all the PCC
>> data in the buffer allows us to see it in networking tools. It is also
>> parallel with how the messages are sent, where the PCC header is written by
>> the driver and then the whole message is mem-copies in one io/read or write.
>>
> No you have misread this part.
> Communication subspace(only part and last entry in shared memory at offset of
> 16 bytes) - "Memory region for reading/writing PCC data. The maximum size of
> this region is 16 bytes smaller than the size of the shared memory region
> (specified in the Master slave Communications Subspace structure). When a
> command is sent to or received from the platform, the size of the data in
> this space will be Length (expressed above) minus the 4 bytes taken up by
> the command."
>
> The keyword is "this space/region" which refers to only the communication
> subspace which is at offset 16 bytes in the shmem.
>
> It should be just length - sizeof(command) i.e. length - 4


I just want to make sure I have this correct.  I want to copy the entire 
PCC buffer, not just the payload, into the sk_buff.  If I wanted the 
payload, I would use the length field.  However, I want the PCC header 
as well, which is the length field, plus sizeof (header).  But that 
double counts the command field, which is part of the header, and thus I 
subtract this out.  I think my math is correct. What you wrote would be 
for the case where I want only the PCC payload.

The giveaway above is the "offset 16 bytes." As this is the size of the 
header.



>
>>>> +	return data_len;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pcc_mbox_query_bytes_available);
>>>> +
>>>> +/**
>>>> + * pcc_mbox_read_from_buffer - Copy bytes from shared buffer into data
>>>> + *
>>>> + * @pchan - channel associated with the shared buffer
>>>> + * @len - number of bytes to read
>>>> + * @data - pointer to memory in which to write the data from the
>>>> + *         shared buffer
>>>> + *
>>>> + * Return: number of bytes read and written into daa
>>>> + */
>>>> +int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
>>>> +{
>>>> +	struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
>>>> +	int data_len;
>>>> +	u64 val;
>>>> +
>>>> +	pcc_chan_reg_read(&pinfo->cmd_complete, &val);
>>>> +	if (val) {
>>>> +		pr_info("%s buffer not enabled for reading", __func__);
>>>> +		return -1;
>>>> +	}
>>> Ditto as above, why is this check necessary ?
>> Possibly just paranoia. I think this is vestige of older code that did
>> polling instead of getting an interrupt.  But it seems correct in keeping
>> with the letter of the PCC protocol.
> Not needed IMO, lets add when we find the need for it, not for paranoia
> reasons please.

Will remove.  I think it is safely checked  by the pcc mailbox.


>>>> +	data_len  = pcc_mbox_query_bytes_available(pchan);
>>>> +	if (len < data_len)
>>>> +		data_len = len;
>>>> +	memcpy_fromio(data, pchan->shmem, len);
>>>> +	return len;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pcc_mbox_read_from_buffer);
>>>> +
>>>> +/**
>>>> + * pcc_mbox_write_to_buffer, copy the contents of the data
>>>> + * pointer to the shared buffer.  Confirms that the command
>>>> + * flag has been set prior to writing.  Data should be a
>>>> + * properly formatted extended data buffer.
>>>> + * pcc_mbox_write_to_buffer
>>>> + * @pchan: channel
>>>> + * @len: Length of the overall buffer passed in, including the
>>>> + *       Entire header. The length value in the shared buffer header
>>>> + *       Will be calculated from len.
>>>> + * @data: Client specific data to be written to the shared buffer.
>>>> + * Return: number of bytes written to the buffer.
>>>> + */
>>>> +int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
>>>> +{
>>>> +	struct pcc_extended_header *pcc_header = data;
>>>> +	struct mbox_chan *mbox_chan = pchan->mchan;
>>>> +
>>>> +	/*
>>>> +	 * The PCC header length includes the command field
>>>> +	 * but not the other values from the header.
>>>> +	 */
>>>> +	pcc_header->length = len - sizeof(struct pcc_extended_header) + sizeof(u32);
>>>> +
>>>> +	if (!pcc_last_tx_done(mbox_chan)) {
>>>> +		pr_info("%s pchan->cmd_complete not set.", __func__);
>>>> +		return 0;
>>>> +	}
>>> The mailbox moves to next message only if the last tx is done. Why is
>>> this check necessary ?
>> I think you are  right, and  these three checks are redundant now.
>>
> Thanks for confirming my understanding, was just worried if there is
> anything that I am not considering.
>
>>>> +	memcpy_toio(pchan->shmem,  data, len);
>>>> +
>>>> +	return len;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pcc_mbox_write_to_buffer);
>>>> +
>>>>
>>> I am thinking if reading and writing to shmem can be made inline helper.
>>> Let me try to hack up something add see how that would look like.
>> That would be a good optimization.
>>
> Thanks, I did try to write to buffer part but I am still not decided on
> the exact formating yet to share it. I will try to share something in
> next couple of days if possible.


Much appreciated.  I will hold off on resubmitting until you do.

Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data

[Sudeep Holla]

On Tue, Oct 21, 2025 at 01:20:50PM -0400, Adam Young wrote:

[...]

> > > Because the PCC spec is wonky.
> > > https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#extended-pcc-subspace-shared-memory-region
> > > 
> > > "Length of payload being transmitted including command field."  Thus in
> > > order to copy all of the data, including  the PCC header, I need to drop the
> > > length (- sizeof(u32) ) and then add the entire header. Having all the PCC
> > > data in the buffer allows us to see it in networking tools. It is also
> > > parallel with how the messages are sent, where the PCC header is written by
> > > the driver and then the whole message is mem-copies in one io/read or write.
> > > 
> > No you have misread this part.
> > Communication subspace(only part and last entry in shared memory at offset of
> > 16 bytes) - "Memory region for reading/writing PCC data. The maximum size of
> > this region is 16 bytes smaller than the size of the shared memory region
> > (specified in the Master slave Communications Subspace structure). When a
> > command is sent to or received from the platform, the size of the data in
> > this space will be Length (expressed above) minus the 4 bytes taken up by
> > the command."
> > 
> > The keyword is "this space/region" which refers to only the communication
> > subspace which is at offset 16 bytes in the shmem.
> > 
> > It should be just length - sizeof(command) i.e. length - 4
> 
> 
> I just want to make sure I have this correct.  I want to copy the entire PCC
> buffer, not just the payload, into the sk_buff.  If I wanted the payload, I
> would use the length field.  However, I want the PCC header as well, which
> is the length field, plus sizeof (header).  But that double counts the
> command field, which is part of the header, and thus I subtract this out.  I
> think my math is correct. What you wrote would be for the case where I want
> only the PCC payload.
> 

Why ? How does sk_buff interpret that as PCC header. Something doesn't align
well here or I might be missing something.

I started writing some helpers and this comment made me to rethink my
approach. I don't have any to share and I will be away for a while. I will
try to review any further changes from you but expect delays.

-- 
Regards,
Sudeep

Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data

[Adam Young]

On 10/24/25 09:50, Sudeep Holla wrote:
> On Tue, Oct 21, 2025 at 01:20:50PM -0400, Adam Young wrote:
>
> [...]
>
>>>> Because the PCC spec is wonky.
>>>> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#extended-pcc-subspace-shared-memory-region
>>>>
>>>> "Length of payload being transmitted including command field."  Thus in
>>>> order to copy all of the data, including  the PCC header, I need to drop the
>>>> length (- sizeof(u32) ) and then add the entire header. Having all the PCC
>>>> data in the buffer allows us to see it in networking tools. It is also
>>>> parallel with how the messages are sent, where the PCC header is written by
>>>> the driver and then the whole message is mem-copies in one io/read or write.
>>>>
>>> No you have misread this part.
>>> Communication subspace(only part and last entry in shared memory at offset of
>>> 16 bytes) - "Memory region for reading/writing PCC data. The maximum size of
>>> this region is 16 bytes smaller than the size of the shared memory region
>>> (specified in the Master slave Communications Subspace structure). When a
>>> command is sent to or received from the platform, the size of the data in
>>> this space will be Length (expressed above) minus the 4 bytes taken up by
>>> the command."
>>>
>>> The keyword is "this space/region" which refers to only the communication
>>> subspace which is at offset 16 bytes in the shmem.
>>>
>>> It should be just length - sizeof(command) i.e. length - 4
>>
>> I just want to make sure I have this correct.  I want to copy the entire PCC
>> buffer, not just the payload, into the sk_buff.  If I wanted the payload, I
>> would use the length field.  However, I want the PCC header as well, which
>> is the length field, plus sizeof (header).  But that double counts the
>> command field, which is part of the header, and thus I subtract this out.  I
>> think my math is correct. What you wrote would be for the case where I want
>> only the PCC payload.
>>
> Why ? How does sk_buff interpret that as PCC header. Something doesn't align
> well here or I might be missing something.
>
> I started writing some helpers and this comment made me to rethink my
> approach. I don't have any to share and I will be away for a while. I will
> try to review any further changes from you but expect delays.

The sk_buff and socket api allows you to specify the various levels of 
headers for a nested protocol.  Just like udp inside IP inside Ethernet 
is a thing, the packets here are mctp inside pcc. The the networking 
stack can look into the packet and pull out the MCTP information when 
routing the packet is routed to the link device.

Since the mctp over pcc driver is PCC specific, I want to be able to see 
the PCC header in a tool like wireshark.  If we only return the MCTP 
portion, we will lose some tracing information. Admittedly, not a lot.

This is fairly well tested in my code base:  I can send and receive MCTP 
messages through the Linux kernel network stack.

The header has the flags, length, and command blocks.  I think we agree 
that PCC mailbox should not hardcode  the command.  The question, then 
is if the PCC layer should be responsible for the signature and header.  
The alternative is that the read/write functions will handle the PCC 
header information.  I think the only real drawback to this would be if 
the driver wanted to be able to set the flags.  For now the only flag 
that is defined is PCC_CMD_COMPLETION_NOTIFY, but this is a driver 
specific decision, and needs  to be accounted for.

So I would like to keep the signatures of the read/write functions as 
is.  We can inline them if you think that there is benefit to it:  as I 
see it, it exposes more internals but reduces the number of external 
symbols for the PCC Mailbox.  This is your call as the maintainer, and I 
can make it work either way.  I would like to submit an updated driver.