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

[Adam Young]

 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@xxxxxxxxxxxxxxxxxxxxxx wrote:
> > From: Adam Young <admiyo@xxxxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >   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.