Hi, Cristian This patch successfully solves the bug. From: Cristian Marussi <cristian.marussi@xxxxxxx> Sent: Thursday, December 21, 2023 1:21 AM > On reception of a completion interrupt the SMT memory area is accessed to > retrieve the message header at first and then, if the message sequence > number identifies a transaction which is still pending, the related > payload is fetched too. > > When an SCMI command times out the channel ownership remains with the > platform until eventually a late reply is received and, as a consequence, > any further transmission attempt remains pending, waiting for the channel > to be relinquished by the platform. > > Once that late reply is received the channel ownership is given back > to the agent and any pending request is then allowed to proceed and > overwrite the SMT area of the just delivered late reply; then the wait for > the reply to the new request starts. > > It has been observed that the spurious IRQ related to the late reply can > be wrongly associated with the freshly enqueued request: when that > happens > the SCMI stack in-flight lookup procedure is fooled by the fact that the > message header now present in the SMT area is related to the new pending > transaction, even though the real reply has still to arrive. > > This race-condition on the A2P channel can be detected by looking at the > channel status bits: a genuine reply from the platform will have set the > channel free bit before triggering the completion IRQ. > > Add a consistency check to validate such condition in the A2P ISR. > > Reported-by: Xinglong Yang <xinglong.yang@xxxxxxxxxxx> > Closes: > https://lore.k/ > ernel.org%2Fall%2FPUZPR06MB54981E6FA00D82BFDBB864FBF08DA%40PUZP > R06MB5498.apcprd06.prod.outlook.com%2F&data=05%7C02%7Cxinglong.ya > ng%40cixtech.com%7C669e9ff5e6764a77791208dc01801b8e%7C0409f77ae53 > d4d23943eccade7cb4811%7C1%7C0%7C638386896955072826%7CUnknown% > 7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW > wiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=T1DOD7KfY%2FJNJHacHtX > d5wcfde%2Fd5UDqGvyW4vuYwYU%3D&reserved=0 > Fixes: 5c8a47a5a91d ("firmware: arm_scmi: Make scmi core independent of > the transport type") > CC: stable@xxxxxxxxxxxxxxx # 5.15+ > Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx> > --- > drivers/firmware/arm_scmi/common.h | 1 + > drivers/firmware/arm_scmi/mailbox.c | 14 ++++++++++++++ > drivers/firmware/arm_scmi/shmem.c | 6 ++++++ > 3 files changed, 21 insertions(+) > > diff --git a/drivers/firmware/arm_scmi/common.h > b/drivers/firmware/arm_scmi/common.h > index 3b7c68a11fd0..0956c2443840 100644 > --- a/drivers/firmware/arm_scmi/common.h > +++ b/drivers/firmware/arm_scmi/common.h > @@ -329,6 +329,7 @@ void shmem_fetch_notification(struct > scmi_shared_mem __iomem *shmem, > void shmem_clear_channel(struct scmi_shared_mem __iomem *shmem); > bool shmem_poll_done(struct scmi_shared_mem __iomem *shmem, > struct scmi_xfer *xfer); > +bool shmem_channel_free(struct scmi_shared_mem __iomem *shmem); > > /* declarations for message passing transports */ > struct scmi_msg_payld; > diff --git a/drivers/firmware/arm_scmi/mailbox.c > b/drivers/firmware/arm_scmi/mailbox.c > index 19246ed1f01f..b8d470417e8f 100644 > --- a/drivers/firmware/arm_scmi/mailbox.c > +++ b/drivers/firmware/arm_scmi/mailbox.c > @@ -45,6 +45,20 @@ static void rx_callback(struct mbox_client *cl, void *m) > { > struct scmi_mailbox *smbox = client_to_scmi_mailbox(cl); > > + /* > + * An A2P IRQ is NOT valid when received while the platform still has > + * the ownership of the channel, because the platform at first releases > + * the SMT channel and then sends the completion interrupt. > + * > + * This addresses a possible race condition in which a spurious IRQ from > + * a previous timed-out reply which arrived late could be wrongly > + * associated with the next pending transaction. > + */ > + if (cl->knows_txdone && !shmem_channel_free(smbox->shmem)) { > + dev_warn(smbox->cinfo->dev, "Ignoring spurious A2P IRQ !\n"); > + return; > + } > + > scmi_rx_callback(smbox->cinfo, shmem_read_header(smbox->shmem), > NULL); > } > > diff --git a/drivers/firmware/arm_scmi/shmem.c > b/drivers/firmware/arm_scmi/shmem.c > index 87b4f4d35f06..517d52fb3bcb 100644 > --- a/drivers/firmware/arm_scmi/shmem.c > +++ b/drivers/firmware/arm_scmi/shmem.c > @@ -122,3 +122,9 @@ bool shmem_poll_done(struct scmi_shared_mem > __iomem *shmem, > (SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR | > SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE); > } > + > +bool shmem_channel_free(struct scmi_shared_mem __iomem *shmem) > +{ > + return (ioread32(&shmem->channel_status) & > + SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE); > +} > -- > 2.34.1 Thanks, Xinglong
