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

[Adam Young <admiyo@amperemail.onmicrosoft.com>]

On 3/3/26 20:12, Jeremy Kerr wrote:
> 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.
Lets hope that it goes not further with this first commit.
>
>> + *
>> + */
>> +
>> +/* 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.

I've dropped QUery bytes, as well as the the other help functions from 
mailbox/pcc.  This error case didn't really make sense on its own.


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

Yes I do.  Specifically, I can now check that the packet is both a PCC 
and and MCTP packet by looking at the signature.

Of course, that data is written into a shared buffer, and I don't think 
there is an safe way to check that the data has actually updated (no 
sequence numbers).  But this is an improvement, and will catch some data 
corruption issues.

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

Good catch.  I can skb_pull the pcc header out before returning the packet.


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

That is correct.  The failure mode here would be if the ring buffer was 
full, which could happen if we have too many incoming packets and the 
backend is processing them slowly. mbox_send_message will only fail if 
the the ring buffer is full, but does not check anything further along 
in the process.  So A TX_BUSY seems appropriate.


>
>> +       }
>> +
>> +       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().

I will remove logging for now, as I don't report on other dropped packets.


>
> 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?

I think that stats can move here from start_xmit.  This is the point 
that we know that the packet has actually made it across the wire or 
not, so recording success or failure here is correct.


I just realized that I am reporting the number of bytes sent including 
the pcc header.  I suspect that will mess up the byte accounting: these 
are being reported on MCTP links, and they have no way of knowing that 
they should be removing the PCC header length.  Should I be removing the 
pcc Header length from the byte counts for packets-sent?


>
> Cheers,
>
>
> Jeremy