lkml.org 
[lkml]   [2023]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: Bug report: kernel paniced when system hibernates
    On Thu, May 25, 2023 at 11:22 AM Conor Dooley <conor@kernel.org> wrote:
    >
    > Hey Atish,
    >
    > On Thu, May 25, 2023 at 10:39:44AM -0700, Atish Patra wrote:
    > > > How about the below?
    >
    > > Instead of disabling hibernate support why not revert the patch
    > > 3335068 ("riscv: Use PUD/P4D/PGD pages for the linear mapping")
    > > which doesn't add any "measured" value at this point.
    > > However, keeping the hibernation feature on and disabling linear
    > > mapping will get more testing on hibernation.
    > > While disabling hibernation and keeping the above patch which
    > > doesn't have any value at all.
    > >
    > > We don't have a regression at this point. So either approach will work though.
    >
    > I favoured this approach so that we do not release a kernel in which
    > hibernate works for these versions of OpenSBI and then stops working in
    > the future when we shore up how communicating this is supposed to work.
    > It allows us to fix the problem "properly" in slow-time, instead of
    > racing against v6.4's release.
    >

    Fair enough.

    > I happened to be talking to Palmer and he suggested making it depend on
    > NONPORTABLE:
    > |> config NONPORTABLE
    > |> bool "Allow configurations that result in non-portable kernels"
    > |> help
    > |> RISC-V kernel binaries are compatible between all known systems
    > |> whenever possible, but there are some use cases that can only be
    > |> satisfied by configurations that result in kernel binaries that are
    > |> not portable between systems.
    > |>
    > |> Selecting N does not guarantee kernels will be portable to all known
    > |> systems. Selecting any of the options guarded by NONPORTABLE will
    > |> result in kernel binaries that are unlikely to be portable between
    > |> systems.
    > |>
    > |> If unsure, say N.
    >
    > I actually think that that makes more sense, as it may actually be fine
    > to use hibernation depending on what your SBI implementation does.
    >

    That works too.

    > > If we choose to go this route, some thoughts about the commit message.
    > > > -- >8 --
    > > > From 1d4381290a1600eff9b29b8ace6be73955d9726c Mon Sep 17 00:00:00 2001
    > > > From: Conor Dooley <conor.dooley@microchip.com>
    > > > Date: Thu, 25 May 2023 15:09:08 +0100
    > > > Subject: [PATCH] RISC-V: mark hibernation as broken
    > > >
    > > > Hibernation support depends on firmware marking its reserved
    > > > regions as not mappable by Linux. As things stand, the de-facto SBI
    > >
    > > either not mappable or no save/restore capable (as We still have not
    > > concluded which way we want to go in)
    >
    > s/mappable/accessible/? Sounds like a good catch all?
    >

    Yeah.

    > >
    > > > implementation (OpenSBI) does not do this, and other implementations may
    > > > not do so either, resulting in kernel panics during hibernation ([1],
    > > > [2]).
    > > >
    > >
    > > we should probably add more context in the commit message.
    > > How about adding something along these lines:
    > >
    > > As things stand, the latest version of de-facto SBI
    > > implementation(OpenSBI) doesn't
    > > do this any more to allow 1G huge page mappings by kernel. Other SBI
    > > implementations are probably
    > > doing the same. Until the commit 3335068 ("riscv: Use PUD/P4D/PGD
    > > pages for the linear mapping"),
    > > the first 2MB region of DRAM (where the typically firmware resides)
    > > was not mappable by kernel. However,
    > > enabling that mapping resulted in the kernel panics during hibernation
    > > ([1], [2]) as the hibernation process
    > > tries to save/restore any mapped region even though it is marked as reserved.
    >
    > SGTM, I could go with that.
    >
    > > > Disable support for hibernation until such time that an SBI
    > > > implementation independent way to communicate what regions are reserved
    > > > has been agreed upon.
    > > >
    > >
    > > Anybody who wants to test the hibernation feature must revert the
    > > above mentioned patch along with turning on
    > > the config.
    >
    > This goes away with the use of non-portable, although I would work
    > mention of the config option into the commit message.
    >

    Any testing of hibernation still needs to revert the patch until we
    have the proper fix.

    > Thanks,
    > Conor.
    >
    > > > Reported-by: Song Shuai <suagrfillet@gmail.com>
    > > > Link: https://lore.kernel.org/all/CAAYs2=gQvkhTeioMmqRDVGjdtNF_vhB+vm_1dHJxPNi75YDQ_Q@mail.gmail.com/ [1]
    > > > Reported-by: JeeHeng Sia <jeeheng.sia@starfivetech.com>
    > > > Link: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/ITXwaKfA6z8
    > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
    > > > ---
    > > > arch/riscv/Kconfig | 2 +-
    > > > 1 file changed, 1 insertion(+), 1 deletion(-)
    > > >
    > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
    > > > index 13f058490608..b2495192f35a 100644
    > > > --- a/arch/riscv/Kconfig
    > > > +++ b/arch/riscv/Kconfig
    > > > @@ -801,7 +801,7 @@ menu "Power management options"
    > > > source "kernel/power/Kconfig"
    > > >
    > > > config ARCH_HIBERNATION_POSSIBLE
    > > > - def_bool y
    > > > + def_bool n
    > > >
    > > > config ARCH_HIBERNATION_HEADER
    > > > def_bool HIBERNATION
    >


    --
    Regards,
    Atish

    \
     
     \ /
      Last update: 2023-05-25 22:39    [W:4.218 / U:0.512 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site