[PATCH net-next v20 0/1] MCTP Over PCC Transport

[PATCH net-next v20 0/1] MCTP Over PCC Transport

[admiyo]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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 (1):
  mctp pcc: Implement MCTP over PCC Transport

 MAINTAINERS                 |   5 +
 drivers/net/mctp/Kconfig    |  13 ++
 drivers/net/mctp/Makefile   |   1 +
 drivers/net/mctp/mctp-pcc.c | 317 ++++++++++++++++++++++++++++++++++++
 4 files changed, 336 insertions(+)
 create mode 100644 drivers/net/mctp/mctp-pcc.c

-- 
2.43.0

[PATCH net-next v20 1/1] mctp pcc: Implement MCTP over PCC Transport

[admiyo]

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

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

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

MCTP devices are specified via ACPI by entries
in DSDT/SDST and reference channels specified
in the PCCT.

Communication with other devices use the PCC based
doorbell mechanism.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 657a67f9031e..7642afb18092 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14200,6 +14200,11 @@ F:	include/net/mctpdevice.h
 F:	include/net/netns/mctp.h
 F:	net/mctp/
 
+MANAGEMENT COMPONENT TRANSPORT PROTOCOL (MCTP) over PCC (MCTP-PCC) Driver
+M:	Adam Young <admiyo@os.amperecomputing.com>
+S:	Maintained
+F:	drivers/net/mctp/mctp-pcc.c
+
 MAPLE TREE
 M:	Liam R. Howlett <Liam.Howlett@oracle.com>
 L:	maple-tree@lists.infradead.org
diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
index cf325ab0b1ef..f69d0237f058 100644
--- a/drivers/net/mctp/Kconfig
+++ b/drivers/net/mctp/Kconfig
@@ -57,6 +57,19 @@ config MCTP_TRANSPORT_USB
 	  MCTP-over-USB interfaces are peer-to-peer, so each interface
 	  represents a physical connection to one remote MCTP endpoint.
 
+config MCTP_TRANSPORT_PCC
+	tristate "MCTP PCC transport"
+	depends on ACPI
+	help
+	  Provides a driver to access MCTP devices over PCC transport,
+	  A MCTP protocol network device is created via ACPI for each
+	  entry in the DST/SDST that matches the identifier. The Platform
+	  communication channels are selected from the corresponding
+	  entries in the PCCT.
+
+	  Say y here if you need to connect to MCTP endpoints over PCC. To
+	  compile as a module, use m; the module will be called mctp-pcc.
+
 endmenu
 
 endif
diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile
index c36006849a1e..2276f148df7c 100644
--- a/drivers/net/mctp/Makefile
+++ b/drivers/net/mctp/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_MCTP_TRANSPORT_PCC) += mctp-pcc.o
 obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o
 obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o
 obj-$(CONFIG_MCTP_TRANSPORT_I3C) += mctp-i3c.o
diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
new file mode 100644
index 000000000000..589ba4387ce6
--- /dev/null
+++ b/drivers/net/mctp/mctp-pcc.c
@@ -0,0 +1,317 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * mctp-pcc.c - Driver for MCTP over PCC.
+ * Copyright (c) 2024, Ampere Computing LLC
+ */
+
+/* Implementation of MCTP over PCC DMTF Specification DSP0256
+ * https://www.dmtf.org/sites/default/files/standards/documents/DSP0256_2.0.0WIP50.pdf
+ */
+
+#include <linux/acpi.h>
+#include <linux/if_arp.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+
+#include <acpi/acpi_bus.h>
+#include <acpi/acpi_drivers.h>
+#include <acpi/acrestyp.h>
+#include <acpi/actbl.h>
+#include <net/mctp.h>
+#include <net/mctpdevice.h>
+#include <acpi/pcc.h>
+
+#define MCTP_PAYLOAD_LENGTH     256
+#define MCTP_CMD_LENGTH         4
+#define MCTP_PCC_VERSION        0x1 /* DSP0292 only has version: 1 */
+#define MCTP_SIGNATURE          "MCTP"
+#define MCTP_SIGNATURE_LENGTH   (sizeof(MCTP_SIGNATURE) - 1)
+#define MCTP_HEADER_LENGTH      12
+#define MCTP_MIN_MTU            68
+#define PCC_MAGIC               0x50434300
+#define PCC_HEADER_FLAG_REQ_INT 0x1
+#define PCC_HEADER_FLAGS        PCC_HEADER_FLAG_REQ_INT
+#define PCC_DWORD_TYPE          0x0c
+
+struct mctp_pcc_hdr {
+	__le32 signature;
+	__le32 flags;
+	__le32 length;
+	char mctp_signature[MCTP_SIGNATURE_LENGTH];
+};
+
+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 {
+	/* spinlock to serialize access to PCC outbox buffer and registers
+	 * Note that what PCC calls registers are memory locations, not CPU
+	 * Registers.  They include the fields used to synchronize access
+	 * between the OS and remote endpoints.
+	 *
+	 * Only the Outbox needs a spinlock, to prevent multiple
+	 * sent packets triggering multiple attempts to over write
+	 * the outbox.  The Inbox buffer is controlled by the remote
+	 * service and a spinlock would have no effect.
+	 */
+	spinlock_t lock;
+	struct mctp_dev mdev;
+	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 *c, void *buffer)
+{
+	struct mctp_pcc_ndev *mctp_pcc_ndev;
+	struct mctp_pcc_hdr mctp_pcc_hdr;
+	struct mctp_skb_cb *cb;
+	struct sk_buff *skb;
+	void *skb_buf;
+	u32 data_len;
+
+	mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, inbox.client);
+	memcpy_fromio(&mctp_pcc_hdr, mctp_pcc_ndev->inbox.chan->shmem,
+		      sizeof(struct mctp_pcc_hdr));
+	data_len = le32_to_cpu(mctp_pcc_hdr.length) + MCTP_HEADER_LENGTH;
+	if (data_len > mctp_pcc_ndev->mdev.dev->mtu) {
+		dev_dstats_rx_dropped(mctp_pcc_ndev->mdev.dev);
+		return;
+	}
+
+	skb = netdev_alloc_skb(mctp_pcc_ndev->mdev.dev, data_len);
+	if (!skb) {
+		dev_dstats_rx_dropped(mctp_pcc_ndev->mdev.dev);
+		return;
+	}
+	dev_dstats_rx_add(mctp_pcc_ndev->mdev.dev, data_len);
+
+	skb->protocol = htons(ETH_P_MCTP);
+	skb_buf = skb_put(skb, data_len);
+	memcpy_fromio(skb_buf, mctp_pcc_ndev->inbox.chan->shmem, data_len);
+
+	skb_reset_mac_header(skb);
+	skb_pull(skb, sizeof(struct mctp_pcc_hdr));
+	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 mctp_pcc_hdr *mctp_pcc_header;
+	void __iomem *buffer;
+	unsigned long flags;
+	int len = skb->len;
+	int rc;
+
+	rc = skb_cow_head(skb, sizeof(struct mctp_pcc_hdr));
+	if (rc)
+		goto err_drop;
+
+	mctp_pcc_header = skb_push(skb, sizeof(struct mctp_pcc_hdr));
+	mctp_pcc_header->signature = cpu_to_le32(PCC_MAGIC | mpnd->outbox.index);
+	mctp_pcc_header->flags = cpu_to_le32(PCC_HEADER_FLAGS);
+	memcpy(mctp_pcc_header->mctp_signature, MCTP_SIGNATURE,
+	       MCTP_SIGNATURE_LENGTH);
+	mctp_pcc_header->length = cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);
+
+	spin_lock_irqsave(&mpnd->lock, flags);
+	buffer = mpnd->outbox.chan->shmem;
+	memcpy_toio(buffer, skb->data, skb->len);
+	mpnd->outbox.chan->mchan->mbox->ops->send_data(mpnd->outbox.chan->mchan,
+							NULL);
+	spin_unlock_irqrestore(&mpnd->lock, flags);
+
+	dev_dstats_tx_add(ndev, len);
+	dev_consume_skb_any(skb);
+	return NETDEV_TX_OK;
+err_drop:
+	dev_dstats_tx_dropped(ndev);
+	kfree_skb(skb);
+	return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops mctp_pcc_netdev_ops = {
+	.ndo_start_xmit = mctp_pcc_tx,
+};
+
+static const struct mctp_netdev_ops mctp_netdev_ops = {
+	NULL
+};
+
+static void mctp_pcc_setup(struct net_device *ndev)
+{
+	ndev->type = ARPHRD_MCTP;
+	ndev->hard_header_len = 0;
+	ndev->tx_queue_len = 0;
+	ndev->flags = IFF_NOARP;
+	ndev->netdev_ops = &mctp_pcc_netdev_ops;
+	ndev->needs_free_netdev = true;
+	ndev->pcpu_stat_type = NETDEV_PCPU_STAT_DSTATS;
+}
+
+struct mctp_pcc_lookup_context {
+	int index;
+	u32 inbox_index;
+	u32 outbox_index;
+};
+
+static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
+				       void *context)
+{
+	struct mctp_pcc_lookup_context *luc = context;
+	struct acpi_resource_address32 *addr;
+
+	switch (ares->type) {
+	case PCC_DWORD_TYPE:
+		break;
+	default:
+		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 void mctp_cleanup_channel(void *data)
+{
+	struct pcc_mbox_chan *chan = data;
+
+	pcc_mbox_free_channel(chan);
+}
+
+static int mctp_pcc_initialize_mailbox(struct device *dev,
+				       struct mctp_pcc_mailbox *box, u32 index)
+{
+	int ret;
+
+	box->index = index;
+	box->chan = pcc_mbox_request_channel(&box->client, index);
+	if (IS_ERR(box->chan))
+		return PTR_ERR(box->chan);
+	ret = devm_add_action_or_reset(dev, mctp_cleanup_channel, box->chan);
+	if (ret)
+		return -EINVAL;
+	return 0;
+}
+
+static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
+{
+	struct mctp_pcc_lookup_context context = {0, 0, 0};
+	struct mctp_pcc_ndev *mctp_pcc_ndev;
+	struct device *dev = &acpi_dev->dev;
+	struct net_device *ndev;
+	acpi_handle dev_handle;
+	acpi_status status;
+	int mctp_pcc_mtu;
+	char name[32];
+	int rc;
+
+	dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
+		acpi_device_hid(acpi_dev));
+	dev_handle = acpi_device_handle(acpi_dev);
+	status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
+				     &context);
+	if (!ACPI_SUCCESS(status)) {
+		dev_err(dev, "FAILURE to lookup PCC indexes from CRS\n");
+		return -EINVAL;
+	}
+
+	/* inbox initialization */
+	snprintf(name, sizeof(name), "mctpipcc%d", context.inbox_index);
+	ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_PREDICTABLE,
+			    mctp_pcc_setup);
+	if (!ndev)
+		return -ENOMEM;
+
+	mctp_pcc_ndev = netdev_priv(ndev);
+	spin_lock_init(&mctp_pcc_ndev->lock);
+
+	rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
+					 context.inbox_index);
+	if (rc)
+		goto free_netdev;
+	mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
+
+	/* outbox initialization */
+	rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox,
+					 context.outbox_index);
+	if (rc)
+		goto free_netdev;
+
+	mctp_pcc_ndev->acpi_device = acpi_dev;
+	mctp_pcc_ndev->inbox.client.dev = dev;
+	mctp_pcc_ndev->outbox.client.dev = dev;
+	mctp_pcc_ndev->mdev.dev = ndev;
+	acpi_dev->driver_data = mctp_pcc_ndev;
+
+	/* There is no clean way to pass the MTU to the callback function
+	 * used for registration, so set the values ahead of time.
+	 */
+	mctp_pcc_mtu = mctp_pcc_ndev->outbox.chan->shmem_size -
+		sizeof(struct mctp_pcc_hdr);
+	ndev->mtu = MCTP_MIN_MTU;
+	ndev->max_mtu = mctp_pcc_mtu;
+	ndev->min_mtu = MCTP_MIN_MTU;
+
+	/* ndev needs to be freed before the iomemory (mapped above) gets
+	 * unmapped,  devm resources get freed in reverse to the order they
+	 * are added.
+	 */
+	rc = mctp_register_netdev(ndev, &mctp_netdev_ops, MCTP_PHYS_BINDING_PCC);
+	if (rc)
+		goto free_netdev;
+	return devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
+free_netdev:
+	free_netdev(ndev);
+	return rc;
+}
+
+static const struct acpi_device_id mctp_pcc_device_ids[] = {
+	{ "DMT0001"},
+	{}
+};
+
+static struct acpi_driver mctp_pcc_driver = {
+	.name = "mctp_pcc",
+	.class = "Unknown",
+	.ids = mctp_pcc_device_ids,
+	.ops = {
+		.add = mctp_pcc_driver_add,
+	},
+};
+
+module_acpi_driver(mctp_pcc_driver);
+
+MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids);
+
+MODULE_DESCRIPTION("MCTP PCC ACPI device");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>");
-- 
2.43.0

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

[Jonathan Cameron]

On Wed, 23 Apr 2025 18:01:42 -0400
admiyo@os.amperecomputing.com wrote:

> From: Adam Young <admiyo@os.amperecomputing.com>
> 
> Implementation of network driver for
> Management Control Transport Protocol(MCTP)
> over Platform Communication Channel(PCC)
> 
> DMTF DSP:0292
> https://www.dmtf.org/sites/default/files/standards/documents/\
> DSP0292_1.0.0WIP50.pdf
> 
> MCTP devices are specified via ACPI by entries
> in DSDT/SDST and reference channels specified
> in the PCCT.
> 
> Communication with other devices use the PCC based
> doorbell mechanism.
> 
> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
Hi Adam,

Sorry it's been a while since I last looked at this.

Anyhow, some fairly general feedback inline.  All minor stuff.
With that tidied up.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  MAINTAINERS                 |   5 +
>  drivers/net/mctp/Kconfig    |  13 ++
>  drivers/net/mctp/Makefile   |   1 +
>  drivers/net/mctp/mctp-pcc.c | 317 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 336 insertions(+)
>  create mode 100644 drivers/net/mctp/mctp-pcc.c

> new file mode 100644
> index 000000000000..589ba4387ce6
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c
> @@ -0,0 +1,317 @@

> +
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
> +	struct mctp_pcc_hdr *mctp_pcc_header;
> +	void __iomem *buffer;
> +	unsigned long flags;
> +	int len = skb->len;
> +	int rc;
> +
> +	rc = skb_cow_head(skb, sizeof(struct mctp_pcc_hdr));
> +	if (rc)
> +		goto err_drop;
> +
> +	mctp_pcc_header = skb_push(skb, sizeof(struct mctp_pcc_hdr));

I'd use sizeof(*mctp_pcc_header) for these to avoid the reader needing to check
types. There are a bunch of these you could do the same with to slightly
improve readability.

> +	mctp_pcc_header->signature = cpu_to_le32(PCC_MAGIC | mpnd->outbox.index);
> +	mctp_pcc_header->flags = cpu_to_le32(PCC_HEADER_FLAGS);
> +	memcpy(mctp_pcc_header->mctp_signature, MCTP_SIGNATURE,
> +	       MCTP_SIGNATURE_LENGTH);
> +	mctp_pcc_header->length = cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);
> +
> +	spin_lock_irqsave(&mpnd->lock, flags);
> +	buffer = mpnd->outbox.chan->shmem;
> +	memcpy_toio(buffer, skb->data, skb->len);
> +	mpnd->outbox.chan->mchan->mbox->ops->send_data(mpnd->outbox.chan->mchan,
> +							NULL);

Slightly odd alignment.  One extra space?

> +	spin_unlock_irqrestore(&mpnd->lock, flags);
> +
> +	dev_dstats_tx_add(ndev, len);
> +	dev_consume_skb_any(skb);
> +	return NETDEV_TX_OK;
> +err_drop:
> +	dev_dstats_tx_dropped(ndev);
> +	kfree_skb(skb);
> +	return NETDEV_TX_OK;
> +}

> +static acpi_status lookup_pcct_indices(struct acpi_resource *ares,
> +				       void *context)
> +{
> +	struct mctp_pcc_lookup_context *luc = context;
> +	struct acpi_resource_address32 *addr;
> +
> +	switch (ares->type) {
> +	case PCC_DWORD_TYPE:
If unlikely we will ever care about other types could simpilfy to
	if (ares->type != PCC_DWORD_TYPE)
		return AE_OK;

	etc

> +		break;
> +	default:
> +		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 int mctp_pcc_initialize_mailbox(struct device *dev,
> +				       struct mctp_pcc_mailbox *box, u32 index)
> +{
> +	int ret;
> +
> +	box->index = index;
> +	box->chan = pcc_mbox_request_channel(&box->client, index);
as below
	box->client.dev = dev;

> +	if (IS_ERR(box->chan))
> +		return PTR_ERR(box->chan);
I'd put a blank line here...
> +	ret = devm_add_action_or_reset(dev, mctp_cleanup_channel, box->chan);
> +	if (ret)
> +		return -EINVAL;
And here.

Why eat ret?  Which incidentally is normally -ENOMEM for these.
Why not the simpler
	return devm_add_action_or_reset(dev, mctp_cleanup_channel, box->chan);


> +	return 0;
> +}
> +
> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
> +{
> +	struct mctp_pcc_lookup_context context = {0, 0, 0};

I'd be tempted to use simply {} or { 0 } so that if we have
extra context in future we aren't somehow implying it should not be
intialized to zero (as it will happen anyway).

> +	struct mctp_pcc_ndev *mctp_pcc_ndev;
> +	struct device *dev = &acpi_dev->dev;
> +	struct net_device *ndev;
> +	acpi_handle dev_handle;
> +	acpi_status status;
> +	int mctp_pcc_mtu;
> +	char name[32];
> +	int rc;
> +
> +	dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
> +		acpi_device_hid(acpi_dev));
> +	dev_handle = acpi_device_handle(acpi_dev);
> +	status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
> +				     &context);
> +	if (!ACPI_SUCCESS(status)) {
> +		dev_err(dev, "FAILURE to lookup PCC indexes from CRS\n");
> +		return -EINVAL;
> +	}
> +
> +	/* inbox initialization */
> +	snprintf(name, sizeof(name), "mctpipcc%d", context.inbox_index);
> +	ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_PREDICTABLE,
> +			    mctp_pcc_setup);

I'd use sizeof(*mctp_pcc_ndev) so we don't have to bother checking types...

> +	if (!ndev)
> +		return -ENOMEM;
> +
> +	mctp_pcc_ndev = netdev_priv(ndev);
> +	spin_lock_init(&mctp_pcc_ndev->lock);
> +
> +	rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
> +					 context.inbox_index);

I think this should be responsible for all the setup of inbox. So that
includes setting inbox.client.dev as set below.

> +	if (rc)
> +		goto free_netdev;
> +	mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
> +
> +	/* outbox initialization */
> +	rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox,
> +					 context.outbox_index);

Same as for inbox.

> +	if (rc)
> +		goto free_netdev;
> +
> +	mctp_pcc_ndev->acpi_device = acpi_dev;
> +	mctp_pcc_ndev->inbox.client.dev = dev;
> +	mctp_pcc_ndev->outbox.client.dev = dev;

As above. I think these should be part of the initialize_mailbox calls
given they are part of the mailboxes and we are passing in dev anyway.

> +	mctp_pcc_ndev->mdev.dev = ndev;
> +	acpi_dev->driver_data = mctp_pcc_ndev;
> +
> +	/* There is no clean way to pass the MTU to the callback function
> +	 * used for registration, so set the values ahead of time.
> +	 */
> +	mctp_pcc_mtu = mctp_pcc_ndev->outbox.chan->shmem_size -
> +		sizeof(struct mctp_pcc_hdr);
> +	ndev->mtu = MCTP_MIN_MTU;
> +	ndev->max_mtu = mctp_pcc_mtu;
> +	ndev->min_mtu = MCTP_MIN_MTU;
> +
> +	/* ndev needs to be freed before the iomemory (mapped above) gets
> +	 * unmapped,  devm resources get freed in reverse to the order they
> +	 * are added.
> +	 */
> +	rc = mctp_register_netdev(ndev, &mctp_netdev_ops, MCTP_PHYS_BINDING_PCC);
> +	if (rc)
> +		goto free_netdev;
> +	return devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
> +free_netdev:
> +	free_netdev(ndev);
> +	return rc;
> +}
> +
> +static const struct acpi_device_id mctp_pcc_device_ids[] = {
> +	{ "DMT0001"},

Bracket before } for consistency of spacing.

> +	{}
> +};
> +
> +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>");

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

[lihuisong (C)]

Hi,

在 2025/4/24 6:01, admiyo@os.amperecomputing.com 写道:
> From: Adam Young <admiyo@os.amperecomputing.com>
>
> Implementation of network driver for
> Management Control Transport Protocol(MCTP)
> over Platform Communication Channel(PCC)
>
> DMTF DSP:0292
> https://www.dmtf.org/sites/default/files/standards/documents/\
> DSP0292_1.0.0WIP50.pdf
>
> MCTP devices are specified via ACPI by entries
> in DSDT/SDST and reference channels specified
> in the PCCT.
>
> Communication with other devices use the PCC based
> doorbell mechanism.
 From your code, I think mctp driver use type3 and type4, right?
So suggest that add some comments about this in commit log.
>
> 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 | 317 ++++++++++++++++++++++++++++++++++++
>   4 files changed, 336 insertions(+)
>   create mode 100644 drivers/net/mctp/mctp-pcc.c
<...>
> +#define MCTP_HEADER_LENGTH      12
> +#define MCTP_MIN_MTU            68
> +#define PCC_MAGIC               0x50434300
> +#define PCC_HEADER_FLAG_REQ_INT 0x1
Please use PCC_SIGNATURE and PCC_CMD_COMPLETION_NOTIFY in pcc.h
no need to repeatedly define macro.
> +#define PCC_HEADER_FLAGS        PCC_HEADER_FLAG_REQ_INT
> +#define PCC_DWORD_TYPE          0x0c
> +
> +struct mctp_pcc_hdr {
> +	__le32 signature;
> +	__le32 flags;
> +	__le32 length;
> +	char mctp_signature[MCTP_SIGNATURE_LENGTH];
> +};
> +
<...>
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
> +	struct mctp_pcc_hdr *mctp_pcc_header;
> +	void __iomem *buffer;
> +	unsigned long flags;
> +	int len = skb->len;
> +	int rc;
> +
> +	rc = skb_cow_head(skb, sizeof(struct mctp_pcc_hdr));
> +	if (rc)
> +		goto err_drop;
> +
> +	mctp_pcc_header = skb_push(skb, sizeof(struct mctp_pcc_hdr));
> +	mctp_pcc_header->signature = cpu_to_le32(PCC_MAGIC | mpnd->outbox.index);
> +	mctp_pcc_header->flags = cpu_to_le32(PCC_HEADER_FLAGS);
> +	memcpy(mctp_pcc_header->mctp_signature, MCTP_SIGNATURE,
> +	       MCTP_SIGNATURE_LENGTH);
> +	mctp_pcc_header->length = cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);
> +
> +	spin_lock_irqsave(&mpnd->lock, flags);
> +	buffer = mpnd->outbox.chan->shmem;
> +	memcpy_toio(buffer, skb->data, skb->len);
> +	mpnd->outbox.chan->mchan->mbox->ops->send_data(mpnd->outbox.chan->mchan,
> +							NULL);
> +	spin_unlock_irqrestore(&mpnd->lock, flags);
> +
Why does it not need to know if the packet is sent successfully?
It's possible for the platform not to finish to send the packet after 
executing this unlock.
In this moment, the previous packet may be modified by the new packet to 
be sent.
> +	dev_dstats_tx_add(ndev, len);
> +	dev_consume_skb_any(skb);
> +	return NETDEV_TX_OK;
> +err_drop:
> +	dev_dstats_tx_dropped(ndev);
> +	kfree_skb(skb);
> +	return NETDEV_TX_OK;
> +}
> +
> +static const struct net_device_ops mctp_pcc_netdev_ops = {
> +	.ndo_start_xmit = mctp_pcc_tx,
> +};
> +
<...>
> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
> +{
> +	struct mctp_pcc_lookup_context context = {0, 0, 0};
> +	struct mctp_pcc_ndev *mctp_pcc_ndev;
> +	struct device *dev = &acpi_dev->dev;
> +	struct net_device *ndev;
> +	acpi_handle dev_handle;
> +	acpi_status status;
> +	int mctp_pcc_mtu;
> +	char name[32];
> +	int rc;
> +
> +	dev_dbg(dev, "Adding mctp_pcc device for HID %s\n",
> +		acpi_device_hid(acpi_dev));
> +	dev_handle = acpi_device_handle(acpi_dev);
> +	status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices,
> +				     &context);
> +	if (!ACPI_SUCCESS(status)) {
> +		dev_err(dev, "FAILURE to lookup PCC indexes from CRS\n");
> +		return -EINVAL;
> +	}
> +
> +	/* inbox initialization */
> +	snprintf(name, sizeof(name), "mctpipcc%d", context.inbox_index);
> +	ndev = alloc_netdev(sizeof(struct mctp_pcc_ndev), name, NET_NAME_PREDICTABLE,
> +			    mctp_pcc_setup);
> +	if (!ndev)
> +		return -ENOMEM;
> +
> +	mctp_pcc_ndev = netdev_priv(ndev);
> +	spin_lock_init(&mctp_pcc_ndev->lock);
> +
> +	rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
> +					 context.inbox_index);
> +	if (rc)
> +		goto free_netdev;
> +	mctp_pcc_ndev->inbox.client.rx_callback = mctp_pcc_client_rx_callback;
It is good to move the assignemnt of  rx_callback pointer to initialize 
inbox mailbox.
> +
> +	/* outbox initialization */
> +	rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->outbox,
> +					 context.outbox_index);
> +	if (rc)
> +		goto free_netdev;
> +
> +	mctp_pcc_ndev->acpi_device = acpi_dev;
> +	mctp_pcc_ndev->inbox.client.dev = dev;
> +	mctp_pcc_ndev->outbox.client.dev = dev;
> +	mctp_pcc_ndev->mdev.dev = ndev;
> +	acpi_dev->driver_data = mctp_pcc_ndev;
> +
> +	/* There is no clean way to pass the MTU to the callback function
> +	 * used for registration, so set the values ahead of time.
> +	 */
> +	mctp_pcc_mtu = mctp_pcc_ndev->outbox.chan->shmem_size -
> +		sizeof(struct mctp_pcc_hdr);
> +	ndev->mtu = MCTP_MIN_MTU;
> +	ndev->max_mtu = mctp_pcc_mtu;
> +	ndev->min_mtu = MCTP_MIN_MTU;
> +
> +	/* ndev needs to be freed before the iomemory (mapped above) gets
> +	 * unmapped,  devm resources get freed in reverse to the order they
> +	 * are added.
> +	 */
> +	rc = mctp_register_netdev(ndev, &mctp_netdev_ops, MCTP_PHYS_BINDING_PCC);
> +	if (rc)
> +		goto free_netdev;
> +	return devm_add_action_or_reset(dev, mctp_cleanup_netdev, ndev);
> +free_netdev:
> +	free_netdev(ndev);
> +	return rc;
> +}
> +
> +static const struct acpi_device_id mctp_pcc_device_ids[] = {
> +	{ "DMT0001"},
> +	{}
> +};
> +
> +static struct acpi_driver mctp_pcc_driver = {
> +	.name = "mctp_pcc",
> +	.class = "Unknown",
> +	.ids = mctp_pcc_device_ids,
> +	.ops = {
> +		.add = mctp_pcc_driver_add,
> +	},
> +};
> +
> +module_acpi_driver(mctp_pcc_driver);
> +
> +MODULE_DEVICE_TABLE(acpi, mctp_pcc_device_ids);
> +
> +MODULE_DESCRIPTION("MCTP PCC ACPI device");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Adam Young <admiyo@os.amperecomputing.com>");

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

[Adam Young]

On 4/24/25 09:03, lihuisong (C) wrote:
>> +    rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
>> +                     context.inbox_index);
>> +    if (rc)
>> +        goto free_netdev;
>> +    mctp_pcc_ndev->inbox.client.rx_callback = 
>> mctp_pcc_client_rx_callback;
> It is good to move the assignemnt of  rx_callback pointer to 
> initialize inbox mailbox.


The other changes are fine, but this one I do not agree with.

The rx callback only makes sense for one of the two mailboxes, and thus 
is not appropriate for a generic function.

Either  initialize_mailbox needs more complex logic, or would blindly 
assign the callback to both mailboxes, neither of which simplifies or 
streamlines the code.  That function emerged as a way to reduce 
duplication.  Lets keep it that way.

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

[Adam Young]

On 4/24/25 05:57, Jonathan Cameron wrote:
>> +
>> +	mctp_pcc_header = skb_push(skb, sizeof(struct mctp_pcc_hdr));
> I'd use sizeof(*mctp_pcc_header) for these to avoid the reader needing to check
> types. There are a bunch of these you could do the same with to slightly
> improve readability.


I get a compilation error when I try that:

drivers/net/mctp/mctp-pcc.c: In function ‘mctp_pcc_client_rx_callback’:
drivers/net/mctp/mctp-pcc.c:80:30: error: invalid type argument of unary 
‘*’ (have ‘struct mctp_pcc_hdr’)
    80 |                       sizeof(*mctp_pcc_hdr));


Maybe a compiler flag difference?

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

[Jeremy Kerr]

Hi Adam,
> I get a compilation error when I try that:
> 
> drivers/net/mctp/mctp-pcc.c: In function ‘mctp_pcc_client_rx_callback’:
> drivers/net/mctp/mctp-pcc.c:80:30: error: invalid type argument of unary 
> ‘*’ (have ‘struct mctp_pcc_hdr’)
>     80 |                       sizeof(*mctp_pcc_hdr));
> 
> 
> Maybe a compiler flag difference?


In the rx path, `mctp_pcc_hdr` is the struct itself, just drop the
dereference for the push:

    skb_pull(skb, sizeof(mctp_pcc_hdr));

But on tx, mctp_pcc_hdr is a pointer, so:

    mctp_pcc_hdr = skb_push(skb, sizeof(*mctp_pcc_hdr));

Cheers,


Jeremy

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

[lihuisong (C)]

在 2025/4/29 2:48, Adam Young 写道:
>
> On 4/24/25 09:03, lihuisong (C) wrote:
>>> +    rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
>>> +                     context.inbox_index);
>>> +    if (rc)
>>> +        goto free_netdev;
>>> +    mctp_pcc_ndev->inbox.client.rx_callback = 
>>> mctp_pcc_client_rx_callback;
>> It is good to move the assignemnt of  rx_callback pointer to 
>> initialize inbox mailbox.
>
>
> The other changes are fine, but this one I do not agree with.
>
> The rx callback only makes sense for one of the two mailboxes, and 
> thus is not appropriate for a generic function.
>
> Either  initialize_mailbox needs more complex logic, or would blindly 
> assign the callback to both mailboxes, neither of which simplifies or 
> streamlines the code.  That function emerged as a way to reduce 
> duplication.  Lets keep it that way.
>
It depends on you. But please reply my below comment. I didn't see any 
change about it in next version.

-->

> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device 
> *ndev)
> +{
> +    struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
> +    struct mctp_pcc_hdr *mctp_pcc_header;
> +    void __iomem *buffer;
> +    unsigned long flags;
> +    int len = skb->len;
> +    int rc;
> +
> +    rc = skb_cow_head(skb, sizeof(struct mctp_pcc_hdr));
> +    if (rc)
> +        goto err_drop;
> +
> +    mctp_pcc_header = skb_push(skb, sizeof(struct mctp_pcc_hdr));
> +    mctp_pcc_header->signature = cpu_to_le32(PCC_MAGIC | 
> mpnd->outbox.index);
> +    mctp_pcc_header->flags = cpu_to_le32(PCC_HEADER_FLAGS);
> +    memcpy(mctp_pcc_header->mctp_signature, MCTP_SIGNATURE,
> +           MCTP_SIGNATURE_LENGTH);
> +    mctp_pcc_header->length = cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);
> +
> +    spin_lock_irqsave(&mpnd->lock, flags);
> +    buffer = mpnd->outbox.chan->shmem;
> +    memcpy_toio(buffer, skb->data, skb->len);
> + mpnd->outbox.chan->mchan->mbox->ops->send_data(mpnd->outbox.chan->mchan,
> +                            NULL);
> +    spin_unlock_irqrestore(&mpnd->lock, flags);
> +
Why does it not need to know if the packet is sent successfully?
It's possible for the platform not to finish to send the packet after 
executing this unlock.
In this moment, the previous packet may be modified by the new packet to 
be sent.

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

[Adam Young]

On 5/30/25 02:19, lihuisong (C) wrote:
>
> 在 2025/4/29 2:48, Adam Young 写道:
>>
>> On 4/24/25 09:03, lihuisong (C) wrote:
>>>> +    rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
>>>> +                     context.inbox_index);
>>>> +    if (rc)
>>>> +        goto free_netdev;
>>>> +    mctp_pcc_ndev->inbox.client.rx_callback = 
>>>> mctp_pcc_client_rx_callback;
>>> It is good to move the assignemnt of  rx_callback pointer to 
>>> initialize inbox mailbox.
>>
>>
>> The other changes are fine, but this one I do not agree with.
>>
>> The rx callback only makes sense for one of the two mailboxes, and 
>> thus is not appropriate for a generic function.
>>
>> Either  initialize_mailbox needs more complex logic, or would blindly 
>> assign the callback to both mailboxes, neither of which simplifies or 
>> streamlines the code.  That function emerged as a way to reduce 
>> duplication.  Lets keep it that way.
>>
> It depends on you. But please reply my below comment. I didn't see any 
> change about it in next version.
>
> -->
>
>> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct 
>> net_device *ndev)
>> +{
>> +    struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
>> +    struct mctp_pcc_hdr *mctp_pcc_header;
>> +    void __iomem *buffer;
>> +    unsigned long flags;
>> +    int len = skb->len;
>> +    int rc;
>> +
>> +    rc = skb_cow_head(skb, sizeof(struct mctp_pcc_hdr));
>> +    if (rc)
>> +        goto err_drop;
>> +
>> +    mctp_pcc_header = skb_push(skb, sizeof(struct mctp_pcc_hdr));
>> +    mctp_pcc_header->signature = cpu_to_le32(PCC_MAGIC | 
>> mpnd->outbox.index);
>> +    mctp_pcc_header->flags = cpu_to_le32(PCC_HEADER_FLAGS);
>> +    memcpy(mctp_pcc_header->mctp_signature, MCTP_SIGNATURE,
>> +           MCTP_SIGNATURE_LENGTH);
>> +    mctp_pcc_header->length = cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);
>> +
>> +    spin_lock_irqsave(&mpnd->lock, flags);
>> +    buffer = mpnd->outbox.chan->shmem;
>> +    memcpy_toio(buffer, skb->data, skb->len);
>> + 
>> mpnd->outbox.chan->mchan->mbox->ops->send_data(mpnd->outbox.chan->mchan,
>> +                            NULL);
>> +    spin_unlock_irqrestore(&mpnd->lock, flags);
>> +
> Why does it not need to know if the packet is sent successfully?
> It's possible for the platform not to finish to send the packet after 
> executing this unlock.
> In this moment, the previous packet may be modified by the new packet 
> to be sent.

I think you missed version  21.

Version 21 of this function ends with:
         memcpy_toio(buffer, skb->data, skb->len);
         rc = mpnd->outbox.chan->mchan->mbox->ops->send_data
                 (mpnd->outbox.chan->mchan, NULL);
         spin_unlock_irqrestore(&mpnd->lock, flags);
         if ACPI_FAILURE(rc)
                 goto err_drop;
         dev_dstats_tx_add(ndev, len);
         dev_consume_skb_any(skb);
         return NETDEV_TX_OK;
err_drop:
         dev_dstats_tx_dropped(ndev);
         kfree_skb(skb);
         return NETDEV_TX_OK;


Once the memcpy_toio completes, the driver will not look at the packet 
again.  if the Kernel did change it at this point, it would not affect 
the flow.  The send of the packet is checked vi rc returned from 
send_data, and it tags the packet as dropped.  Is this not sufficient?

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

[lihuisong (C)]

在 2025/6/3 4:51, Adam Young 写道:
>
> On 5/30/25 02:19, lihuisong (C) wrote:
>>
>> 在 2025/4/29 2:48, Adam Young 写道:
>>>
>>> On 4/24/25 09:03, lihuisong (C) wrote:
>>>>> +    rc = mctp_pcc_initialize_mailbox(dev, &mctp_pcc_ndev->inbox,
>>>>> +                     context.inbox_index);
>>>>> +    if (rc)
>>>>> +        goto free_netdev;
>>>>> +    mctp_pcc_ndev->inbox.client.rx_callback = 
>>>>> mctp_pcc_client_rx_callback;
>>>> It is good to move the assignemnt of  rx_callback pointer to 
>>>> initialize inbox mailbox.
>>>
>>>
>>> The other changes are fine, but this one I do not agree with.
>>>
>>> The rx callback only makes sense for one of the two mailboxes, and 
>>> thus is not appropriate for a generic function.
>>>
>>> Either  initialize_mailbox needs more complex logic, or would 
>>> blindly assign the callback to both mailboxes, neither of which 
>>> simplifies or streamlines the code.  That function emerged as a way 
>>> to reduce duplication.  Lets keep it that way.
>>>
>> It depends on you. But please reply my below comment. I didn't see 
>> any change about it in next version.
>>
>> -->
>>
>>> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct 
>>> net_device *ndev)
>>> +{
>>> +    struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
>>> +    struct mctp_pcc_hdr *mctp_pcc_header;
>>> +    void __iomem *buffer;
>>> +    unsigned long flags;
>>> +    int len = skb->len;
>>> +    int rc;
>>> +
>>> +    rc = skb_cow_head(skb, sizeof(struct mctp_pcc_hdr));
>>> +    if (rc)
>>> +        goto err_drop;
>>> +
>>> +    mctp_pcc_header = skb_push(skb, sizeof(struct mctp_pcc_hdr));
>>> +    mctp_pcc_header->signature = cpu_to_le32(PCC_MAGIC | 
>>> mpnd->outbox.index);
>>> +    mctp_pcc_header->flags = cpu_to_le32(PCC_HEADER_FLAGS);
>>> +    memcpy(mctp_pcc_header->mctp_signature, MCTP_SIGNATURE,
>>> +           MCTP_SIGNATURE_LENGTH);
>>> +    mctp_pcc_header->length = cpu_to_le32(len + 
>>> MCTP_SIGNATURE_LENGTH);
>>> +
>>> +    spin_lock_irqsave(&mpnd->lock, flags);
>>> +    buffer = mpnd->outbox.chan->shmem;
>>> +    memcpy_toio(buffer, skb->data, skb->len);
>>> + 
>>> mpnd->outbox.chan->mchan->mbox->ops->send_data(mpnd->outbox.chan->mchan,
>>> +                            NULL);
>>> +    spin_unlock_irqrestore(&mpnd->lock, flags);
>>> +
>> Why does it not need to know if the packet is sent successfully?
>> It's possible for the platform not to finish to send the packet after 
>> executing this unlock.
>> In this moment, the previous packet may be modified by the new packet 
>> to be sent.
>
> I think you missed version  21.
>
> Version 21 of this function ends with:
>         memcpy_toio(buffer, skb->data, skb->len);
>         rc = mpnd->outbox.chan->mchan->mbox->ops->send_data
>                 (mpnd->outbox.chan->mchan, NULL);
>         spin_unlock_irqrestore(&mpnd->lock, flags);
>         if ACPI_FAILURE(rc)
>                 goto err_drop;
>         dev_dstats_tx_add(ndev, len);
>         dev_consume_skb_any(skb);
>         return NETDEV_TX_OK;
> err_drop:
>         dev_dstats_tx_dropped(ndev);
>         kfree_skb(skb);
>         return NETDEV_TX_OK;
>
>
> Once the memcpy_toio completes, the driver will not look at the packet 
> again.  if the Kernel did change it at this point, it would not affect 
> the flow.  The send of the packet is checked vi rc returned from 
> send_data, and it tags the packet as dropped.  Is this not sufficient?
>
Yes, it is not enough.
Once send_data() return success, platform can receive an interrupt，but 
the processing of the platform has not ended.
This processing includes handling data and then triggering an interrupt 
to notify OS.
>
>
>

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

[Adam Young]

On 6/3/25 08:03, lihuisong (C) wrote:
>>
>> Once the memcpy_toio completes, the driver will not look at the 
>> packet again.  if the Kernel did change it at this point, it would 
>> not affect the flow.  The send of the packet is checked vi rc 
>> returned from send_data, and it tags the packet as dropped. Is this 
>> not sufficient?
>>
> Yes, it is not enough.
> Once send_data() return success, platform can receive an interrupt，but 
> the processing of the platform has not ended.
> This processing includes handling data and then triggering an 
> interrupt to notify OS. 


This comment caused me to rethink how I was using the PCC mailbox API.  
I realized that it was not actually enforcing the PCC protocol, which 
you identified.  It lead me to rewrite a postion of the PCC Mailbox API, 
and that is in my next patch series.  I would appreciate it if you would 
take a look. I think it addresses this concern, but it might not be 
completely transparent to a reviewer.   I would greatly appreciate if 
you were to look at it and confirm it fixes the issue, or, if I have 
missed something, let me know what you see.