linux-kernel - Re: [PATCH v3 1/3] mctp pcc: Check before sending MCTP PCC response ACK

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] [day] [month] [year] [list]

Message-ID: <cda3dac1-3ad4-4c52-8b07-15a1f8b002fc@amperemail.onmicrosoft.com>
Date: Fri, 28 Jun 2024 14:13:20 -0400
From: Adam Young <admiyo@...eremail.onmicrosoft.com>
To: Sudeep Holla <sudeep.holla@....com>, admiyo@...amperecomputing.com,
 lihuisong@...wei.com
Cc: 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>
Subject: Re: [PATCH v3 1/3] mctp pcc: Check before sending MCTP PCC response
 ACK

  Huisong Li you wrote the original patch that I am working around.

What would I break if I disabled the IRQ ACK?  It should not be the 
default behavior as it is an optional feature.

There needs to be a mechanism for the driver to trigger the ACK, but it 
needs to be based on the content of the message buffer.

I was thinking it should be in the return code  of the callback, but 
that breaks all of the other mailbox implementations.

I suspect a better approach would be to provide a function pointer to 
the  driver and let the driver decide whether or not to trigger the ACK.

On 6/26/24 08:27, Sudeep Holla wrote:
> On Tue, Jun 25, 2024 at 02:53:31PM -0400, admiyo@...amperecomputing.com wrote:
>> From: Adam Young <admiyo@...erecomputing.com>
>>
>> Type 4 PCC channels have an option to send back a response
>> to the platform when they are done processing the request.
>> The flag to indicate whether or not to respond is inside
>> the message body, and thus is not available to the pcc
>> mailbox.  Since only one message can be processed at once per
>> channel, the value of this flag is checked during message processing
>> and passed back via the channels global structure.
>>
>> Ideally, the mailbox callback function would return a value
>> indicating whether the message requires an ACK, but that
>> would be a change to the mailbox API.  That would involve
>> some change to all (about 12) of the mailbox based drivers,
>> and the majority of them would not need to know about the
>> ACK call.
>>
> Next time when you post new series, I prefer to be cc-ed in all the patches.
> So far I ignored v1 and v2 thinking it has landed in my mbox my mistake and
> deleted them. But just checked the series on lore, sorry for that.
>
>> Signed-off-by: Adam Young <admiyo@...amperecomputing.com>
>> ---
>>   drivers/mailbox/pcc.c | 6 +++++-
>>   include/acpi/pcc.h    | 1 +
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>> index 94885e411085..5cf792700d79 100644
>> --- a/drivers/mailbox/pcc.c
>> +++ b/drivers/mailbox/pcc.c
>> @@ -280,6 +280,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>   {
>>   	struct pcc_chan_info *pchan;
>>   	struct mbox_chan *chan = p;
>> +	struct pcc_mbox_chan *pmchan;
>>   	u64 val;
>>   	int ret;
>>   
>> @@ -304,6 +305,8 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p)
>>   	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
>>   		return IRQ_NONE;
>>   
>> +	pmchan = &pchan->chan;
>> +	pmchan->ack_rx = true;  //TODO default to False
> Indeed, default must be false. You need to do this conditionally at runtime
> otherwise I see no need for this patch as it doesn't change anything as it
> stands. It needs to be fixed to get this change merged.
>
> Also we should set any such flag once at the boot, IRQ handler is not
> the right place for sure.
>