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

[lihuisong (C)]

On 5/23/2026 12:52 AM, Adam Young wrote:
> I am not getting li hui song's messages, only your (Sudeep's) responses.
>
> On 5/20/26 09:32, 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/
>
>
> These are the only drivers that have a callback defined so far. IN all 
> cases, they are only doing error reporting, but no change of behavior.
>
>
>     drivers/acpi/cppc_acpi.c
>     drivers/i2c/busses/i2c-xgene-slimpro.c
>     drivers/hwmon/xgene-hwmon.c
>     drivers/soc/hisilicon/kunpeng_hccs.c
>     drivers/devfreq/hisi_uncore_freq.c
Need to cleanup for these driver without introducing other issues.
> > > > 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?
> >
> > > 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.
> >
> > > > 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.
>
> I could make this path conditional on that being set.  Something like:
>
> rc = pcc_mbox_error_check_and_clear(pchan);
>
>       if (rc  &&   chan->txdone_method & MBOX_TXDONE_BY_POLL)
> -               return IRQ_NONE;
>
> Which lets the ACK and IRQ paths continue.
Client using POLL doesn't reach this code.
> > > 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.
>
> IN the PCC case, an error in handling a packet (PCC message) is 
> returned in the error register and read during the IRQ response. That 
> error message needs to propagate to the MCTP network driver so it can 
> free up the SKB and not leak memory. We cannot free it before that 
> point as it is still in the rbuf/active_request pointer.
The direction you said should be Tx, right?
I understand that your driver needs to wait for this message to be 
processed.
If the driver does not wait for the message, driver will receive timeout 
and can instruct to release the SKB.
> > > 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.
> >
> > > 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.