Message-ID: <b7808a42-aa11-45d5-8c8b-b8ec4fd81b1f@amperemail.onmicrosoft.com>
Date: Fri, 5 Sep 2025 12:57:56 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: Sudeep Holla <sudeep.holla@....com>
Cc: admiyo@...amperecomputing.com, 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



On 9/5/25 10:37, Sudeep Holla wrote:
> On Thu, Sep 04, 2025 at 01:06:09PM -0400, Adam Young wrote:
>> Answers inline.
>>
>> On 9/4/25 07:00, Sudeep Holla wrote:
> [...]
>
>>> 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.
>>
> What if driver changes in the middle of an ongoing transaction ? That
> doesn't sound like a good idea to me.
It would not be a good idea.  This should be setup only.  Is there a 
cleaner way to pass an initialization value like this in the mailbox API?
>
> You didn't respond as why tx_prepare callback can be used to do exactly
> same thing ?

Note that write_to_buffer checks pcc_chan_reg_read(&pchan->cmd_complete, &val);  This flag comes from struct pcc_chan_info which is defined in the pcc.c, not in a header.  Thus it is not accessible outside of the mailbox driver.  This needs to be done before data is written into the buffer, or there is a chance the far side is still reading it.  Checking it before send_data leads to a race condition.

>
>>>> +	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.
>>
> I still don't understand the requirement. The PCC users has access to shmem
> and can do what they want in rx_callback, so I don't see any reason for
> this API.

I was not around for the discussion where it was decided to diverge from 
the mailbox abstraction, and provide direct access to the buffer.  I 
assume it was due to the desire not to copy into temporary-allocated 
memory.  However, the pcc mailbox driver should be handling the access 
into and out of the buffer, and not each individual driver.  Looking at 
most of them, there is very little type 4 usage where you are getting 
significant amounts of data back in the buffer, most of them are using 
rx for notification only, not transfer of data into the buffer.  Every 
single driver that will need to copy data from the shmem into a kernel 
memory buffer will have to do the exact same logic, and you can see it 
is non-trivial and worth getting right in the mailbox driver.


>
>> This is really a mailbox-api level issue, but I was trying to limit the
>> scope of my changes as much as possible.
>>
> Please explain the issue. Sorry if I have missed, pointer are enough if
> already present in some mail thread.

I don't think it is in any mail-thread: I expected the conversation to 
happen once I submitted this patch.  I did attempt to clarify my 
reasoning as much as possible in the commit message.  But the patch got 
merged with no discussion, and without a maintainer ACK.

The issue is that the mailbox driver is generic, and the actual use of 
it (MCTP-PCC in my case) is bound to other kernel subsystems, that need 
to manage memory in various ways.  The mailbox driver should handle the 
protocol (PCC) but not the memory management.

Thus, the rights solution would be to provide this callback function at 
the mailbox API level, much like the tx_prepare functions.  However, 
doing that would only change where he pcc mailbox driver finds the 
function, and not the logic.  Thus, I limited the change to only the PCC 
subsystem:  I did not want to have to sell this to the entire mailbox 
community without some prior art to point to.


>
>> 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.
>>
> Yes that's because each user have their own requirement. You can do what
> you want in rx_callback.

Each driver needs to do the same thing: reading the header from 
iomemory, checking the length, allocating a buffer, and then reading the 
whole message from iomemory and setting the flag that says the buffer 
read is complete.  This is a way to get that logic right once, and  
reuse for any PCC driver that receives data in the buffer.


>
>> 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.
>>
> Pointers please, sorry again. But I really don't like the merged code and
> looking for ways to clean it up as well as address the requirement if it
> is not available esp. if we have to revert this change.

In retrospect, this patch made two changes, one which was completely 
required (access to the flag before writing to the buffer) and one which 
is a don't-repeat-yourself implementation.

If needs be, I can work around the changes to the RX  path. But I cannot 
work around the changes  in the TX path: it will lead to a race condition.

Making the RX change makes both paths equivalent, and will lead to 
cleaner PCC clients in the future.

And I can imagine your shock in seeing this patch post-merge.  I did 
send email out to you directly, but I realize now it must have gotten 
lost in the noise.  I really wish we had this discussion prior to 
merge.  However, I hope you will consider not reverting it.  I wrote it 
to be backwards compatible with existing mailbox clients, and to only 
take the new path upon explicit enabling.  I am more than happy to 
consider better ways to enable the features.

Powered by blists - more mailing lists