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.

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.

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.

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.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.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 */
-- 
2.34.1

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@arm.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 */

On Mon, Sep 29, 2025 at 01:11:23PM -0400, Adam Young wrote:
> 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.
> 

Please, let us keep the avoiding duplication as a separate topic atleast for
the discussion. We can take care of it even before merging if you prefer that
way but we need to explore what other drivers can use it. Otherwise it is
not yet duplication right ?

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

Sure, if there is only and first driver needing large buffers, it is still
not duplication yet. I agree it can be moved to PCC, but lets start with
you client driver code first and then take it from there.

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

No, it is left to the client driver interpretation as it clearly varies even
within PCC type 1-5. Again, let us start with client driver code and see how
to standardise later. I agree with PCC being standard, there is scope for
avoiding duplication, but we will get to know that only if you first present
it with the client driver code and we can then see how and what to make
generic.

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

Don't even go there. It is much bigger beast with all sorts of things to
consider. Now that you have mentioned that, I am interested more to look
at MPAM driver usage as well before merging anything as generic as I know
MPAM is not so trivial. You pulled that topic into this, sorry 😉.

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

Sorry if I am slow in understanding but I still struggle why tx_prepare won't
work for you. Please don't jump to solve 2 problems at the same time as it
just adds more confusion. Let us see if and how to make tx_prepare work for
your driver. And then we can look at standardising it as a helper function
that can be use in all the PCC mailbox client drivers if we can do that.

You are just adding parallel and optional APIs just to get your driver
working here. I am not against standardising to avoid duplication which
is your concern(very valid) but doen't need to be solved by adding another
API when the existing APIs already provides mechanism to do that.

If you need information about the PCC type3/4, we can explore that as well.

> 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:
> 

OK, but I would like to start fresh reverting this 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.
> 

Not always necessary. Yes if it is agreed to get the user merged. But I am
now questioning why you need it when you do have rx_callback.

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

Sorry if I have missed it. Can you please point me to the race condition in
question. I am interested to know more details.

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

I assume it can't be beyond the shmem area anyways. That can be read from the
rx_callback. Again I haven't understood your reasoning as why the allocation
and copy can't be part of rx_callback.

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

True but you have changed the generic code which could break Type1/2 PCC.
I am not sure if it is tested yet.

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

No you need to take care to apply your changes only for Type3/4 so that
Type1/2 is unaffected. You can expect to break and someone else to fix
the breakage later.

> Command Complete is part of the PCC specification for type 3 drivers.
>

Agreed, that's not the argument. The check is done unconditionally. I will
send the patch once we agree to revert this change and start fresh. And each
feature handled separately instead of mixing 3 different things in one patch.

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

Sure. Lets revert this patch and start discussing your individual requirements
in individual patches and check why tx_prepare and rx_callback can't work for
you. Please share the client driver code changes you tried when checking
tx_prepare and rx_callback as well so that we can see why it can't work.

-- 
Regards,
Sudeep

ACK to reverting. I will submit the changes again, and in separate patches.

I will try to address our comments in line, so we have continuity of 
discussion.  We can point to these messages from future reviews.

On 9/30/25 05:37, Sudeep Holla wrote:
> On Mon, Sep 29, 2025 at 01:11:23PM -0400, Adam Young wrote:
>> 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.
>>
> Please, let us keep the avoiding duplication as a separate topic atleast for
> the discussion. We can take care of it even before merging if you prefer that
> way but we need to explore what other drivers can use it. Otherwise it is
> not yet duplication right ?
>
>> 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.
>>
> Sure, if there is only and first driver needing large buffers, it is still
> not duplication yet. I agree it can be moved to PCC, but lets start with
> you client driver code first and then take it from there.
>
>> 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.
>>
> No, it is left to the client driver interpretation as it clearly varies even
> within PCC type 1-5. Again, let us start with client driver code and see how
> to standardise later. I agree with PCC being standard, there is scope for
> avoiding duplication, but we will get to know that only if you first present
> it with the client driver code and we can then see how and what to make
> generic.
>
>> 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.
>>
> Don't even go there. It is much bigger beast with all sorts of things to
> consider. Now that you have mentioned that, I am interested more to look
> at MPAM driver usage as well before merging anything as generic as I know
> MPAM is not so trivial. You pulled that topic into this, sorry 😉.

This actually got me to laugh.  Nervously.  I wonder what else is coming.


>
>> 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.
>>
> Sorry if I am slow in understanding but I still struggle why tx_prepare won't
> work for you. Please don't jump to solve 2 problems at the same time as it
> just adds more confusion. Let us see if and how to make tx_prepare work for
> your driver. And then we can look at standardising it as a helper function
> that can be use in all the PCC mailbox client drivers if we can do that.
>
> You are just adding parallel and optional APIs just to get your driver
> working here. I am not against standardising to avoid duplication which
> is your concern(very valid) but doen't need to be solved by adding another
> API when the existing APIs already provides mechanism to do that.
>
> If you need information about the PCC type3/4, we can explore that as well.

I will submit a more detailed explaination when I resubmit that 
functionality.

The short of it is that the Type3 Register information is in the PCCT, 
and that is not available outside mailbox/pcc.c

For Type 2, there is an accessor function that exposes if the buffer is 
safe to write.  For Type 3, you need the command completion fields and 
registers,...and there are two of them, one for setting and one for 
reading.

Without this field, you cannot safely write to the shared buffer in 
tx_prepare.  I am attempting to write it in such a way that it works for 
any extended memory driver.  What I followed here is the PCC spec, not 
anything specific to the network driver I  submitted to call it.

So, yes, the alternative is to create a new accessor function that 
returns the cmd completion bit, but that would need to be called  from 
inside of a spin lock.




>
>> 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:
>>
> OK, but I would like to start fresh reverting this patch.

I can see the value in that, and support the decision.



>
>>> 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.
>>
> Not always necessary. Yes if it is agreed to get the user merged. But I am
> now questioning why you need it when you do have rx_callback.

RX callback is optional.  For large buffers, we want to let the driver 
specify how to allocate the buffers.   RX Callback will tell the driver 
that there is data, but would extend the pattern of requiring direct 
IO-memory access instead of using the message parameter.


>
>> 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.
> Sorry if I have missed it. Can you please point me to the race condition in
> question. I am interested to know more details.

The review in question was on Re: [PATCH net-next v28 1/1] mctp pcc: 
Implement MCTP over PCC Transport

Jeremy's comment was:

"Also: you're setting the client rx_callback *after*having set up the 
PCC channel. Won't this race with RX on the inbox?"

And he is right.  If you bring up a driver when the platform has 
messages ready to send, the alloc function needs to be available as soon 
as the mailbox is active.  If not, there will be a race between message 
delivery and the assignment of the alloc function.

That is why I am proposing a change to the mailbox API.  I realize that 
this greatly increases the scope of the discussion. However, without 
providing the driver some way to specify how to allocate large buffers,  
message deliver becomes much more complicated.  Essentially, the mailbox 
needs to hard code a message allocation scheme, and that means that a 
mechanism like PCC cannot handle different allocation schemes.   Since 
the driver I am writing is a network driver, the right type of buffer is 
of type struct sk_buff.  I would not want to make all PCC type 3 drivers 
use struct sk_buff, obviously.  I wanted to limit the change to the PCC 
mailbox, but it does not appear to be possible without the race condition.

The change to the mailbox api was submitted in a change titled
  [PATCH net-next v29 1/3] mailbox: add callback function for rx buffer 
allocation


>
>> 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.
>>
> I assume it can't be beyond the shmem area anyways. That can be read from the
> rx_callback. Again I haven't understood your reasoning as why the allocation
> and copy can't be part of rx_callback.

That is correct.  It is only the shared memory region.  Yes, it can be 
read from rx_callback.  But the protocol is more complex than just 
reading the buffer, and I was  trying to write it in a reusable fashion 
and inaccordance with the intention of the mailbox API.

>
>>> 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.
> True but you have changed the generic code which could break Type1/2 PCC.
> I am not sure if it is tested yet.
I did basic testing: CPPC still works correctly on the systems that this 
code runs on.
>
>> 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.
> No you need to take care to apply your changes only for Type3/4 so that
> Type1/2 is unaffected. You can expect to break and someone else to fix
> the breakage later.

Agreed.  The code was written to only affect the path for Type3/4 
interfaces.

The managed_writes flag was originally for just that reason: make the 
code explicitly opt in.  However, the write (outgoing) message flow is 
only changed for type 3, and thus managed_writes is not needed.   I 
would suggest a standard that the mssg is NULL for cases where the 
driver is not going to actually send the data via mbox_send_message and 
instead is going to write the buffer directly.

  The write_response path was only taken if the rx_alloc callback is 
set, and cannot happen for a pre-existing driver that does not set that 
callback.


>
>> Command Complete is part of the PCC specification for type 3 drivers.
>>
> Agreed, that's not the argument. The check is done unconditionally. I will
> send the patch once we agree to revert this change and start fresh. And each
> feature handled separately instead of mixing 3 different things in one patch.

The check happens in pcc_write_to_buffer and only

if (!pchan->chan.manage_writes)

However, we can and should make that check be that the channel is a 
type3/4 instead.  I did that in another patch, which I will replicate 
post revert.

>
>>> 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.
>>
> Sure. Lets revert this patch and start discussing your individual requirements
> in individual patches and check why tx_prepare and rx_callback can't work for
> you. Please share the client driver code changes you tried when checking
> tx_prepare and rx_callback as well so that we can see why it can't work.

Thanks for your attention and feedback.  Much appreciated.

On Mon, Sep 29, 2025 at 12:11 PM Adam Young
<admiyo@amperemail.onmicrosoft.com> wrote:
>
> 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.
>
Mailbox api does not make assumptions about the format of message
hence it simply asks for void*.
Probably I don't understand your requirement, but why can't you pass the pointer
to the 'word' you want to use otherwise?

-jassi

On 9/29/25 20:19, Jassi Brar wrote:
> On Mon, Sep 29, 2025 at 12:11 PM Adam Young
> <admiyo@amperemail.onmicrosoft.com> wrote:
>> 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.
>>
> Mailbox api does not make assumptions about the format of message
> hence it simply asks for void*.
> Probably I don't understand your requirement, but why can't you pass the pointer
> to the 'word' you want to use otherwise?
>
> -jassi
The mbox_send_message call will then take the pointer value that you 
give it and put it in a ring buffer.  The function then returns, and the 
value may be popped off the stack before the message is actually sent.  
In practice we don't see this because much of the code that calls it is 
blocking code, so the value stays on the stack until it is read.  Or, in 
the case of the PCC mailbox, the value is never read or used.  But, as 
the API is designed, the memory passed into to the function should 
expect to live longer than the function call, and should not be 
allocated on the stack.

On Wed, Oct 1, 2025 at 12:25 AM Adam Young
<admiyo@amperemail.onmicrosoft.com> wrote:
>
>
> On 9/29/25 20:19, Jassi Brar wrote:
> > On Mon, Sep 29, 2025 at 12:11 PM Adam Young
> > <admiyo@amperemail.onmicrosoft.com> wrote:
> >> 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.
> >>
> > Mailbox api does not make assumptions about the format of message
> > hence it simply asks for void*.
> > Probably I don't understand your requirement, but why can't you pass the pointer
> > to the 'word' you want to use otherwise?
> >
> The mbox_send_message call will then take the pointer value that you
> give it and put it in a ring buffer.  The function then returns, and the
> value may be popped off the stack before the message is actually sent.
> In practice we don't see this because much of the code that calls it is
> blocking code, so the value stays on the stack until it is read.  Or, in
> the case of the PCC mailbox, the value is never read or used.  But, as
> the API is designed, the memory passed into to the function should
> expect to live longer than the function call, and should not be
> allocated on the stack.
>
Mailbox api doesn't dictate the message format, so it simply accepts the message
pointer from the client and passes that to the controller driver. The
message, pointed
to by the submitted pointer, should be available to the controller
driver until transmitted.
So yes, the message should be allocated either not on stack or, if on stack, not
popped until tx_done. You see it as a "shortcoming" because your
message is simply
a word that you want to submit and be done with.

On Wed, Oct 01, 2025 at 01:25:42AM -0400, Adam Young wrote:
> 
> On 9/29/25 20:19, Jassi Brar wrote:
> > On Mon, Sep 29, 2025 at 12:11 PM Adam Young
> > <admiyo@amperemail.onmicrosoft.com> wrote:
> > > 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.
> > > 
> > Mailbox api does not make assumptions about the format of message
> > hence it simply asks for void*.
> > Probably I don't understand your requirement, but why can't you pass the pointer
> > to the 'word' you want to use otherwise?
> > 
> > -jassi
> The mbox_send_message call will then take the pointer value that you give it
> and put it in a ring buffer.  The function then returns, and the value may
> be popped off the stack before the message is actually sent.  In practice we
> don't see this because much of the code that calls it is blocking code, so
> the value stays on the stack until it is read.  Or, in the case of the PCC
> mailbox, the value is never read or used.  But, as the API is designed, the
> memory passed into to the function should expect to live longer than the
> function call, and should not be allocated on the stack.

I’m still not clear on what exactly you are looking for. Let’s look at
mbox_send_message(). It adds the provided data pointer to the queue, and then
passes the same pointer to tx_prepare() just before calling send_data(). This
is what I’ve been pointing out that you can obtain the buffer pointer there and
use it to update the shared memory in the client driver.

-- 
Regards,
Sudeep