Message-ID: <aL70PVhM-UVi5UrS@bogus>
Date: Mon, 8 Sep 2025 16:20:29 +0100
From: Sudeep Holla <sudeep.holla@....com>
To: Adam Young <admiyo@...eremail.onmicrosoft.com>
Cc: <admiyo@...amperecomputing.com>, Jassi Brar <jassisinghbrar@...il.com>,
	Sudeep Holla <sudeep.holla@....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

On Mon, Sep 08, 2025 at 10:58:55AM -0400, Adam Young wrote:
> 
> 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.
> 
> Have you decided what to do?  The MCTP over PCC driver depends on the
> behavior in this patch. If you do revert, I will need a path forward.
> 

Sorry not yet. I still need to understand and analyse your last reply.

> Based on other code review feed back, I need to make an additional change: 
> the rx_alloc callback function needs to be atomically set, and thus needs to
> move to the mailbox API.  There it will pair with the prepare transaction
> function.  It is a small change, but I expect some feedback from the mailbox
> maintainers.
> 
> I know all of the other drivers that use the PCC mailbox currently do direct
> management of the shared buffer.  I suspect that is the biggest change that
> is causing you concern.  Are you OK with maintaining a mailbox-managed path
> to buffer management as well?  I think it will be beneficial to other
> drivers in the long run.
> 

If you are really interesting to consolidating, then move all the buffer
management into the core. Just don't introduce things that just work on
your platform and for your use case. You need move all the drivers to this
new model of accessing the buffers. Otherwise I see no point as it is just
another churn but in core mailbox PCC driver instead of a client driver
using PCC. So, I am not OK with the change as is and needs to be reworked
or reverted. I need sometime to understand your email and requirements.

-- 
Regards,
Sudeep

Powered by blists - more mailing lists