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

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

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@os.amperecomputing.com/
>>
>> 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