Re: [PATCH v02] mailbox: pcc: report errors for PCC clients

[lihuisong (C)]

On 5/20/2026 9:32 PM, Sudeep Holla wrote:
> On Wed, May 20, 2026 at 07:53:45PM +0800, lihuisong (C) wrote:
> > On 5/20/2026 12:25 AM, Sudeep Holla wrote:
> > > On Tue, May 19, 2026 at 09:54:47PM +0800, lihuisong (C) wrote:
> [...]
>
> > > > @Sudeep, I have always had doubts about the addition of this line of code in
> > > > the
> > > >    commit 9c753f7c953c (mailbox: pcc: Mark Tx as complete in PCC IRQ handler).
> > > > The patch seems to avoid the timeouts in the mailbox core according to its
> > > > commit log.
> > > > Regardless of whether the command succeeds or fails, each mbox client
> > > > driver, like cppc_acpi/acpi_pcc，kunpeng_hccs and so on, is responsible to
> > > > call mbox_chan_txdone() to tell mailbox core.
> > > Few controller drivers do have mbox_chan_txdone(), so Tx complete is detected
> > Which controller driver?
> git grep mbox_chan_txdone drivers/mailbox/
Ok
> > > by PCC, so not sure why you think this is not the right place to do. The irq
> > Because many mbox client drivers call mbox_chan_txdone() after running
> > rx_callback() in mbox_chan_received_data().
> OK, but why can't the controller hide that for the clients ? What am I missing?
Now I know what you want to do.
It's ok for me to do that in controller on irq scene.
But we also need to fix the mbox_chan_txdone() code in all client drivers.
> > These drivers doesn't set chan->cl->tx_block to true.
> > It seems that the client driver having tx_block need to set
> > chan->tx_complete in tx_tick().
> > Do you add this code for them?
> I don't quite follow you.
please ship this.
> > > is to indicate the completion. I am confused as why you think otherwise.
> > > It is defined in include/linux/mailbox_controller.h for the same reason.
> > >
> > > The client drivers can you mbox_client_txdone() if they wish to as defined
> > > in include/linux/mailbox_client.h
> > mbox_client_txdone() is used in the case that txdone_method is
> > MBOX_TXDONE_BY_ACK.
> Yes and agreed.
>
> > And mbox clinte driver using IRQ method need to use mbox_chan_txdone().
> Client doesn't handle IRQ its always controller driver and client must have
> no business to do that IMO.
Ack
mbox_chan_txdone should be used by controller as this function comment 
said.
> > It seems that all the current client drivers are used in this way.
> > These interface internal would verify chan->txdone_method.
> Yes, sounds wrong to me.
>
> drivers/acpi/acpi_pcc.c
> drivers/acpi/cppc_acpi.c
> drivers/hwmon/xgene-hwmon.c
> drivers/i2c/busses/i2c-xgene-slimpro.c
> drivers/soc/hisilicon/kunpeng_hccs.c
>
> It is very clear from the code in mailbox.c, mbox_client_txdone() is for
> the client drivers and mbox_chan_txdone() is for the controller. We need
> to fix the above list but I need to check if there is anything I am missing
> to understand first. Please let me know.
Agreed.
> > In addition, I find that you also modify the txdone_irq/poll in the commit
> > 3349f800609e (mailbox: pcc: Set txdone_irq/txdone_poll based on PCCT flags).
> > The txdone_method will change from MBOX_TXDONE_BY_ACK to MBOX_TXDONE_BY_POLL
> > on the platform using poll mode.
> > This may lead to the original mbox client driver printing exceptions in
> > mbox_client_txdone.
> > I haven't observed it based on the latest code yet, it's just code analysis.
> Right, I do remember seeing something and wonder if I moved to
> mbox_chan_txdone() in drivers/acpi/acpi_pcc.c for that reason. But if the
> expectations I have mentioned are correct, then we need to fix the framework
> to avoid throwing that warnings.
Yeah, we also need to do something for your another commit
3349f800609e (mailbox: pcc: Set txdone_irq/txdone_poll based on PCCT 
flags).
>