netdev - Re: [PATCH v23 1/2] mailbox/pcc: support mailbox management of the shared buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a9c7d124-f757-4b2a-8add-aefba7b82280@amperemail.onmicrosoft.com>
Date: Fri, 5 Sep 2025 16:35:35 -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 12:57, Adam Young 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? 

My initial idea was that we should use the mssg pointer to dictate 
whether or not the mailbox should attempt the write.  If the client 
passes in a NULL pointer (and they all should, with the exception of new 
ones) then there is nothing to try to write.

But at lease one of the clients seem to set the message, and I don't 
think there is any really good reason for it.

These are  the drivers that explicitly call pcc_mbox_request_channel.  
There might be other drivers that use the mailbox and request the 
channel through the mailbox API, but lets start with these

drivers/acpi/cppc_acpi.c

drivers/hwmon/xgene-hwmon.c

drivers/i2c/busses/i2c-xgene-slimpro.c

drivers/soc/hisilicon/kunpeng_hccs.c

drivers/devfreq/hisi_uncore_freq.c

For example, the last driver calls:          rc = 
mbox_send_message(pchan->mchan, &cmd);

There is actually no reason to assume that the doorbell will be rung at 
that point:  the cmd object is not null, and thus gets added to the 
ring_buffer.  They then have to poll.  The poll may timeout, but the cmd 
pointer may still be in the ring buffer, and get sent on the next 
message.  But there is no benefit to sending the cmd object here, as the 
ring buffer pointer is not read.

slimpro_i2c_send_msg same thing.  The message is a 32bit value. None of 
these calls actually make use of  the PCC buffer: there is no PCC header 
written etc.  You could argue they don't actually meet the protocol 
definition.

Here is drivers/acpi/cppc_acpi.c

         /* Flip CMD COMPLETE bit */
         writew_relaxed(0, &generic_comm_base->status);

         pcc_ss_data->platform_owns_pcc = true;

         /* Ring doorbell */
         ret = mbox_send_message(pcc_ss_data->pcc_channel->mchan, &cmd);

If, instead, these drivers did

         ret = mbox_send_message(pcc_ss_data->pcc_channel->mchan,  NULL);

You would have proper functioning, and the void * mssg parameter could 
be used for the drivers that actually need it.

All of these drivers would have a simpler code path if they called the 
function pcc_send_data directly, without putting values on the ring 
buffer.  That was how I originally wrote my driver, but I actually want 
to make use of  the mailbox abstraction, and can use the ring buffer as 
rate limiter etc.

So, instead of going an changing the other drivers, I provided a default 
that left the existing behavior alone, and only performs the 
mailbox-assisted-write if the flag is set.