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

[Adam Young]

On 6/2/26 00:42, Jeremy Kerr wrote:
> Hi Adam,
>
> > > 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.
> Yeah, that was more my question - what is 64-bit specific about the
> code?

I'd really have to dig, as this decision was mixed in with the earlier 
endinaness conversions.  I suspect, that it was origianally triggered by 
the ACPICA insisting on Machine architecture for these values, where 
they are supposed to be explicitly 32, 64 bit etc.

I can dig a bit more if your feel it really important to support, but I 
think the intersection of ARM32 and MCTP over PCC will remain an empty set.

It might be perfectly safe, but I have no way to test. Treat it as a 
general trend toward not supporting newer technology on older architectures.




> > > 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.
> OK.
>
> > > > > > +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
> You can't acquire that mutex here anyway, you're in atomic context.
Right.  And this would also prevent me from implementing any other mutex.
> > I could run mincore
> No, that wouldn't work (and would be moderately horrific)
I am glad we agree on that point.
> Since you did not answer the guard question,

Sorry, I thought I had.  I have to admit that I misunderstood what you 
meant by "a guard for a valid shmem pointer?"

> are you implying that the
> shmem pointer may be unstable *during* the rx_complete call? (ie.,
> rather than being invalidated before the call, and so a simple `if
> (chan->shmem)` guard would not suffice?)
>
> The Sashiko review seems to be incorrect in that you are not the first
> user of the shmem data; there are other uses of chan->shmem that do not
> seem to have any intrinsic exclusion between the rx callback and the
> free_channel() paths, nor check the validity of shmem in their rx
> callback.
>
> However, the closest existing use - the xgene i2c driver - seems like it
> can not free the pcc mailbox while a rx irq might occur, with the i2c
> locking in place. You could do something similar here, possibly via a
> spinlock acquired in both the ndo_stop handler, and the rx callback.
Could I safely acquire a spinlock in the rx_ callback during interupt 
context? I thought that had a significant impact on the system.
> But if your proposed change to the iounmap() ordering works, that would
> be a neater solution here.

I think it is the right solution.


> > 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.
> As you can probably tell, the pcc mailbox interface is definitely not my
> area of expertise.

It really is the area of expertise of  very few people.  That is one of 
the reason that this patch has taken so long: I have had to learn by 
trial and error.    After the initial work to get the shmem managment 
into the PCC layer, I have tried to avoid changing it, since there are a 
lot of erlier, type 2 drivers that don't seem to get a lot of attention, 
and yet we don't want to risk breaking them.

I thought we were done with PCC  until the Codex based code reviews 
exposed a few issues in the PCC layer. the biggest is the one we discuss 
here, but there is another that I think I need to address as well:  
setting up the interface (specifically calculateing MTU) requires 
opening the channel.  While I know this is safe oin my system (the 
remote side never initiates conversations) it might not be true of all 
senders.  But opening a channel is an indicator to the remote side that 
you are ready to receive.

Thus I have another PCC level patch that I will send on my next 
resubmission that creats a query-only interface.  That, too will require 
an additional change to the MCTP-PCC driver, but one that makes it more  
futureproof.

Looks like this is turning back into a patch series.


> Cheers,
>
>
> Jeremy