Message-ID: <2456ece8-0490-4d57-b882-6d4646edc86d@amperemail.onmicrosoft.com>
Date: Thu, 4 Sep 2025 13:06:09 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: Sudeep Holla <sudeep.holla@....com>, 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 v23 1/2] mailbox/pcc: support mailbox management of the
 shared buffer

Answers inline.

On 9/4/25 07:00, Sudeep Holla wrote:
> On Mon, Jul 14, 2025 at 08:10:07PM -0400, admiyo@...amperecomputing.com wrote:
>> From: Adam Young <admiyo@...amperecomputing.com>
>>
>> Define a new, optional, callback that allows the driver to
>> specify how the return data buffer is allocated.  If that callback
>> is set,  mailbox/pcc.c is now responsible for reading from and
>> writing to the PCC shared buffer.
>>
>> This also allows for proper checks of the Commnand complete flag
>> between the PCC sender and receiver.
>>
>> For Type 4 channels, initialize the command complete flag prior
>> to accepting messages.
>>
>> Since the mailbox does not know what memory allocation scheme
>> to use for response messages, the client now has an optional
>> callback that allows it to allocate the buffer for a response
>> message.
>>
>> When an outbound message is written to the buffer, the mailbox
>> checks for the flag indicating the client wants an tx complete
>> notification via IRQ.  Upon receipt of the interrupt It will
>> pair it with the outgoing message. The expected use is to
>> free the kernel memory buffer for the previous outgoing message.
>>
> I know this is merged. Based on the discussions here, I may send a revert
> to this as I don't think it is correct.
>
>> Signed-off-by: Adam Young <admiyo@...amperecomputing.com>
>> ---
>>   drivers/mailbox/pcc.c | 102 ++++++++++++++++++++++++++++++++++++++++--
>>   include/acpi/pcc.h    |  29 ++++++++++++
>>   2 files changed, 127 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index f6714c233f5a..0a00719b2482 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -306,6 +306,22 @@ 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
>> @@ -317,6 +333,8 @@ 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;
>>   
>> @@ -340,7 +358,17 @@ 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;
>> -	mbox_chan_received_data(chan, NULL);
>> +
>> +	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);
>>   
>>   	pcc_chan_acknowledge(pchan);
>>   
>> @@ -384,9 +412,24 @@ 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)
>> -		return pcc_mchan;
>> +	if (!pcc_mchan->shmem)
>> +		goto err;
>> +
>> +	pcc_mchan->manage_writes = false;
>> +
> Who will change this value as it is fixed to false always.
> That makes the whole pcc_write_to_buffer() reduntant. It must go away.
> Also why can't you use tx_prepare callback here. I don't like these changes
> at all as I find these redundant. Sorry for not reviewing it in time.
> I was totally confused with your versioning and didn't spot the mailbox/pcc
> changes in between and assumed it is just MCTP net driver changes. My mistake.

This was a case of leaving the default as is to not-break the existing 
mailbox clients.

The maibox client can over ride it in its driver setup.



>
>> +	/* 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;
>>   
>> +err:
>>   	mbox_free_channel(chan);
>>   	return ERR_PTR(-ENXIO);
>>   }
>> @@ -417,8 +460,38 @@ 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. Used
>> + * 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
>>    *		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
>> @@ -434,17 +507,37 @@ 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;
>> +
> Completely null as manages_write is false always.
Not if re-set by the client.
>
>>   	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);
> Not checking return from pcc_chan_reg_read(). Be consistent with the
> other code in the file.
OK, this is legit.
>
>> +	if (!val)
>> +		return false;
>> +	else
>> +		return true;
>> +}
>> +
>> +
>> +
>>   /**
>>    * pcc_startup - Called from Mailbox Controller code. Used here
>>    *		to request the interrupt.
>> @@ -490,6 +583,7 @@ 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 840bfc95bae3..9af3b502f839 100644
>> --- a/include/acpi/pcc.h
>> +++ b/include/acpi/pcc.h
>> @@ -17,6 +17,35 @@ 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);
> Why this can't be in rx_callback ?

Because that is too late.

The problem is that the client needs  to allocate the memory that the 
message comes in in order to hand it off.

In the case of a network device, the rx_alloc code is going to return 
the memory are of a struct sk_buff. The Mailbox does not know how to 
allocate this. If the driver just kmallocs memory for the return 
message, we would have a re-copy of the message.

This is really a mailbox-api level issue, but I was trying to limit the 
scope of my changes as much as possible.

The PCC mailbox code really does not match the abstractions of the 
mailbox in general.  The idea that copying into and out of the buffer is 
done by each individual driver leads to a lot of duplicated code.  With 
this change, most of the other drivers could now be re-written to let 
the mailbox manage the copying, while letting the mailbox client specify 
only how to allocate the message buffers.


Much of this change  was driven by the fact that the PCC mailbox does 
not properly check the flags before allowing writes to the rx channel, 
and that code is not exposed to the driver.  Thus, it was impossible to 
write everything in the rx callback regardless. This work was based on 
Huisong's comments on version 21 of the patch series.

>
>
> I am convinced to send a revert, please respond so that I can understand
> the requirements better.

Powered by blists - more mailing lists