[<prev] [next>] [<thread-prev] [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.
Powered by blists - more mailing lists