Re: [net-next v35 1/1] mctp pcc: Implement MCTP over PCC Transport

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Adam,

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.

> diff --git a/drivers/net/mctp/mctp-pcc.c b/drivers/net/mctp/mctp-pcc.c
> 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);

Minor, but you're using two different calculations for this: here,
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?

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.

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

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

This condition (size <= sizeof(pcc_header)) reads a bit strangely given
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.

> +       if (memcmp(&pcc_header.command, MCTP_SIGNATURE, MCTP_SIGNATURE_LENGTH) != 0) {
> +               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");

We only got half way through the discussion on this, my bad. In that,
you mentioned:

> They get dropped by the MCTP layer in the Kernel, apparently, so the
> dev-dbg is to help someone debug why.

... which isn't the case. We're fine to receive packets larger than the
MTU. It's a MTU, not a MRU :)

Your driver is still fine to impose a limit if suitable, but you're not
constrained by the MCTP core here.

Cheers,


Jeremy