Re: [net-next v44] mctp pcc: Implement MCTP over PCC Transport

[Adam Young]

On 5/28/26 21:44, Jeremy Kerr wrote:
> Hi Adam,
>
> > > [Low]
> > > The Link tag and the body of the commit message both reference
> > > DSP0292_1.0.0WIP50. Is it intentional to merge an in-tree binding
> > > based on a Work-In-Progress draft of the spec rather than the
> > > published 1.0.0?
> This is probably worth an update to reference the non-WIP spec version.

Wilco

> > The ACPICA code used to access the PCC region and registers is imported
> > an external project, and it hard codes the values to machine, not
> > explicitly little endian.  So making this little-endian only ensures
> > that the ordering is correct.  The only machines that are going to
> > support MCTP over PCC are little endian and 64 bit, so this is strictly
> > speaking correct.
> The configuration is less about "which machines you think might use
> this", but more "what does the implementation require".
>
> The LE condition makes sense, as the driver does not support BE. But
> what is requiring 64-bit?
The code itself will not, as written, work on a 32 Bit system, as there 
are 64 bit specific code.  There is no polan to support 64 bit nor a way 
to test it currently, so this is preventative.  Should this change in 
the future, the driver will need to be modified. Limiting the scope.
> > > The header comment says DSP0256 but the URL and the commit message
> > > both say DSP0292. DSP0256 is a different DMTF document (PLDM for
> > > Monitoring and Control). Should the comment read DSP0292?
> > >
> > > [ ... ]
> > The DMTF DSP0256 is the MCTP Host Interface Specification. It
> > standardizes how Management Component Transport Protocol (MCTP) packets
> > and messages are communicated between a system's host software and its
> > internal management controllers (like a BMC or satellite controller).
> > DSP0292 IS specific to the binding. Neither are PLDM. THis comment is
> > correct
> The comment is:
>
>    Implementation of MCTP over PCC DMTF Specification DSP0256
>
> But DSP0256 is not the "MCTP over PCC" transport binding spec. Either
> mention the host interface specifically, or use the correct number for
> the PCC spec. I think the latter would be best, as the host interface
> spec seems less relevant here.

Yeah, just going to drop that line as it is redundant with other info in 
the header.

> > > > +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));
> > > [High]
> > > Can this dereference a NULL shmem during teardown?
> > >
> > > Looking at pcc_mbox_free_channel() in drivers/mailbox/pcc.c, the
> > > channel's shmem is iounmapped (and the pointer cleared) before the
> > > shutdown path runs free_irq(). Until free_irq() returns, an inbound
> > > doorbell on another CPU can still fire pcc_mbox_irq, which calls
> > > mbox_chan_received_data(), which calls chan->cl->rx_callback (cl is
> > > still set at that moment) — landing here.
> > >
> > > Existing PCC clients such as drivers/acpi/acpi_pcc.c::pcc_rx_callback
> > > do not touch shmem from their callback, so this driver appears to be
> > > the first one to expose this ordering.
> > >
> > > Should the rx_callback bail out when inbox->chan->shmem is NULL, or
> > > should the ordering in pcc_mbox_free_channel() (free_irq before
> > > iounmap) be addressed in the mailbox layer first?
> > That needs to be fixed in the PCC layer, and is submitted in a different
> > patch.  When that patch is accepted, this issue will be fixed.  I do not
> > want to hold up this driver for that issue.
> Understood that there's a fix in development for this, but we can't
> introduce a potential null pointer dereference in the interim.
>
> If you want this merged before the fix, how about a guard for a valid
> shmem pointer?

The mutux used to control access to the create-teardown path is static 
to drivers/mailbox/mailbox.c

static DEFINE_MUTEX(con_mutex);

I could run mincore, but we would still have a race condition 
immediately after the mincore call.

The patch that this gates on is:

https://lore.kernel.org/lkml/20260522205220.237355-1-admiyo@xxxxxxxxxxxxxxxxxxxxxx/

Please comment on that one to move it along.


> > Yeah, the Big Endian reference is spurious.  If there is another
> > iteration I will pull it.
> Sounds good.
>
> Cheers,
>
>
> Jeremy