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

[Jeremy Kerr]

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.

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

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

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

Sounds good.

Cheers,


Jeremy