linux-kernel - Re: [PATCH v2 3/6] soc/tegra: pmc: Specify restart mode

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: <20230817084812.323742-1-bbara93@gmail.com>
Date:   Thu, 17 Aug 2023 10:48:12 +0200
From:   Benjamin Bara <bbara93@...il.com>
To:     dmitry.osipenko@...labora.com
Cc:     bbara93@...il.com, benjamin.bara@...data.com, broonie@...nel.org,
        jneanne@...libre.com, jonathanh@...dia.com, lee@...nel.org,
        lgirdwood@...il.com, linux-kernel@...r.kernel.org,
        linux-omap@...r.kernel.org, linux-tegra@...r.kernel.org,
        peng.fan@....nxp.com, rafael.j.wysocki@...el.com,
        thierry.reding@...il.com, tony@...mide.com, treding@...dia.com
Subject: Re: [PATCH v2 3/6] soc/tegra: pmc: Specify restart mode

Hi Dmitry,

thanks for your feedback!

On Tue, 15 Aug 2023 at 00:38, Dmitry Osipenko <dmitry.osipenko@...labora.com> wrote:
> On 8/9/23 22:24, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@...data.com>
> >
> > The current restart handler registration does not specify whether the
> > restart is a cold or a warm one. Now, as do_kernel_restart() knows about
> > the type, the priorization is implicitly done (cold restarts are
> > executed first) and the reboot_mode kernel parameter (which is currently
> > mostly ignored) can be respected.
> >
> > Acked-by: Thierry Reding <treding@...dia.com>
> > Signed-off-by: Benjamin Bara <benjamin.bara@...data.com>
> > ---
> > v2:
> > - improve commit message
> > ---
> >  drivers/soc/tegra/pmc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> > index 162f52456f65..4f42febb9b0f 100644
> > --- a/drivers/soc/tegra/pmc.c
> > +++ b/drivers/soc/tegra/pmc.c
> > @@ -2962,7 +2962,7 @@ static int tegra_pmc_probe(struct platform_device *pdev)
> >       }
> > 
> >       err = devm_register_sys_off_handler(&pdev->dev,
> > -                                         SYS_OFF_MODE_RESTART,
> > +                                         SYS_OFF_MODE_RESTART_WARM,
> >                                           SYS_OFF_PRIO_LOW,
> >                                           tegra_pmc_restart_handler, NULL);
> >       if (err) {
> >
>
> You have tegra-pmc restart handler that uses low priority. And then
> you're adding cold/warm handlers to tps65219 and pca9450 drivers with a
> default priorities. 

Exactly, but I guess it makes sense to also use the handler with default
priority for the pmc reboot. The reason I kept it low prio was because
there is a comment that PMC should be last resort, but the reason
applies to any other soft restart handler too. I will adapt in the next
version.

> Hence this cold/warm separation of handlers doesn't do any practical
> difference in yours case because tegra-pmc will never be used as it
> did before your changes?

The change is e.g. relevant for platforms without PMIC-based soft reset,
e.g. when the tps6586x is in use. AFAIK, there is no possibility to
actually do a soft reboot, as the hard reboot will always be executed
first. This also happens if I set the kernel parameter "reboot_mode"
(also available via SysFS) to "soft" and a soft restart handler is
registered.

> Previously you wanted to make tps6586x driver to skip the warm reboot,
> but you're not touching tps6586x in this patchset.

True, there might also be other affected patches which are currently not
in linux-next yet. Will adapt the tps6586x too and depend on the whole
series in the next version.

> There is no real problem that is solved by these patches?

I think another problem is if the user sets the "reboot_mode" to "cold",
but there is no cold handler registered (as it was the case for me with
the pca9450), a warning should indicate that. AFAIK, there is no
possibility in user-space to find out if a cold restart handler is
registered, there is just this knob which can be switched to "cold". I
can also add a SysFS entry with "supported_modes" and check if the new
"mode" is supported on a store.

My other idea was to add a flags field to the notifier_block which
indicates (in case of a reboot notifier) the supported reboot_modes of
the registered handler, but I guess other notifier_block users won't
really benefit from an additional field, therefore I decided to add a
second list instead.

Best regards
Benjamin