linux-kernel - RE: [PATCH] mailbox: imx: Add context save/restore for suspend/resume

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM6PR04MB496671BFF3496FD1C4C51E7E80D00@AM6PR04MB4966.eurprd04.prod.outlook.com>
Date:   Fri, 24 Apr 2020 03:05:12 +0000
From:   Aisheng Dong <aisheng.dong@....com>
To:     Anson Huang <anson.huang@....com>,
        "jassisinghbrar@...il.com" <jassisinghbrar@...il.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
CC:     dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH] mailbox: imx: Add context save/restore for suspend/resume

> From: Anson Huang <anson.huang@....com>
> Sent: Friday, April 24, 2020 10:33 AM
> 
> > Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> > suspend/resume
> >
> > > From: Anson Huang <Anson.Huang@....com>
> > > Sent: Friday, April 24, 2020 7:01 AM
> > >
> > > For "mem" mode suspend on i.MX8 SoCs, MU settings could be lost
> > > because its power is off, so save/restore is needed for MU settings
> > > during
> > suspend/resume.
> > > However, the restore can ONLY be done when MU settings are actually
> > > lost, for the scenario of settings NOT lost in "freeze" mode
> > > suspend, since there could be still IPC going on multiple CPUs,
> > > restoring the MU settings could overwrite the TIE by mistake and
> > > cause system freeze, so need to make sure ONLY restore the MU
> > > settings when it is
> > powered off.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@....com>
> >
> > As mentioned before, we'd better keep the original author.
> >
> > > ---
> > >  drivers/mailbox/imx-mailbox.c | 35
> > > +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/mailbox/imx-mailbox.c
> > > b/drivers/mailbox/imx-mailbox.c index 97bf0ac..b53cf63 100644
> > > --- a/drivers/mailbox/imx-mailbox.c
> > > +++ b/drivers/mailbox/imx-mailbox.c
> > > @@ -67,6 +67,8 @@ struct imx_mu_priv {
> > >  	struct clk		*clk;
> > >  	int			irq;
> > >
> > > +	u32 xcr;
> > > +
> > >  	bool			side_b;
> > >  };
> > >
> > > @@ -583,12 +585,45 @@ static const struct of_device_id
> > > imx_mu_dt_ids[] = {  };  MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> > >
> > > +static int imx_mu_suspend_noirq(struct device *dev) {
> > > +	struct imx_mu_priv *priv = dev_get_drvdata(dev);
> > > +
> > > +	priv->xcr = imx_mu_read(priv, priv->dcfg->xCR);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int imx_mu_resume_noirq(struct device *dev) {
> > > +	struct imx_mu_priv *priv = dev_get_drvdata(dev);
> > > +
> > > +	/*
> > > +	 * ONLY restore MU when context lost, the TIE could
> > > +	 * be set during noirq resume as there is MU data
> > > +	 * communication going on, and restore the saved
> > > +	 * value will overwrite the TIE and cause MU data
> > > +	 * send failed, may lead to system freeze. This issue
> > > +	 * is observed by testing freeze mode suspend.
> > > +	 */
> > > +	if (!imx_mu_read(priv, priv->dcfg->xCR))
> > > +		imx_mu_write(priv, priv->xcr, priv->dcfg->xCR);
> >
> > This could be separate patch if it aims to fix a specific corner case.
> 
> This is NOT corner case, it can be reproduced with our imx_5.4.y very easily, and
> this issue cause me many days to debug...Also cause Clark's effort to help test it
> a lot for many days...
> 

Is this issue only happen for non-state lost case (eg. Freeze mode)?
If yes, it's a specific case and worth a separate patch to highlight it IMHO.

BTW, it seems most drivers have this issue in current kernel because they don't know
whether the state are really lost, it seems like kernel still doesn't support this well.

> I do NOT think it makes sense to first send out your patch with bug for review,
> And then add another patch to fix it. 1 patch is enough for this feature.
> 

Anyway, if you really want to go with one patch, for this case, we usually could
keep original author and add a small fix note in commit message.
(You could see many community guys do like this if you search kernel commit log)

Basically we try our best to keep origin author in order to respect others' work
for community work.

Regards
Aisheng

>