linux-kernel - Re: [PATCH v2 08/13] mailbox: pcc: Refactor and simplify check_and

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: <8361a849-2b3d-a5d2-464f-da597f0e2516@huawei.com>
Date: Tue, 11 Mar 2025 20:19:32 +0800
From: "lihuisong (C)" <lihuisong@...wei.com>
To: Sudeep Holla <sudeep.holla@....com>
CC: <linux-acpi@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Jassi Brar
	<jassisinghbrar@...il.com>, Adam Young <admiyo@...amperecomputing.com>,
	Robbie King <robbiek@...ghtlabs.com>
Subject: Re: [PATCH v2 08/13] mailbox: pcc: Refactor and simplify
 check_and_ack()


在 2025/3/11 20:08, Sudeep Holla 写道:
> On Tue, Mar 11, 2025 at 07:47:39PM +0800, lihuisong (C) wrote:
>> 在 2025/3/6 0:38, Sudeep Holla 写道:
>>> The existing check_and_ack() function had unnecessary complexity. The
>>> logic could be streamlined to improve code readability and maintainability.
>>>
>>> The command update register needs to be updated in order to acknowledge
>>> the platform notification through type 4 channel. So it can be done
>>> unconditionally. Currently it is complicated just to make use of
>>> pcc_send_data() which also executes the same updation.
>>>
>>> In order to simplify, let us just ring the doorbell directly from
>>> check_and_ack() instead of calling into pcc_send_data(). While at it,
>>> rename it into pcc_chan_check_and_ack() to maintain consistency in the
>>> driver.
>> LGTM except for some trivial,
>> Acked-by: Huisong Li <lihuisong@...wei.com>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@....com>
>>> ---
>>>    drivers/mailbox/pcc.c | 37 +++++++++++++------------------------
>>>    1 file changed, 13 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>> index b3d133170aac7f8acfd1999564c69b7fe4f6d582..90d6f5e24df7e796f8c29705808eb6df2806c1f2 100644
>>> --- a/drivers/mailbox/pcc.c
>>> +++ b/drivers/mailbox/pcc.c
>>> @@ -117,8 +117,6 @@ struct pcc_chan_info {
>>>    static struct pcc_chan_info *chan_info;
>>>    static int pcc_chan_count;
>>> -static int pcc_send_data(struct mbox_chan *chan, void *data);
>>> -
>>>    /*
>>>     * PCC can be used with perf critical drivers such as CPPC
>>>     * So it makes sense to locally cache the virtual address and
>>> @@ -288,33 +286,24 @@ static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
>>>    	return 0;
>>>    }
>>> -static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
>>> +static void pcc_chan_check_and_ack(struct pcc_chan_info *pchan)
>> How about use pcc_chan_ack?
What do you think of this?
>>>    {
>>> -	struct acpi_pcct_ext_pcc_shared_memory pcc_hdr;
>>> +	struct acpi_pcct_ext_pcc_shared_memory __iomem *pcc_hdr;
>>>    	if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
>>>    		return;
>>> -	/* If the memory region has not been mapped, we cannot
>>> -	 * determine if we need to send the message, but we still
>>> -	 * need to set the cmd_update flag before returning.
>>> -	 */
>>> -	if (pchan->chan.shmem == NULL) {
>>> -		pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>>> -		return;
>>> -	}
>>> -	memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
>>> -		      sizeof(struct acpi_pcct_ext_pcc_shared_memory));
>>> +
>>> +	pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>>> +
>>> +	pcc_hdr = pchan->chan.shmem;
>> Should use the original code?
>>
>> memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
>> 		      sizeof(struct acpi_pcct_ext_pcc_shared_memory));
>>
> ioread32(&pcc_hdr->flags) just reads 4 byte flag instead of copying entire
> header for no reason. It should be same as memcpy_fromio(.., .., 4)
>
get it.
>
> .