Message-ID: <ZYQU8n7btB17nvuc@pluto>
Date: Thu, 21 Dec 2023 10:35:30 +0000
From: Cristian Marussi <cristian.marussi@....com>
To: Xinglong Yang <xinglong.yang@...tech.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	"sudeep.holla@....com" <sudeep.holla@....com>,
	"vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] firmware: arm_scmi: Check Mailbox/SMT channel for
 consistency

On Thu, Dec 21, 2023 at 10:31:29AM +0000, Xinglong Yang wrote:
> Hi, Cristian
> 
> This patch successfully solves the bug.
> 

Hi Xinglong,

thanks for reporting and testing !

Cristian

> From: Cristian Marussi <cristian.marussi@....com> 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@...tech.com>
> > 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@...r.kernel.org # 5.15+
> > Signed-off-by: Cristian Marussi <cristian.marussi@....com>
> > ---
> >  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
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Powered by blists - more mailing lists