Re: [net-next v35 1/1] mctp pcc: Implement MCTP over PCC Transport
From: Adam Young
Date: Wed Mar 25 2026 - 12:05:22 EST
On 3/25/26 04:49, Jeremy Kerr wrote:
Hi Adam,Fair enough. I just thought reviewers might like the chain to the other 34 versions, as well as documenting the thoroughness that this has been rev
Only a couple of minor things inline, but overall: now that you have a
one-patch series, you may drop the 0/1 cover-letter. The content of that
will be dropped anyway - I think what you have in 1/1 here is
comprehensive.
Yes, this is a vestige of pulling it out of the Mailbox layer that I should have removed.
diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.cMinor, but you're using two different calculations for this: here,
new file mode 100644
index 000000000000..f8d620795349
--- /dev/null
+++ b/drivers/net/mctp/mctp-pcc.c
@@ -0,0 +1,366 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * mctp-pcc.c - Driver for MCTP over PCC.
+ * Copyright (c) 2024-2026, 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;
+};
+
+/**
+ * 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.
+ */
+static int mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
+{
+ struct acpi_pcct_ext_pcc_shared_memory *pcc_header = data;
+ /*
+ * The PCC header length includes the command field
+ * but not the other values from the header.
+ */
+ pcc_header->length = len - sizeof(struct acpi_pcct_ext_pcc_shared_memory) + sizeof(u32);
and in rx_callback(), where you use sizeof(pcc_header.command) instead
of sizeof(u32), and have swapped the order.
However, it's a bit odd that you're setting ->length here anyway, as
you seem to have already done so earlier mctp_pcc_tx, but there using a
(third, different) calculation?
Agreed
Perhaps just set up all pcc_header fields in mctp_pcc_tx, rather than
casting skb->data back to a pcc header, then re-setting fields in that.
Removed.
With that changed, you may not need a helper for this at all, as it's
just doing the length check and the memcpy_toio().
+This condition (size <= sizeof(pcc_header)) reads a bit strangely given
+ if (len > pchan->shmem_size)
+ return 0;
+
+ memcpy_toio(pchan->shmem, data, len);
+
+ return len;
+}
+
+static void mctp_pcc_client_rx_callback(struct mbox_client *cl, void *mssg)
+{
+ struct acpi_pcct_ext_pcc_shared_memory 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;
+ memcpy_fromio(&pcc_header, inbox->chan->shmem, sizeof(pcc_header));
+ size = pcc_header.length - sizeof(pcc_header.command) + sizeof(pcc_header);
+
+ if (size <= sizeof(pcc_header)) {
+ dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
+ return;
+ }
that you have just *added* a sizeof(pcc_header) one line up.
The only way this path is taken is if you end up underflowing `size`,
before adding the pcc_header size back on, which seems awkward. I would
suggest checking the raw value of pcc_header.length, do the drop if that
fails, *then* adjust for the header/command sizes.
The whole "length = length of the payload + 4 bytes for command" logic is already confusing. I felt this was the clearest way to show exactly what needed to be checked to ensure we have a valid header. I guess the alternative is
if (pcc_header.length <= 0) {
dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
return;
}
size = pcc_header.length - sizeof(pcc_header.command) + sizeof(pcc_header);
Yeah that is clearer.
+ if (memcmp(&pcc_header.command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH) != 0) {We only got half way through the discussion on this, my bad. In that,
+ dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
+ return;
+ }
+
+ if (size > mctp_pcc_ndev->ndev->mtu)
+ dev_dbg(cl->dev, "MCTP_PCC bytes available exceeds MTU");
you mentioned:
They get dropped by the MCTP layer in the Kernel, apparently, so the... which isn't the case. We're fine to receive packets larger than the
dev-dbg is to help someone debug why.
MTU. It's a MTU, not a MRU :)
I can't explain it, as I did not debug that deeply. I will come back to this at some point, but it does not affect the proper functioning of the driver.
I'll remove it. We can keep it internally until I have the problem diagnosed and re-add it if it makes sense.
Your driver is still fine to impose a limit if suitable, but you're not
constrained by the MCTP core here.
Cheers,
Jeremy