Message-ID: <2ef6360e-834f-474d-ac4d-540b8f0c0f79@amperemail.onmicrosoft.com>
Date: Mon, 29 Sep 2025 13:11:23 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: Sudeep Holla <sudeep.holla@....com>, Jassi Brar
 <jassisinghbrar@...il.com>, Adam Young <admiyo@...amperecomputing.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Revert "mailbox/pcc: support mailbox management of the
 shared buffer"

I posted a patch that addresses a few of these issues.  Here is a top 
level description of the isse


The correct way to use the mailbox API would be to allocate a buffer for 
the message,write the message to that buffer, and pass it in to 
mbox_send_message.  The abstraction is designed to then provide 
sequential access to the shared resource in order to send the messages 
in order.  The existing PCC Mailbox implementation violated this 
abstraction.  It requires each individual driver re-implement all of the 
sequential ordering to access the shared buffer.

Why? Because they are all type 2 drivers, and the shared buffer is 
64bits in length:  32bits for signature, 16 bits for command, 16 bits 
for status.  It would be execessive to kmalloc a buffer of this size.

This shows the shortcoming of the mailbox API.  The mailbox API assumes 
that there is a large enough buffer passed in to only provide a void * 
pointer to the message.  Since the value is small enough to fit into a 
single register, it the mailbox abstraction could provide an 
implementation that stored a union of a void * and word.  With that 
change, all of the type 2 implementations could have their logic 
streamlined and moved into the PCC mailbox.

However, I am providing an implementation for a type3/type4 based 
driver, and I do need the whole managmenet of the message buffer. IN 
addition, I know of at least one other subsystem (MPAM) that will 
benefit from a type3 implementation.

On 9/26/25 11:33, Sudeep Holla wrote:
> This reverts commit 5378bdf6a611a32500fccf13d14156f219bb0c85.
>
> Commit 5378bdf6a611 ("mailbox/pcc: support mailbox management of the shared buffer")
> attempted to introduce generic helpers for managing the PCC shared memory,
> but it largely duplicates functionality already provided by the mailbox
> core and leaves gaps:
>
> 1. TX preparation: The mailbox framework already supports this via
>    ->tx_prepare callback for mailbox clients. The patch adds
>    pcc_write_to_buffer() and expects clients to toggle pchan->chan.manage_writes,
>    but no drivers set manage_writes, so pcc_write_to_buffer() has no users.

tx prepare is insufficient, as it does not provide access to the type3 
flags.  IN addition, it forces the user to manage the buffer memory 
directly.  WHile this is a necessary workaround for type 2 non extended 
memory regions, it does not make sense for a managed resource like the 
mailbox.

You are correct that the manage_writes flag can be removed, but if (and 
only if) we limit the logic to type 3 or type 4 drivers.  I have made 
that change in a follow on patch:


> 2. RX handling: Data reception is already delivered through
>     mbox_chan_received_data() and client ->rx_callback. The patch adds an
>     optional pchan->chan.rx_alloc, which again has no users and duplicates
>     the existing path.

The change needs to go in before there are users. The patch series that 
introduced this change requires this or a comparable callback mechanism.

However, the reviewers have shown that there is a race condition if the 
callback is provided to the PCC  mailbox Channel, and thus I have 
provided a patch which moves this callback up to the Mailbox API.  This 
change, which is obviosuly not required when returning a single byte, is 
essential when dealing with larger buffers, such as those used by 
network drivers.

>
> 3. Completion handling: While adding last_tx_done is directionally useful,
>     the implementation only covers Type 3/4 and fails to handle the absence
>     of a command_complete register, so it is incomplete for other types.

Applying it to type 2 and earlier would require a huge life of rewriting 
code that is both  multi architecture (CPPC)  and on esoteric hardware 
(XGene) and thus very hard to test.  While those drivers should make 
better use of  the mailbox mechanism, stopping the type 3 drivers from 
using this approach  stops an effort to provide a common implementation 
base. That should happen in future patches, as part of reqorking the 
type 2 drivers.  Command Complete is part of the PCC specification for 
type 3 drivers.


>
> Given the duplication and incomplete coverage, revert this change. Any new
> requirements should be addressed in focused follow-ups rather than bundling
> multiple behavioral changes together.

I am willing to break up the previous work into multiple steps, provided 
the above arguments you provided are not going to prevent them from 
getting merged.  Type 3/4 drivers can and should make use of the Mailbox 
abstraction. Doing so can lay the ground work for making the type 2 
drivers share a common implementation of the shared buffer management.



>
> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
> ---
>   drivers/mailbox/pcc.c | 102 ++----------------------------------------
>   include/acpi/pcc.h    |  29 ------------
>   2 files changed, 4 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 0a00719b2482..f6714c233f5a 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -306,22 +306,6 @@ static void pcc_chan_acknowledge(struct pcc_chan_info *pchan)
>   		pcc_chan_reg_read_modify_write(&pchan->db);
>   }
>   
> -static void *write_response(struct pcc_chan_info *pchan)
> -{
> -	struct pcc_header pcc_header;
> -	void *buffer;
> -	int data_len;
> -
> -	memcpy_fromio(&pcc_header, pchan->chan.shmem,
> -		      sizeof(pcc_header));
> -	data_len = pcc_header.length - sizeof(u32) + sizeof(struct pcc_header);
> -
> -	buffer = pchan->chan.rx_alloc(pchan->chan.mchan->cl, data_len);
> -	if (buffer != NULL)
> -		memcpy_fromio(buffer, pchan->chan.shmem, data_len);
> -	return buffer;
> -}
> -
>   /**
>    * pcc_mbox_irq - PCC mailbox interrupt handler
>    * @irq:	interrupt number
> @@ -333,8 +317,6 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   {
>   	struct pcc_chan_info *pchan;
>   	struct mbox_chan *chan = p;
> -	struct pcc_header *pcc_header = chan->active_req;
> -	void *handle = NULL;
>   
>   	pchan = chan->con_priv;
>   
> @@ -358,17 +340,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>   	 * required to avoid any possible race in updatation of this flag.
>   	 */
>   	pchan->chan_in_use = false;
> -
> -	if (pchan->chan.rx_alloc)
> -		handle = write_response(pchan);
> -
> -	if (chan->active_req) {
> -		pcc_header = chan->active_req;
> -		if (pcc_header->flags & PCC_CMD_COMPLETION_NOTIFY)
> -			mbox_chan_txdone(chan, 0);
> -	}
> -
> -	mbox_chan_received_data(chan, handle);
> +	mbox_chan_received_data(chan, NULL);
>   
>   	pcc_chan_acknowledge(pchan);
>   
> @@ -412,24 +384,9 @@ pcc_mbox_request_channel(struct mbox_client *cl, int subspace_id)
>   	pcc_mchan = &pchan->chan;
>   	pcc_mchan->shmem = acpi_os_ioremap(pcc_mchan->shmem_base_addr,
>   					   pcc_mchan->shmem_size);
> -	if (!pcc_mchan->shmem)
> -		goto err;
> -
> -	pcc_mchan->manage_writes = false;
> -
> -	/* This indicates that the channel is ready to accept messages.
> -	 * This needs to happen after the channel has registered
> -	 * its callback. There is no access point to do that in
> -	 * the mailbox API. That implies that the mailbox client must
> -	 * have set the allocate callback function prior to
> -	 * sending any messages.
> -	 */
> -	if (pchan->type == ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
> -		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
> -
> -	return pcc_mchan;
> +	if (pcc_mchan->shmem)
> +		return pcc_mchan;
>   
> -err:
>   	mbox_free_channel(chan);
>   	return ERR_PTR(-ENXIO);
>   }
> @@ -460,38 +417,8 @@ void pcc_mbox_free_channel(struct pcc_mbox_chan *pchan)
>   }
>   EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>   
> -static int pcc_write_to_buffer(struct mbox_chan *chan, void *data)
> -{
> -	struct pcc_chan_info *pchan = chan->con_priv;
> -	struct pcc_mbox_chan *pcc_mbox_chan = &pchan->chan;
> -	struct pcc_header *pcc_header = data;
> -
> -	if (!pchan->chan.manage_writes)
> -		return 0;
> -
> -	/* The PCC header length includes the command field
> -	 * but not the other values from the header.
> -	 */
> -	int len = pcc_header->length - sizeof(u32) + sizeof(struct pcc_header);
> -	u64 val;
> -
> -	pcc_chan_reg_read(&pchan->cmd_complete, &val);
> -	if (!val) {
> -		pr_info("%s pchan->cmd_complete not set", __func__);
> -		return -1;
> -	}
> -	memcpy_toio(pcc_mbox_chan->shmem,  data, len);
> -	return 0;
> -}
> -
> -
>   /**
> - * pcc_send_data - Called from Mailbox Controller code. If
> - *		pchan->chan.rx_alloc is set, then the command complete
> - *		flag is checked and the data is written to the shared
> - *		buffer io memory.
> - *
> - *		If pchan->chan.rx_alloc is not set, then it is used
> + * pcc_send_data - Called from Mailbox Controller code. Used
>    *		here only to ring the channel doorbell. The PCC client
>    *		specific read/write is done in the client driver in
>    *		order to maintain atomicity over PCC channel once
> @@ -507,37 +434,17 @@ static int pcc_send_data(struct mbox_chan *chan, void *data)
>   	int ret;
>   	struct pcc_chan_info *pchan = chan->con_priv;
>   
> -	ret = pcc_write_to_buffer(chan, data);
> -	if (ret)
> -		return ret;
> -
>   	ret = pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>   	if (ret)
>   		return ret;
>   
>   	ret = pcc_chan_reg_read_modify_write(&pchan->db);
> -
>   	if (!ret && pchan->plat_irq > 0)
>   		pchan->chan_in_use = true;
>   
>   	return ret;
>   }
>   
> -
> -static bool pcc_last_tx_done(struct mbox_chan *chan)
> -{
> -	struct pcc_chan_info *pchan = chan->con_priv;
> -	u64 val;
> -
> -	pcc_chan_reg_read(&pchan->cmd_complete, &val);
> -	if (!val)
> -		return false;
> -	else
> -		return true;
> -}
> -
> -
> -
>   /**
>    * pcc_startup - Called from Mailbox Controller code. Used here
>    *		to request the interrupt.
> @@ -583,7 +490,6 @@ static const struct mbox_chan_ops pcc_chan_ops = {
>   	.send_data = pcc_send_data,
>   	.startup = pcc_startup,
>   	.shutdown = pcc_shutdown,
> -	.last_tx_done = pcc_last_tx_done,
>   };
>   
>   /**
> diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
> index 9af3b502f839..840bfc95bae3 100644
> --- a/include/acpi/pcc.h
> +++ b/include/acpi/pcc.h
> @@ -17,35 +17,6 @@ struct pcc_mbox_chan {
>   	u32 latency;
>   	u32 max_access_rate;
>   	u16 min_turnaround_time;
> -
> -	/* Set to true to indicate that the mailbox should manage
> -	 * writing the dat to the shared buffer. This differs from
> -	 * the case where the drivesr are writing to the buffer and
> -	 * using send_data only to  ring the doorbell.  If this flag
> -	 * is set, then the void * data parameter of send_data must
> -	 * point to a kernel-memory buffer formatted in accordance with
> -	 * the PCC specification.
> -	 *
> -	 * The active buffer management will include reading the
> -	 * notify_on_completion flag, and will then
> -	 * call mbox_chan_txdone when the acknowledgment interrupt is
> -	 * received.
> -	 */
> -	bool manage_writes;
> -
> -	/* Optional callback that allows the driver
> -	 * to allocate the memory used for receiving
> -	 * messages.  The return value is the location
> -	 * inside the buffer where the mailbox should write the data.
> -	 */
> -	void *(*rx_alloc)(struct mbox_client *cl,  int size);
> -};
> -
> -struct pcc_header {
> -	u32 signature;
> -	u32 flags;
> -	u32 length;
> -	u32 command;
>   };
>   
>   /* Generic Communications Channel Shared Memory Region */

Powered by blists - more mailing lists