Re: [net-next v32 2/2] mctp pcc: Implement MCTP over PCC Transport

[Jeremy Kerr]

Hi Adam,

This is looking pretty concise now, nice work. A few things inline:

> 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..d0e7699ebe82
> --- /dev/null
> +++ b/drivers/net/mctp/mctp-pcc.c
> @@ -0,0 +1,322 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * mctp-pcc.c - Driver for MCTP over PCC.
> + * Copyright (c) 2024-2025, Ampere Computing LLC

Minor, but if you end up re-rolling: -2026.

> + *
> + */
> +
> +/* 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 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;
> +       size = pcc_mbox_query_bytes_available(inbox->chan);
> +       if (size == 0)
> +               return;

Your new pcc_mbox_query_bytes_available() returns -1 on error, but you
don't handle that here.


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

Do you want to check that the header describes a valid MCTP-over-PCC
packet?

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

If you return NETDEV_TX_BUSY here, and the core does a retransmit,
you will end up prepending another header.

Just to confirm from earlier discussions: you have no way to determine
that the channel is unavailable in advance, right?

> +       }
> +
> +       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)
> +               netdev_info(mctp_pcc_ndev->ndev,  "packet dropped");

You probably don't want a netdev_info() here, perhaps netdev_dbg().

Or, could you increment the tx_dropped stat? I see you're doing all the
stats accounting in start_xmit - I'm not familiar with the PCC API, but
there seem to be failure paths past that point. Is it possible to
distinguish between successful tx and dropped tx in the tx_done
callback, and do the accounting there, perhaps?

Or is this a trivial-enough case (len > shmem_size) to not worry about
accounting?

Cheers,


Jeremy