Re: [net-next v44] mctp pcc: Implement MCTP over PCC Transport

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
mctp pcc: Implement MCTP over PCC Transport

This patch adds a new mctp-pcc network driver that implements the DMTF
DSP0292 binding for MCTP over the ACPI Platform Communication Channel.
Type 3 channels are used for transmit and Type 4 channels for receive,
with each interface created from a DMT0001 ACPI device referencing two
PCC subspaces.

Link: https://www.dmtf.org/sites/default/files/standards/documents/DSP0292_1.0.0WIP50.pdf
Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html

A few questions inline below.

[Low]
The Link tag and the body of the commit message both reference
DSP0292_1.0.0WIP50. Is it intentional to merge an in-tree binding
based on a Work-In-Progress draft of the spec rather than the
published 1.0.0?

Anything userspace-visible that ships in this driver (the netdev name
mctppccN, the ARPHRD type, the literal "MCTP" marker overloaded into
the PCC command field) becomes a de-facto ABI once it lands in a
stable kernel; if the published DSP0292 finalises any of those points
differently, the kernel would have to keep bug-compatibility with the
WIP variant. Could the rationale for picking the WIP baseline be
captured in the commit message, or alternatively could the merge wait
on the published revision?

> diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig
> index cf325ab0b1ef..e68d23794a80 100644
> --- a/drivers/net/mctp/Kconfig
> +++ b/drivers/net/mctp/Kconfig
> @@ -47,6 +47,22 @@ 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
> +	depends on PCC
> +	depends on 64BIT
> +	depends on CPU_LITTLE_ENDIAN

[Medium]
Could the rationale for depends on 64BIT and depends on
CPU_LITTLE_ENDIAN be added to the commit message?

DSP0292 and ACPI PCC are not 64-bit-only, and the PCC shared memory
header is fixed little-endian which can be handled with __le32 fields
and le32_to_cpu()/readl() accessors so the driver runs on any host
endianness. Other PCC consumers such as drivers/acpi/cppc_acpi.c and
drivers/mailbox/pcc.c are not LE-only.

If these depends on lines are present to paper over byte-handling
choices in the driver, would it be preferable to use endian-correct
accessors and drop the dependencies?

> +	help
> +	  Provides a driver to access MCTP devices over PCC transport,
[ ... ]

> diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
> new file mode 100644
> index 000000000000..f2dd5286efa7
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c
> @@ -0,0 +1,429 @@
> +// 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
> + */

[Low]
The header comment says DSP0256 but the URL and the commit message
both say DSP0292. DSP0256 is a different DMTF document (PLDM for
Monitoring and Control). Should the comment read DSP0292?

[ ... ]

> +#define MCTP_SIGNATURE          "MCTP"
> +#define MCTP_SIGNATURE_LENGTH   (sizeof(MCTP_SIGNATURE) - 1)
> +#define MCTP_MIN_MTU            68
> +#define PCC_HEADER_SIZE         sizeof(struct acpi_pcct_ext_pcc_shared_memory)
> +#define MCTP_PCC_MIN_SIZE       (PCC_HEADER_SIZE + MCTP_MIN_MTU)
> +#define PCC_EXTRA_LEN           (PCC_HEADER_SIZE - sizeof(pcc_header.command))

[Low]
PCC_EXTRA_LEN refers to pcc_header, which is not a global symbol or
type but a local variable inside mctp_pcc_client_rx_callback(). It
expands successfully today only because every caller happens to have
a pcc_header in scope. Would something like the following be more
robust?

  #define PCC_EXTRA_LEN (PCC_HEADER_SIZE - \
      sizeof_field(struct acpi_pcct_ext_pcc_shared_memory, command))

[ ... ]

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

[High]
Can this dereference a NULL shmem during teardown?

Looking at pcc_mbox_free_channel() in drivers/mailbox/pcc.c, the
channel's shmem is iounmapped (and the pointer cleared) before the
shutdown path runs free_irq(). Until free_irq() returns, an inbound
doorbell on another CPU can still fire pcc_mbox_irq, which calls
mbox_chan_received_data(), which calls chan->cl->rx_callback (cl is
still set at that moment) — landing here.

Existing PCC clients such as drivers/acpi/acpi_pcc.c::pcc_rx_callback
do not touch shmem from their callback, so this driver appears to be
the first one to expose this ordering.

Should the rx_callback bail out when inbox->chan->shmem is NULL, or
should the ordering in pcc_mbox_free_channel() (free_irq before
iounmap) be addressed in the mailbox layer first?

> +
> +	// The message must at least have the PCC command indicating it is an MCTP
> +	// message followed by the MCTP header, or we have a malformed message.
> +	// This may be run on big endian system, but the data in the buffer is
> +	// explicitly little endian.

[Medium]
This comment claims the callback may run on a big-endian system, but
the Kconfig has depends on CPU_LITTLE_ENDIAN, and the u32 fields
length, command and signature are read from MMIO via memcpy_fromio
without any le32_to_cpu() or readl() byteswap. On big-endian, those
header fields would be misinterpreted.

Is the comment stale, or is the intent eventually to drop the
CPU_LITTLE_ENDIAN dependency? If a future change relaxes the Kconfig
dependency based on this comment, the driver will silently corrupt
multi-byte header fields. Could the comment and the Kconfig be made
consistent (and the code made endian-correct if the goal is to
support BE)?

> +	if (pcc_header.length < sizeof(pcc_header.command) + sizeof(struct mctp_hdr))
> +		goto error;
> +
> +	// If the reported size is larger than the shared memory minus headers,
> +	// something is wrong and treat the buffer as corrupted data.
> +	if (pcc_header.length > inbox->chan->shmem_size - PCC_EXTRA_LEN)
> +		goto error;
> +
> +	if (memcmp(&pcc_header.command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH) != 0)
> +		goto error;

[Low]
The TX path stamps pcc_header.signature with PCC_SIGNATURE |
subspace_id but the RX path validates pcc_header.length and the
"MCTP" command field only — signature is never checked. Would it be
worth adding a signature check here for symmetry with TX and to
reject obviously malformed peer input?

> +
> +	size = pcc_header.length + PCC_EXTRA_LEN;

[Medium]
Is the type of size correct here?

pcc_header.length is u32 and is sourced from peer-writable shared
memory. PCC_EXTRA_LEN is a sizeof() expression of type size_t. The
sum is a size_t that is then narrowed to a signed int.

The only upper bound on pcc_header.length is the earlier check
against inbox->chan->shmem_size - PCC_EXTRA_LEN. shmem_size is u64
from the PCCT and is only checked to be >= MCTP_PCC_MIN_SIZE in
mctp_pcc_open(). If firmware ever exposes a PCC channel whose
shmem_size exceeds INT_MAX, pcc_header.length can be near 4 GiB and
still pass that bound, after which size wraps to a small or negative
int.

The subsequent skb_pull(skb, sizeof(pcc_header)) on a too-small skb
ends up in __skb_pull() which calls BUG(). Would using unsigned int
or size_t for size, validating pcc_header.length against an
INT_MAX-safe bound, and sanity-checking shmem_size in mctp_pcc_open()
close this off?

> +	skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
> +	if (!skb)
> +		goto error;
> +
> +	skb_put(skb, size);
> +	skb->protocol = htons(ETH_P_MCTP);
> +	memcpy_fromio(skb->data, inbox->chan->shmem, size);

[Medium, Low]
Two concerns about reading from shmem here:

First, the header bytes are read twice — once into the local
pcc_header for validation, and again as the leading bytes of this
memcpy_fromio() into skb->data. Between the two reads the remote
endpoint can rewrite the shared region, so the bytes the driver
validated and the bytes the MCTP layer parses are not guaranteed to
match. A common hardening pattern is to snapshot once into a kernel
buffer, validate from the snapshot, and then either copy the
validated header into skb->data and memcpy_fromio() only the
remaining payload, or do a single MMIO copy into the skb and validate
from that copy.

Second, on weakly-ordered architectures (arm64 is included via 64BIT)
memcpy_fromio is a byte/word loop without a final dma_rmb() and the
PCC mailbox layer does not insert a barrier on the consumer side
before invoking the rx callback. Would a dma_rmb() between the IRQ
and consuming buffer contents (and between successive memcpy_fromio()
calls of the same region) be appropriate here?

> +	dev_dstats_rx_add(mctp_pcc_ndev->ndev, size);
> +	skb_pull(skb, sizeof(pcc_header));
> +	skb_reset_mac_header(skb);
> +	skb_reset_network_header(skb);
> +	cb = __mctp_cb(skb);
> +	cb->halen = 0;
> +	netif_rx(skb);
> +	return;
> +
> +error:
> +	dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> +}
> +
> +static netdev_tx_t mctp_pcc_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct acpi_pcct_ext_pcc_shared_memory *pcc_header;
> +	struct mctp_pcc_ndev *mpnd = netdev_priv(ndev);
> +	int len = skb->len;
> +
> +	if (skb_cow_head(skb, sizeof(*pcc_header)))
> +		goto error;
> +
> +	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;
> +
> +	if (skb->len > mpnd->outbox.chan->shmem_size)
> +		goto error;

[Low]
This length check is performed after skb_cow_head() and skb_push()
have already mutated the skb. It also looks unreachable in normal
operation: net core enforces skb->len <= dev->mtu, and
initialize_mtu() sets max_mtu = shmem_size - sizeof(pcc_header), so
after the skb_push the resulting length is at most shmem_size.

Could the check either be moved before the mutation (comparing
len + sizeof(*pcc_header) against shmem_size) or removed?

> +
[ ... ]
> +	netif_stop_queue(ndev);
> +	if (mbox_send_message(mpnd->outbox.chan->mchan, skb) >= 0) {
> +		netif_wake_queue(ndev);
> +	} else {
> +		// Remove the header in case it gets sent again
> +		skb_pull(skb, sizeof(*pcc_header));
> +		return NETDEV_TX_BUSY;
> +	}
> +	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;
> +
> +	mctp_pcc_ndev = container_of(cl, struct mctp_pcc_ndev, outbox.client);
> +	outbox = &mctp_pcc_ndev->outbox;
> +
> +	/* The PCC Mailbox typically does not make use of the mssg pointer
> +	 * The mctp-over pcc driver is the only client that uses it.
> +	 * This value should always be non-null; it is possible
> +	 * that a change in the Mailbox level will break that assumption.
> +	 */

[Medium]
The comment effectively concedes that this driver depends on
behaviour that other PCC clients (cppc, acpi_pcc, hisi_uncore) do not
rely on, and that a change in the mailbox layer could break it.
Building a new in-tree driver on an undocumented contract is fragile
— if PCC core ever ignores or sanitises mssg, this driver's TX path
would silently leak SKBs or crash.

Could the mssg semantics be formalised in the mailbox/PCC API, or
could the driver instead track outstanding SKBs in its own state
(for example, a single-slot or queue indexed by send order) so it
does not depend on mssg being passed through?

> +	if (!skb) {
> +		netdev_warn_once(mctp_pcc_ndev->ndev,
> +				 "%s called with null message.\n", __func__);
> +		return;
> +	}
> +	memcpy_toio(outbox->chan->shmem, skb->data, skb->len);
> +}
> +
> +static void mctp_pcc_tx_done(struct mbox_client *c, void *mssg, int rc)
> +{
> +	struct mctp_pcc_ndev *mctp_pcc_ndev;
> +	struct pcpu_dstats *dstats;
> +	struct sk_buff *skb = mssg;
> +	unsigned long flags;
> +
> +	/*
> +	 * If there is a packet in flight during driver cleanup
> +	 * It may have been freed already.
> +	 */
> +	if (!mssg)
> +		return;

[Medium]
The comment describes a use-after-free hazard, but a NULL check does
not detect a freed pointer — freed memory is not NULL.

Looking at the actual cleanup path, mctp_pcc_stop() takes
chan->lock, swaps active_req to MBOX_NO_MSG and frees the skb, while
tx_tick() in drivers/mailbox/mailbox.c also reads-and-clears
active_req under the same lock before invoking tx_done. So tx_done
should never run with a freed pointer here, and this NULL check is
effectively dead.

Either the documented race is real and this guard does not actually
catch it (the subsequent dev_consume_skb_any(skb) and skb->len reads
would still touch freed memory), or the race cannot occur and the
comment is misleading. Could this be reworked so the comment matches
the actual invariants?

[ ... ]
> +}
> +
> +static int mctp_pcc_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);
> +	if (outbox->chan->shmem_size < MCTP_PCC_MIN_SIZE) {
> +		pcc_mbox_free_channel(outbox->chan);
> +		return -EINVAL;
> +	}
> +
> +	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);
> +	}
> +	if (inbox->chan->shmem_size < MCTP_PCC_MIN_SIZE) {
> +		pcc_mbox_free_channel(outbox->chan);
> +		pcc_mbox_free_channel(inbox->chan);
> +		return -EINVAL;
> +	}

[Medium]
On the shmem_size validation failure paths, outbox->chan and
inbox->chan are not reset to NULL after pcc_mbox_free_channel(),
which is the invariant mctp_pcc_stop() establishes elsewhere (it
sets the pointers to NULL after freeing). After ndo_open returns an
error those fields hold pointers to channels whose shmem has been
unmapped and whose chan->cl has been cleared. Should these be
NULLed for consistency?

> +	return 0;
> +}
> +
> +static int mctp_pcc_stop(struct net_device *ndev)
> +{
> +	struct mctp_pcc_ndev *mctp_pcc_ndev;
> +	unsigned int count, idx;
> +	struct mbox_chan *chan;
> +	struct sk_buff *skb;
> +
> +	mctp_pcc_ndev = netdev_priv(ndev);
> +	chan = mctp_pcc_ndev->outbox.chan->mchan;
> +	pcc_mbox_free_channel(mctp_pcc_ndev->inbox.chan);
> +	mctp_pcc_ndev->inbox.chan = NULL;
> +	scoped_guard(spinlock_irqsave, &chan->lock) {
> +		if (chan->active_req !=  MBOX_NO_MSG) {
> +			skb = chan->active_req;
> +			chan->active_req = MBOX_NO_MSG;
> +			dev_dstats_tx_dropped(ndev);
> +			dev_consume_skb_any(skb);
> +		}
> +		while (chan->msg_count > 0) {
> +			count = chan->msg_count;
> +			idx = chan->msg_free;
> +			if (idx >= count)
> +				idx -= count;
> +			else
> +				idx += MBOX_TX_QUEUE_LEN - count;
> +			skb = chan->msg_data[idx];
> +			dev_dstats_tx_dropped(ndev);
> +			dev_consume_skb_any(skb);
> +			chan->msg_count--;
> +		}
> +	}

[High]
Should this driver be reaching into the private internals of struct
mbox_chan?

This block takes chan->lock (commented in mailbox_controller.h as
"Serialise access to the channel"), reads and clears chan->active_req,
walks chan->msg_data[] using a verbatim copy of msg_submit()'s tail
math:

    count = chan->msg_count;
    idx = chan->msg_free;
    if (idx >= count)
        idx -= count;
    else
        idx += MBOX_TX_QUEUE_LEN - count;

and decrements chan->msg_count. These fields are owned by
drivers/mailbox/mailbox.c and there is no public API for clients to
drain queued TX messages.

Note also that chan->msg_free is never advanced by this loop, leaving
the channel with msg_count=0 but msg_free unchanged — this only
happens to be safe because pcc_mbox_free_channel() runs immediately
afterwards.

Any future change to the mailbox queue layout, locking primitive, or
indexing semantics would silently produce leaks, double-frees, or
corruption in this driver only. Would it be feasible to add a generic
mbox_drain_channel() helper, or have the mailbox call tx_done with a
failure code on free, instead of duplicating this internal logic in a
client?

> +	pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
> +	mctp_pcc_ndev->outbox.chan = NULL;
[ ... ]
> +	return 0;
> +}
[ ... ]
> +static int initialize_mtu(struct net_device *ndev)
> +{
> +	struct mctp_pcc_ndev *mctp_pcc_ndev;
> +	struct mctp_pcc_mailbox *outbox;
> +	struct pcc_mbox_chan *pchan;
> +	int mctp_pcc_max_mtu;
> +
> +	mctp_pcc_ndev = netdev_priv(ndev);
> +	outbox = &mctp_pcc_ndev->outbox;
> +	pchan = pcc_mbox_request_channel(&outbox->client, outbox->index);
> +	if (IS_ERR(pchan))
> +		return PTR_ERR(pchan);
> +	if (pchan->shmem_size < MCTP_MIN_MTU + sizeof(struct acpi_pcct_ext_pcc_shared_memory)) {
> +		pcc_mbox_free_channel(pchan);
> +		return -EINVAL;
> +	}
> +	mctp_pcc_max_mtu = pchan->shmem_size - sizeof(struct acpi_pcct_ext_pcc_shared_memory);
> +	pcc_mbox_free_channel(pchan);

[Medium]
Probe time acquires the outbox channel just to read shmem_size and
then immediately releases it; mctp_pcc_open() requests the same
channel again on first ifup. A few questions:

This maps and unmaps shmem twice. Could shmem_size be obtained
without binding the channel (for example by inspecting the PCCT
entry) or by acquiring the channel once and holding it across probe
and open?

There is also a small TOCTOU window between probe and ifup where
nothing prevents another mailbox client from grabbing the channel —
pcc_mbox_request_channel() returns -EBUSY if chan->cl is set, so a
later ndo_open would silently fail.

Finally, only the outbox shmem_size is validated at probe time; the
inbox is only validated on first open. Should both be checked at the
same point?

> +	ndev->mtu = MCTP_MIN_MTU;
> +	ndev->max_mtu = mctp_pcc_max_mtu;
> +	ndev->min_mtu = MCTP_MIN_MTU;
> +
> +	return 0;
> +}
> +
> +static int mctp_pcc_driver_add(struct acpi_device *acpi_dev)
> +{
[ ... ]
> +	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;
> +}

[Medium]
acpi_dev->driver_data is set to mctp_pcc_ndev before initialize_mtu()
and mctp_register_netdev() are called. On the goto free_netdev path,
free_netdev(ndev) frees the netdev along with the embedded
mctp_pcc_ndev, but acpi_dev->driver_data still points at that freed
memory. ACPI does not call .remove() when .add() returns non-zero,
so nothing zeroes the pointer afterwards.

Would it be safer to set acpi_dev->driver_data only after all the
fallible operations succeed, or to NULL it on the error path?

> +
> +static const struct acpi_device_id mctp_pcc_device_ids[] = {
> +	{ "DMT0001" },
> +	{}
> +};
> +
> +static struct acpi_driver mctp_pcc_driver = {
> +	.name = "mctp_pcc",
> +	.class = "Unknown",

[Low]
Is .class = "Unknown" intentional? The ACPI subsystem exposes this
string via sysfs and uses it for grouping. A more meaningful value
(for example "mctp_pcc") or omitting the field would seem
preferable for a new in-tree driver.

> +	.ids = mctp_pcc_device_ids,
> +	.ops = {
> +		.add = mctp_pcc_driver_add,
> +	},
> +};
-- 
This is an AI-generated review.