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);
+
+
/**
* 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;
+ }
+ memcpy_fromio(&pcc_header, pchan->shmem,
+ sizeof(pcc_header));
+ data_len = pcc_header.length - sizeof(u32) + sizeof(pcc_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;
+ }
+ 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;
+ }
+ memcpy_toio(pchan->shmem, data, len);
+
+ return len;
+}
+EXPORT_SYMBOL_GPL(pcc_mbox_write_to_buffer);
+
+
/**
* pcc_send_data - Called from Mailbox Controller code. Used
* here only to ring the channel doorbell. The PCC client
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 840bfc95bae3..96a6f85fc1ba 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -19,6 +19,13 @@ struct pcc_mbox_chan {
u16 min_turnaround_time;
};
+struct pcc_extended_header {
+ u32 signature;
+ u32 flags;
+ u32 length;
+ u32 command;
+};
+
/* Generic Communications Channel Shared Memory Region */
#define PCC_SIGNATURE 0x50434300
/* Generic Communications Channel Command Field */
@@ -37,6 +44,17 @@ struct pcc_mbox_chan {
extern struct pcc_mbox_chan *
pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id);
extern void pcc_mbox_free_channel(struct pcc_mbox_chan *chan);
+extern
+int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data);
+extern
+int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan);
+extern
+int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len,
+ void *data);
+extern
+int pcc_mbox_buffer_size(int index);
+
+
#else
static inline struct pcc_mbox_chan *
pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
@@ -44,6 +62,26 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
return ERR_PTR(-ENODEV);
}
static inline void pcc_mbox_free_channel(struct pcc_mbox_chan *chan) { }
+static inline
+int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
+{
+ return 0;
+}
+static inline int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan);
+{
+ return 0;
+}
+static inline
+int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
+{
+ return 0;
+}
+static inline
+int pcc_mbox_buffer_size(int index)
+{
+ return -1;
+}
+
#endif
#endif /* _PCC_H */
--
2.43.0On 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.
> +
> /**
> * 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 ?
> + 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 ?
> + 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 ?
> + 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 ?
> + 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.
> /**
> * pcc_send_data - Called from Mailbox Controller code. Used
> * here only to ring the channel doorbell. The PCC client
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 840bfc95bae3..96a6f85fc1ba 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -19,6 +19,13 @@ struct pcc_mbox_chan {
> u16 min_turnaround_time;
> };
>
> +struct pcc_extended_header {
> + u32 signature;
> + u32 flags;
> + u32 length;
> + u32 command;
> +};
> +
This again is a duplicate of struct acpi_pcct_ext_pcc_shared_memory.
It can be dropped.
--
Regards,
Sudeep
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.
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.
>
>> +
>> /**
>> * 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.
+ 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.
>
>> + 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.
>
>> + 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.
>
>> + 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.
>
>> + 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.
>
>> /**
>> * pcc_send_data - Called from Mailbox Controller code. Used
>> * here only to ring the channel doorbell. The PCC client
>> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
>> index 840bfc95bae3..96a6f85fc1ba 100644
>> --- a/include/acpi/pcc.h
>> +++ b/include/acpi/pcc.h
>> @@ -19,6 +19,13 @@ struct pcc_mbox_chan {
>> u16 min_turnaround_time;
>> };
>>
>> +struct pcc_extended_header {
>> + u32 signature;
>> + u32 flags;
>> + u32 length;
>> + u32 command;
>> +};
>> +
> This again is a duplicate of struct acpi_pcct_ext_pcc_shared_memory.
> It can be dropped.
Will do.
>
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.
> 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.
>
> >
> > > +
> > > /**
> > > * 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
> >
> > > + 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.
>
> >
> > > + 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.
--
Regards,
Sudeep
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.
On Tue, Oct 21, 2025 at 01:20:50PM -0400, Adam Young wrote: [...] > > > 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. > Why ? How does sk_buff interpret that as PCC header. Something doesn't align well here or I might be missing something. I started writing some helpers and this comment made me to rethink my approach. I don't have any to share and I will be away for a while. I will try to review any further changes from you but expect delays. -- Regards, Sudeep
On 10/17/2025 2:32 AM, 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.
/s available
>
> 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)
use subspace_id for consistent name and use in header
> +{
> + 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);
> +
> +
> /**
> * 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;
> + }
> + memcpy_fromio(&pcc_header, pchan->shmem,
> + sizeof(pcc_header));
> + data_len = pcc_header.length - sizeof(u32) + sizeof(pcc_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
typo daa -> data
> + */
> +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;
> + }
> + 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;
> + }
> + memcpy_toio(pchan->shmem, data, len);
> +
> + return len;
> +}
> +EXPORT_SYMBOL_GPL(pcc_mbox_write_to_buffer);
> +
> +
> /**
> * pcc_send_data - Called from Mailbox Controller code. Used
> * here only to ring the channel doorbell. The PCC client
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 840bfc95bae3..96a6f85fc1ba 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -19,6 +19,13 @@ struct pcc_mbox_chan {
> u16 min_turnaround_time;
> };
>
> +struct pcc_extended_header {
> + u32 signature;
> + u32 flags;
> + u32 length;
> + u32 command;
> +};
> +
> /* Generic Communications Channel Shared Memory Region */
> #define PCC_SIGNATURE 0x50434300
> /* Generic Communications Channel Command Field */
> @@ -37,6 +44,17 @@ struct pcc_mbox_chan {
> extern struct pcc_mbox_chan *
> pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id);
> extern void pcc_mbox_free_channel(struct pcc_mbox_chan *chan);
> +extern
> +int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data);
> +extern
> +int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan);
> +extern
> +int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len,
> + void *data);
> +extern
> +int pcc_mbox_buffer_size(int index);
> +
> +
> #else
> static inline struct pcc_mbox_chan *
> pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
> @@ -44,6 +62,26 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
> return ERR_PTR(-ENODEV);
> }
> static inline void pcc_mbox_free_channel(struct pcc_mbox_chan *chan) { }
> +static inline
> +int pcc_mbox_write_to_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
> +{
> + return 0;
> +}
> +static inline int pcc_mbox_query_bytes_available(struct pcc_mbox_chan *pchan);
remove ;
> +{
> + return 0;
> +}
> +static inline
> +int pcc_mbox_read_from_buffer(struct pcc_mbox_chan *pchan, int len, void *data)
> +{
> + return 0;
> +}
> +static inline
> +int pcc_mbox_buffer_size(int index)
> +{
> + return -1;
> +}
> +
> #endif
>
> #endif /* _PCC_H */
Thanks,
Alok
© 2016 - 2025 Red Hat, Inc.