Message-ID: <78c30517-4b16-4929-b10b-917da68ff01c@amperemail.onmicrosoft.com>
Date: Mon, 20 Oct 2025 13:22:23 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: Sudeep Holla <sudeep.holla@....com>,
 Adam Young <admiyo@...amperecomputing.com>
Cc: Jassi Brar <jassisinghbrar@...il.com>,
 "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 Robert Moore <robert.moore@...el.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, Jeremy Kerr <jk@...econstruct.com.au>,
 Matt Johnston <matt@...econstruct.com.au>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Huisong Li <lihuisong@...wei.com>
Subject: Re: [PATCH v30 2/3] mailbox: pcc: functions for reading and writing
 PCC extended data

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@...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.



>

Powered by blists - more mailing lists