Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing PCC extended data
From: Adam Young
Date: Tue Oct 21 2025 - 13:20:59 EST
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.No you just need to establish the channel by calling pcc_mbox_request_channel()
On 10/20/25 08:52, Sudeep Holla wrote:
On Thu, Oct 16, 2025 at 05:02:20PM -0400, Adam Young wrote:This is required by the Network driver. Specifically, the network driver
Adds functions that aid in compliance with the PCC protocol byWhy do you need to export this when you can grab this from
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@xxxxxxxxxxxxxxxxxxxxxx>
---
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);
+
struct pcc_mbox_chan which is returned from pcc_mbox_request_channel().
Please drop the above 2 functions completely.\
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.
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 getNot sure if that is needed.
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 needed.
Yes I thought so, I think we must be able to manage this with helper as well.Because I need to allocate a buffer to read the bytes in to. In the+Why would you call pcc_mbox_query_bytes_available() if the transfer is
/**
* 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;
+ }
not complete ?
driver, it is called this way.
I will try out some things and share.
+ size = pcc_mbox_query_bytes_available(inbox->chan);Fair enough.
+ 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.
No you have misread this part.Because the PCC spec is wonky.+ memcpy_fromio(&pcc_header, pchan->shmem,Why are you adding the header size to the length above ?
+ sizeof(pcc_header));
+ data_len = pcc_header.length - sizeof(u32) + sizeof(pcc_header);
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.
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.
Not needed IMO, lets add when we find the need for it, not for paranoiaPossibly just paranoia. I think this is vestige of older code that did+ return data_len;Ditto as above, why is this check necessary ?
+}
+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;
+ }
polling instead of getting an interrupt. But it seems correct in keeping
with the letter of the PCC protocol.
reasons please.
Will remove. I think it is safely checked by the pcc mailbox.
Thanks for confirming my understanding, was just worried if there isI think you are right, and these three checks are redundant now.+ data_len = pcc_mbox_query_bytes_available(pchan);The mailbox moves to next message only if the last tx is done. Why is
+ 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;
+ }
this check necessary ?
anything that I am not considering.
Thanks, I did try to write to buffer part but I am still not decided onThat would be a good optimization.+ memcpy_toio(pchan->shmem, data, len);I am thinking if reading and writing to shmem can be made inline helper.
+
+ return len;
+}
+EXPORT_SYMBOL_GPL(pcc_mbox_write_to_buffer);
+
Let me try to hack up something add see how that would look like.
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.