Re: [net-next v39] mctp pcc: Implement MCTP over PCC Transport
From: Jeremy Kerr
Date: Wed May 06 2026 - 23:02:55 EST
Hi Adam,
> +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;
> + u32 header_length;
> + 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));
> +
> + // 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.
> + header_length = le32_to_cpu(pcc_header.length);
> + if (header_length < sizeof(pcc_header.command) + sizeof(struct mctp_hdr)) {
> + dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> + return;
> + }
> + // If the reported size is larger than the shared memory minus headers,
> + // something is wrong and treat the buffer as corrupted data.
> + if (header_length > inbox->chan->shmem_size - PCC_EXTRA_LEN) {
> + dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> + return;
> + }
> + if (memcmp(&pcc_header.command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH) != 0) {
> + dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> + return;
> + }
> +
> + size = header_length + PCC_EXTRA_LEN;
> + skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
> + if (!skb) {
> + dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
> + return;
> + }
Minor, but you have four instances of the dstats_rx_dropped call, that
may work as a shared error path.
> + skb_put(skb, size);
> + skb->protocol = htons(ETH_P_MCTP);
> + memcpy_fromio(skb->data, inbox->chan->shmem, size);
> + 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);
> +}
> +
> +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;
> +
> + /* Consolidated a fragmented packet into contiguous memory */
> + if (skb_is_nonlinear(skb)) {
> + if (skb_linearize(skb))
> + goto error;
> + }
skb_linearize() already has the skb_is_nonlinear() check.
However, you don't need to call skb_linearize() anyway, as that will
happen for you in validate_xmit_skb(), since the driver does not
advertise support for nonlinear skbs.
> +
> + if (skb_cow_head(skb, sizeof(*pcc_header)))
> + goto error;
> +
> + /**
> + * This code could potentially be run on A Big Endian
> + * System. The ACPI specification requires that values
> + * in the shared buffer be little endian.
> + */
Minor, but I don't think this comment is necessary.
More importantly though, you're getting a bunch of warnings from the use
of these endian helpers:
drivers/net/mctp/mctp-pcc.c:70:25: warning: cast to restricted __le32
drivers/net/mctp/mctp-pcc.c:125:31: warning: incorrect type in assignment (different base types)
drivers/net/mctp/mctp-pcc.c:125:31: expected unsigned int [usertype] signature
drivers/net/mctp/mctp-pcc.c:125:31: got restricted __le32 [usertype]
drivers/net/mctp/mctp-pcc.c:126:27: warning: incorrect type in assignment (different base types)
drivers/net/mctp/mctp-pcc.c:126:27: expected unsigned int [usertype] flags
drivers/net/mctp/mctp-pcc.c:126:27: got restricted __le32 [usertype]
drivers/net/mctp/mctp-pcc.c:128:28: warning: incorrect type in assignment (different base types)
drivers/net/mctp/mctp-pcc.c:128:28: expected unsigned int [usertype] length
drivers/net/mctp/mctp-pcc.c:128:28: got restricted __le32 [usertype]
Looks like struct acpi_pcct_ext_pcc_shared_memory does not specify
fields as little endian, but host endian.
You could change to endian-annotated types to those ACPI structs, but
there are probably reasons why that's not already done. This might be an
indication that there are no big-endian ACPI platforms, so you don't
need the endian conversions?
> + pcc_header = skb_push(skb, sizeof(*pcc_header));
> + pcc_header->signature = cpu_to_le32(PCC_SIGNATURE | mpnd->outbox.index);
> + pcc_header->flags = cpu_to_le32(PCC_CMD_COMPLETION_NOTIFY);
> + memcpy(&pcc_header->command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH);
> + pcc_header->length = cpu_to_le32(len + MCTP_SIGNATURE_LENGTH);
> +
> + if (mbox_send_message(mpnd->outbox.chan->mchan, skb) < 0) {
> + // Remove the header in case it gets sent again
> + skb_pull(skb, sizeof(*pcc_header));
> + netif_stop_queue(ndev);
> + return NETDEV_TX_BUSY;
> + }
> +
> + return NETDEV_TX_OK;
> +error:
> + dev_dstats_tx_dropped(ndev);
> + kfree_skb(skb);
> + 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.
> + */
> + if (!skb) {
> + WARN_ONCE(!skb, "%s called with null message.\n", __func__);
> + return;
> + }
I'd suggest netdev_warn_once(), so we know which device is causing this.
> +
> + if (skb->len > outbox->chan->shmem_size) {
> + dev_dstats_tx_dropped(mctp_pcc_ndev->ndev);
> + return;
> + }
> + memcpy_toio(outbox->chan->shmem, skb->data, skb->len);
Super minor: double space before skb->data there.
> + dev_dstats_tx_add(mctp_pcc_ndev->ndev, skb->len);
> +}
> +
> +static void mctp_pcc_tx_done(struct mbox_client *c, void *mssg, int r)
> +{
> + struct mctp_pcc_ndev *mctp_pcc_ndev;
> + struct sk_buff *skb = mssg;
> +
> + /*
> + * If there is a packet in flight during driver cleanup
> + * It may have been freed already.
> + */
> + if (!mssg)
> + return;
> + /*
> + * If the return code is non-zero, we should not report the packet
> + * as transmitted. However, we are in IRQ context right now, and we
> + * cannot safely write transmission statistics.
> + */
This reads as if you're not updating stats at all, but you do so in
mctp_pcc_tx_prepare(). I don't think this comment is necessary - if
you really want to mention this, add a comment on the
dev_dstats_tx_add() to indicate why you're calling it early.
> + mctp_pcc_ndev = container_of(c, struct mctp_pcc_ndev, outbox.client);
> + dev_consume_skb_any(skb);
> + netif_wake_queue(mctp_pcc_ndev->ndev);
> +}
> +
> +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) {
Minor: odd spacing here.
> + 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;
> + }
> + 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);
> + scoped_guard(spinlock_irqsave, &chan->lock) {
> + skb = chan->active_req;
> + chan->active_req = NULL;
> + if (skb) {
> + 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--;
> + }
> + }
I'm going to assume this is all OK with respect to PCC channel activity.
> + pcc_mbox_free_channel(mctp_pcc_ndev->outbox.chan);
> + return 0;
> +}
> +
> +static int mctp_pcc_change_mtu(struct net_device *dev, int new_mtu)
> +{
> + if (new_mtu > dev->max_mtu)
> + return -EINVAL;
> + if (new_mtu < MCTP_MIN_MTU)
> + return -EINVAL;
> + return 0;
> +}
The core already does these checks, I don't think you need a
ndo_change_mtu callback at all.
Cheers,
Jeremy