linux-kernel - RE: [PATCH 2/2] firmware: arm_scmi: mark channel free when init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM0PR04MB44817B64CB35B2B2FB50D8F7881C0@AM0PR04MB4481.eurprd04.prod.outlook.com>
Date:   Fri, 7 Feb 2020 02:16:04 +0000
From:   Peng Fan <peng.fan@....com>
To:     Sudeep Holla <sudeep.holla@....com>
CC:     "viresh.kumar@...aro.org" <viresh.kumar@...aro.org>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        dl-linux-imx <linux-imx@....com>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/2] firmware: arm_scmi: mark channel free when init


> Subject: Re: [PATCH 2/2] firmware: arm_scmi: mark channel free when init
> 
> On Thu, Feb 06, 2020 at 08:57:26PM +0800, peng.fan@....com wrote:
> > From: Peng Fan <peng.fan@....com>
> >
> > The firmware itself might not mark channel free, so let's explicitly
> > mark it free when do initialization.
> >
> > Also move struct scmi_shared_mem to common.h
> >
> > Signed-off-by: Peng Fan <peng.fan@....com>
> > ---
> >  drivers/firmware/arm_scmi/common.h  | 19 +++++++++++++++++--
> > drivers/firmware/arm_scmi/mailbox.c |  2 ++
> >  drivers/firmware/arm_scmi/shmem.c   | 18 ------------------
> >  3 files changed, 19 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/common.h
> > b/drivers/firmware/arm_scmi/common.h
> > index fd091a4ccbff..5df262a564a4 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -211,8 +211,23 @@ extern const struct scmi_desc scmi_mailbox_desc;
> > void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr);
> > void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr,
> > int id);
> >
> > -/* shmem related declarations */
> > -struct scmi_shared_mem;
> > +/*
> > + * SCMI specification requires all parameters, message headers,
> > +return
> > + * arguments or any protocol data to be expressed in little endian
> > + * format only.
> > + */
> > +struct scmi_shared_mem {
> > +	__le32 reserved;
> > +	__le32 channel_status;
> > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR	BIT(1)
> > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE	BIT(0)
> > +	__le32 reserved1[2];
> > +	__le32 flags;
> > +#define SCMI_SHMEM_FLAG_INTR_ENABLED	BIT(0)
> > +	__le32 length;
> > +	__le32 msg_header;
> > +	u8 msg_payload[0];
> > +};
> >
> >  void shmem_tx_prepare(struct scmi_shared_mem __iomem *shmem,
> >  		      struct scmi_xfer *xfer);
> > diff --git a/drivers/firmware/arm_scmi/mailbox.c
> > b/drivers/firmware/arm_scmi/mailbox.c
> > index 68ed58e2a47a..2d34bf6e94e2 100644
> > --- a/drivers/firmware/arm_scmi/mailbox.c
> > +++ b/drivers/firmware/arm_scmi/mailbox.c
> > @@ -104,6 +104,8 @@ static int mailbox_chan_setup(struct
> scmi_chan_info *cinfo, struct device *dev,
> >  	cinfo->transport_info = smbox;
> >  	smbox->cinfo = cinfo;
> >
> > +	iowrite32(BIT(0), &smbox->shmem->channel_status);
> > +
> 

+arm list

> If we need this then we may need to put this as a function in shmem.c I am
> still not convinced if we can do this unconditionally, i.e. will that affect Rx
> channel if there's notification pending before we initialise. But we can deal
> with that later.

Per understanding, channel is specific to an agent, it could not be shared.
So the shmem binded to the channel will not be used by others.

Since this is the initialization process, the firmware might not init the shmem.

The shmem.c shmem_tx_prepare will spin until channel free, so I did the patch.
Otherwise it might spin forever.

I'll add a check as following
if (tx)
 iowrite32(BIT(0), &smbox->shmem->channel_status);

I not find a good place to put this in shmem.c (:

> 
> Also what about error fields ? I would rather clear it to 0, not just BIT(0)

Tx channel error should also be cleared, fix in v2.

Thanks,
Peng

> 
> --
> Regards,
> Sudeep