Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data

Adam Young
2025-10-21T17:20:59Z

On 10/21/25 10:02, Sudeep Holla wrote:
> On Mon, Oct 20, 2025 at 01:22:23PM -0400, Adam Young wrote:
>> Answers inline.  Thanks for the review.
>>
>> On 10/20/25 08:52, Sudeep Holla wrote:
>>> On Thu, Oct 16, 2025 at 05:02:20PM -0400, Adam Young wrote:
>>>> Adds functions that aid in compliance with the PCC protocol by
>>>> checking the command complete flag status.
>>>>
>>>> Adds a function that exposes the size of the shared buffer without
>>>> activating the channel.
>>>>
>>>> Adds a function that allows a client to query the number of bytes
>>>> avaialbel to read in order to preallocate buffers for reading.
>>>>
>>>> Signed-off-by: Adam Young <admiyo@os.amperecomputing.com>
>>>> ---
>>>> drivers/mailbox/pcc.c | 129 ++++++++++++++++++++++++++++++++++++++++++
>>>> include/acpi/pcc.h | 38 +++++++++++++
>>>> 2 files changed, 167 insertions(+)
>>>>
>>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>>> index 978a7b674946..653897d61db5 100644
>>>> --- a/drivers/mailbox/pcc.c
>>>> +++ b/drivers/mailbox/pcc.c
>>>> @@ -367,6 +367,46 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>>> return IRQ_HANDLED;
>>>> }
>>>> +static
>>>> +struct pcc_chan_info *lookup_channel_info(int subspace_id)
>>>> +{
>>>> + struct pcc_chan_info *pchan;
>>>> + struct mbox_chan *chan;
>>>> +
>>>> + if (subspace_id < 0 || subspace_id >= pcc_chan_count)
>>>> + return ERR_PTR(-ENOENT);
>>>> +
>>>> + pchan = chan_info + subspace_id;
>>>> + chan = pchan->chan.mchan;
>>>> + if (IS_ERR(chan) || chan->cl) {
>>>> + pr_err("Channel not found for idx: %d\n", subspace_id);
>>>> + return ERR_PTR(-EBUSY);
>>>> + }
>>>> + return pchan;
>>>> +}
>>>> +
>>>> +/**
>>>> + * pcc_mbox_buffer_size - PCC clients call this function to
>>>> + * request the size of the shared buffer in cases
>>>> + * where requesting the channel would prematurely
>>>> + * trigger channel activation and message delivery.
>>>> + * @subspace_id: The PCC Subspace index as parsed in the PCC client
>>>> + * ACPI package. This is used to lookup the array of PCC
>>>> + * subspaces as parsed by the PCC Mailbox controller.
>>>> + *
>>>> + * Return: The size of the shared buffer.
>>>> + */
>>>> +int pcc_mbox_buffer_size(int index)
>>>> +{
>>>> + struct pcc_chan_info *pchan = lookup_channel_info(index);
>>>> +
>>>> + if (IS_ERR(pchan))
>>>> + return -1;
>>>> + return pchan->chan.shmem_size;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pcc_mbox_buffer_size);
>>>> +
>>> Why do you need to export this when you can grab this from
>>> struct pcc_mbox_chan which is returned from pcc_mbox_request_channel().
>>>
>>> Please drop the above 2 functions completely.\
>> This is required by the Network driver. Specifically, the network driver
>> needs to tell the OS what the Max MTU size  is before the network is
>> active.  If I have to call pcc_mbox_request_channel I then activate the
>> channel for message delivery, and we have a race condition.
>>
> No you just need to establish the channel by calling pcc_mbox_request_channel()
> from probe or init routines. After that the shmem size should be available.
> No need to send any message or activating anything.

I guess I can get away with that if I only do it for the type 3...that
should not immediately send an interrupt.  I was thinking that the type
4 could have messages queued up already, and when I request the channel,
I get a flood that I am not ready for.

Ok, I think I can remove the function.

>
>> One alternative I did consider was to return all of the data that you get
>> from  request channel is a non-active format.  For the type 2 drivers, this
>> information is available outside of  the mailbox interface.  The key effect
>> is that the size of the shared message buffer be available without
>> activating the channel.
>>
> Not sure if that is needed.

Not needed.

>
>>>> +
>>>> /**
>>>> * pcc_mbox_request_channel - PCC clients call this function to
>>>> * request a pointer to their PCC subspace, from which they
>>>> @@ -437,6 +477,95 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>>>> }
>>>> EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>>>> +/**
>>>> + * pcc_mbox_query_bytes_available
>>>> + *
>>>> + * @pchan pointer to channel associated with buffer
>>>> + * Return: the number of bytes available to read from the shared buffer
>>>> + */
>>>> +int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan)
>>>> +{
>>>> + struct pcc_extended_header pcc_header;
>>>> + struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
>>>> + int data_len;
>>>> + u64 val;
>>>> +
>>>> + pcc_chan_reg_read(&pinfo->cmd_complete, &val);
>>>> + if (val) {
>>>> + pr_info("%s Buffer not enabled for reading", __func__);
>>>> + return -1;
>>>> + }
>>> Why would you call pcc_mbox_query_bytes_available() if the transfer is
>>> not complete ?
>> Because I need to  allocate a buffer to read the bytes in to.  In the
>> driver, it is called this way.
>>
> Yes I thought so, I think we must be able to manage this with helper as well.
> I will try out some things and share.
>
>> +       size = pcc_mbox_query_bytes_available(inbox->chan);
>> +       if (size == 0)
>> +               return;
>> +       skb = netdev_alloc_skb(mctp_pcc_ndev->ndev, size);
>> +       if (!skb) {
>> +               dev_dstats_rx_dropped(mctp_pcc_ndev->ndev);
>> +               return;
>> +       }
>> +       skb_put(skb, size);
>> +       skb->protocol = htons(ETH_P_MCTP);
>> +       pcc_mbox_read_from_buffer(inbox->chan, size, skb->data);
>>
>> While we could pre-allocate a sk_buff that is MTU size, that is likely to be
>> wasteful for many messages.
>>
> Fair enough.
>
>>>> + memcpy_fromio(&pcc_header, pchan->shmem,
>>>> + sizeof(pcc_header));
>>>> + data_len = pcc_header.length - sizeof(u32) + sizeof(pcc_header);
>>> Why are you adding the header size to the length above ?
>> Because the PCC spec is wonky.
>> https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/14_Platform_Communications_Channel/Platform_Comm_Channel.html#extended-pcc-subspace-shared-memory-region
>>
>> "Length of payload being transmitted including command field."  Thus in
>> order to copy all of the data, including  the PCC header, I need to drop the
>> length (- sizeof(u32) ) and then add the entire header. Having all the PCC
>> data in the buffer allows us to see it in networking tools. It is also
>> parallel with how the messages are sent, where the PCC header is written by
>> the driver and then the whole message is mem-copies in one io/read or write.
>>
> No you have misread this part.
> Communication subspace(only part and last entry in shared memory at offset of
> 16 bytes) - "Memory region for reading/writing PCC data. The maximum size of
> this region is 16 bytes smaller than the size of the shared memory region
> (specified in the Master slave Communications Subspace structure). When a
> command is sent to or received from the platform, the size of the data in
> this space will be Length (expressed above) minus the 4 bytes taken up by
> the command."
>
> The keyword is "this space/region" which refers to only the communication
> subspace which is at offset 16 bytes in the shmem.
>
> It should be just length - sizeof(command) i.e. length - 4

I just want to make sure I have this correct.  I want to copy the entire
PCC buffer, not just the payload, into the sk_buff.  If I wanted the
payload, I would use the length field.  However, I want the PCC header
as well, which is the length field, plus sizeof (header).  But that
double counts the command field, which is part of the header, and thus I
subtract this out.  I think my math is correct. What you wrote would be
for the case where I want only the PCC payload.

The giveaway above is the "offset 16 bytes." As this is the size of the
header.

>
>>>> + return data_len;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pcc_mbox_query_bytes_available);
>>>> +
>>>> +/**
>>>> + * pcc_mbox_read_from_buffer - Copy bytes from shared buffer into data
>>>> + *
>>>> + * @pchan - channel associated with the shared buffer
>>>> + * @len - number of bytes to read
>>>> + * @data - pointer to memory in which to write the data from the
>>>> + * shared buffer
>>>> + *
>>>> + * Return: number of bytes read and written into daa
>>>> + */
>>>> +int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
>>>> +{
>>>> + struct pcc_chan_info *pinfo = pchan->mchan->con_priv;
>>>> + int data_len;
>>>> + u64 val;
>>>> +
>>>> + pcc_chan_reg_read(&pinfo->cmd_complete, &val);
>>>> + if (val) {
>>>> + pr_info("%s buffer not enabled for reading", __func__);
>>>> + return -1;
>>>> + }
>>> Ditto as above, why is this check necessary ?
>> Possibly just paranoia. I think this is vestige of older code that did
>> polling instead of getting an interrupt.  But it seems correct in keeping
>> with the letter of the PCC protocol.
> Not needed IMO, lets add when we find the need for it, not for paranoia
> reasons please.

Will remove.  I think it is safely checked  by the pcc mailbox.

>>>> + data_len = pcc_mbox_query_bytes_available(pchan);
>>>> + if (len < data_len)
>>>> + data_len = len;
>>>> + memcpy_fromio(data, pchan->shmem, len);
>>>> + return len;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pcc_mbox_read_from_buffer);
>>>> +
>>>> +/**
>>>> + * pcc_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.
>>>> + */
>>>> +int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
>>>> +{
>>>> + struct pcc_extended_header *pcc_header = data;
>>>> + struct mbox_chan *mbox_chan = pchan->mchan;
>>>> +
>>>> + /*
>>>> + * The PCC header length includes the command field
>>>> + * but not the other values from the header.
>>>> + */
>>>> + pcc_header->length = len - sizeof(struct pcc_extended_header) + sizeof(u32);
>>>> +
>>>> + if (!pcc_last_tx_done(mbox_chan)) {
>>>> + pr_info("%s pchan->cmd_complete not set.", __func__);
>>>> + return 0;
>>>> + }
>>> The mailbox moves to next message only if the last tx is done. Why is
>>> this check necessary ?
>> I think you are  right, and  these three checks are redundant now.
>>
> Thanks for confirming my understanding, was just worried if there is
> anything that I am not considering.
>
>>>> + memcpy_toio(pchan->shmem, data, len);
>>>> +
>>>> + return len;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pcc_mbox_write_to_buffer);
>>>> +
>>>>
>>> I am thinking if reading and writing to shmem can be made inline helper.
>>> Let me try to hack up something add see how that would look like.
>> That would be a good optimization.
>>
> Thanks, I did try to write to buffer part but I am still not decided on
> the exact formating yet to share it. I will try to share something in
> next couple of days if possible.

Much appreciated.  I will hold off on resubmitting until you do.