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@os.amperecomputing.com wrote:

quoted

From: Adam Young <redacted>

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.

quoted

Signed-off-by: Adam Young <redacted>
---
  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.