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

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

On 3/25/26 04:49, Jeremy Kerr wrote:
> 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.
Fair enough.  I just thought reviewers might like the chain to the other 
34 versions, as well as documenting the thoroughness that this has been rev
>
>> 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?
Yes, this is a vestige of pulling it out of the Mailbox layer that I 
should have removed.
>
> 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.
Agreed
>
> 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().
Removed.
>
>> +
>> +       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.

The whole "length = length of the payload + 4 bytes for command" logic 
is already confusing.  I felt this was the clearest way to show exactly 
what needed to be checked to ensure we have a valid header.  I guess the 
alternative is

  if (pcc_header.length <= 0)  {
                dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
		return;
  }
size = pcc_header.length - sizeof(pcc_header.command) + sizeof(pcc_header);

Yeah that is clearer.

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

I can't explain it, as I did not debug that deeply.  I will come back to 
this at some point, but it does not affect the proper functioning of the 
driver.


>
> Your driver is still fine to impose a limit if suitable, but you're not
> constrained by the MCTP core here.
I'll remove it.  We can keep it internally until I have the problem 
diagnosed and re-add it if it makes sense.
>
> Cheers,
>
>
> Jeremy