Message-ID: <20251006-large-astute-rattlesnake-c913fe@sudeepholla>
Date: Mon, 6 Oct 2025 16:48:57 +0100
From: Sudeep Holla <sudeep.holla@....com>
To: Adam Young <admiyo@...eremail.onmicrosoft.com>,
	Jassi Brar <jassisinghbrar@...il.com>
Cc: Adam Young <admiyo@...amperecomputing.com>,
	Sudeep Holla <sudeep.holla@....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 net-next v29 1/3] mailbox: add callback function for rx
 buffer allocation

On Mon, Oct 06, 2025 at 11:24:23AM -0400, Adam Young wrote:
> 
> On 10/5/25 19:34, Jassi Brar wrote:
> > On Sun, Oct 5, 2025 at 12:13 AM Adam Young
> > <admiyo@...eremail.onmicrosoft.com> wrote:
> > > Jassi, this one needs your attention specifically.
> > > 
> > > Do you have an issue with adding this callback?  I think it will add an
> > > important ability to the receive path for the mailbox API: letting the
> > > client driver specify how to allocate the memory that the message is
> > > coming in.  For general purpose mechanisms like PCC, this is essential:
> > > the mailbox cannot know all of the different formats that the drivers
> > > are going to require.  For example, the same system might have MPAM
> > > (Memory Protection) and MCTP (Network Protocol) driven by the same PCC
> > > Mailbox.
> > > 
> > Looking at the existing code, I am not even sure if rx_alloc() is needed at all.
> > 
> > Let me explain...
> > 1) write_response, via rx_alloc, is basically asking the client to
> > allocate a buffer of length parsed from the pcc header in shmem.
> Yes, that is exactly what it is doing.  Write response encapsulates the PCC
> specific logic for extracting the message length from the shared buffer. 
> Anything using an extended memory (type 3 or 4) PCC channel is going to have
> to do this logic.
> > 2) write_response is called from isr and even before the
> > mbox_chan_received_data() call.
> Yes. Specifically, it is marshalling the data from the shared buffer into
> kernel space.  This is logic that every single PCC driver needs to do.  It
> should be put in  common code.
> > 
> > Why can't you get rid of write_response() and simply call
> >      mbox_chan_received_data(chan, pchan->chan.shmem)
> > for the client to allocate and memcpy_fromio itself?
> 
> Moving write_response into the client and out of the mailbox means that it
> has to be implemented properly on every driver, leading to cut-and-paste
> errors.
> 

Agreed, but I don’t think we need to add a new API to the mailbox framework
solely to handle duplication across PCC client drivers. As I’ve mentioned a
few times already, we can introduce common helpers later if a second driver
ends up replicating this logic. That alone isn’t a good reason to add generic
APIs to the mailbox framework right now.

We can revisit this idea in the future if there’s a clear benefit, but in my
opinion, a multi-step approach is more appropriate:

1. First, add the initial driver with all the required client handling.

2. Then, if a second driver needs similar functionality, we can introduce
   shared helpers.

3. Finally, if it still makes sense at that point, we can discuss defining a
   new mailbox API - with the benefit of having concrete examples already in the
   upstream tree.

> So, yes, I can do that, but then every single driver that needs to use the
> pcc mailbox has to do the exact same code.  This is the first Type 3/4 PCC
> driver to use extended memory, and thus it needs to implement new logic. 
> That logic is to make sure  we have proper serialized access to the shared
> buffer.  It is this kind of access that the mailbox API is well designed to
> provide:  if both sides follow the protocol, it will only deliver a single
> message at a time.  If we move the logic out of the mailbox, we end up
> duplicating the serialization code in the client driver.  I could make a
> helper function for it, but we are getting to the point, then, where the
> mailbox API is not very useful.   If we are going to have an abstraction
> like this (and I think we should) then we should use it.
>

Sorry but you haven't demonstrated this with example. First try the above
mentioned step and lets talk later if you still have issues.

> We have more drivers like this coming.  There is code that is going to be
> non-PCC, but really PCC like that will need an MCTP driver.  That driver
> will also need to allocate an sk_buff for receiving the  data.  There is
> also MPAM code that will use the PCC driver and a type3  (extended memory)
> channel.  The mailbox API, in order to be more generally useful, should
> allow for swapping the memory allocation scheme between different clients of
> the same mailbox.  Then the mailbox layer is responsible for handling the
> mailboxes, and the clients are domain-specific code.
> 

You can always improve the code later. We don't know what will the discussions
lead to when you submit that driver later. We can keep those discussion for
later and just concentrate on getting the first step done here.

Hi Jassi,

If you don't have any objections, can we first get the revert in place and
let Adam attempt adding code to the client and take the discussions from
there.

-- 
Regards,
Sudeep

Powered by blists - more mailing lists