linux-kernel - RE: [PATCH V3 4/8] genpd: imx: scu-pd: do not power off console if no_console

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

Message-ID: <DU0PR04MB94179E6BAA53A996DA3596F78817A@DU0PR04MB9417.eurprd04.prod.outlook.com>
Date:   Mon, 14 Aug 2023 10:26:26 +0000
From:   Peng Fan <peng.fan@....com>
To:     Ulf Hansson <ulf.hansson@...aro.org>,
        "Peng Fan (OSS)" <peng.fan@....nxp.com>
CC:     "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>,
        dl-linux-imx <linux-imx@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: RE: [PATCH V3 4/8] genpd: imx: scu-pd: do not power off console if
 no_console_suspend

> Subject: Re: [PATCH V3 4/8] genpd: imx: scu-pd: do not power off console if
> no_console_suspend
> 
> On Mon, 31 Jul 2023 at 08:43, Peng Fan (OSS) <peng.fan@....nxp.com>
> wrote:
> >
> > From: Peng Fan <peng.fan@....com>
> >
> > Do not power off console if no_console_suspend
> 
> Perhaps extend this a bit to let the reader understand this is about leaving
> the serial device's corresponding PM domain on.

ok

> 
> >
> > Signed-off-by: Peng Fan <peng.fan@....com>
> 
> A comment below - that doesn't need to stop this from being applied
> though.
> 
> > ---
> >  drivers/genpd/imx/scu-pd.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
> > index 08583a10ac62..d69da79d3130 100644
> > --- a/drivers/genpd/imx/scu-pd.c
> > +++ b/drivers/genpd/imx/scu-pd.c
> > @@ -52,6 +52,7 @@
> >   */
> >
> >  #include <dt-bindings/firmware/imx/rsrc.h>
> > +#include <linux/console.h>
> >  #include <linux/firmware/imx/sci.h>
> >  #include <linux/firmware/imx/svc/rm.h>  #include <linux/io.h> @@
> > -324,6 +325,10 @@ static int imx_sc_pd_power(struct
> generic_pm_domain *domain, bool power_on)
> >         msg.resource = pd->rsrc;
> >         msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON :
> > IMX_SC_PM_PW_MODE_LP;
> >
> > +       /* keep uart console power on for no_console_suspend */
> > +       if (imx_con_rsrc == pd->rsrc && !console_suspend_enabled &&
> > + !power_on)
> 
> As I indicated above, I don't mind this, but I also think this is a rather generic
> problem that you are trying to solve here.
> 
> In principle, I think it should be the serial driver's responsibility to check the
> console_suspend_enabled flag. Based upon that, it should inform upper
> layers (genpd) that its device may need to stay powered on during system
> suspend. Quite similar to how we deal with system wakeups. To make this
> work we could do the following instead of $subject patch.
> 
> 1. The serial driver should call device_set_wakeup_path() (the name of that
> function is a bit confusing in this regard, but let's discuss that separately) in
> its ->suspend() callback.
> 2. Set the GENPD_FLAG_ACTIVE_WAKEUP (again the name is a bit confusing
> in this regard) for the corresponding genpd provider.

Thanks for your suggestion, I will try it. For the current v3 4/8, I will keep it in
V4. For your suggested method, I will post in a separate patch after I finish
and verified.

Thanks,
Peng.

> 
> In this way, genpd will keep the PM domain powered on during system
> suspend.
> 
> > +               return -EBUSY;
> > +
> >         ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
> >         if (ret)
> >                 dev_err(&domain->dev, "failed to power %s resource %d
> > ret %d\n",
> > --
> > 2.37.1
> >
> 
> Kind regards
> Uffe