| Message ID | 20230822054739.33552-1-nick.hu@sifive.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] riscv: suspend: Add syscore ops for suspend | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD 9f944d2e0ab3 |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 4 and now 4 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 12 this patch: 12 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 57 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Tue, Aug 22, 2023 at 11:17 AM Nick Hu <nick.hu@sifive.com> wrote: > > If a task is the one who performs the suspend flow and it also do some > floating or vector operations before the suspend, we might lose the FPU > and vector states when it goes to the non-retention system suspend state. > Add syscore ops to save and restore the FPU and vector states of the > current task to fix the issue above. This only applies to non-retentive system suspend so why do we need this before SBI system suspend is merged ? Regards, Anup > > Co-developed-by: Andy Chiu <andy.chiu@sifive.com> > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > Signed-off-by: Nick Hu <nick.hu@sifive.com> > --- > Changes in v2: > a) Add Co-developed-by and adjust the order of signed-off > b) Rephrase the commit message > > arch/riscv/kernel/suspend.c | 45 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c > index 3c89b8ec69c4..ff69ff8a1974 100644 > --- a/arch/riscv/kernel/suspend.c > +++ b/arch/riscv/kernel/suspend.c > @@ -4,9 +4,14 @@ > * Copyright (c) 2022 Ventana Micro Systems Inc. > */ > > +#include <linux/cpu_pm.h> > #include <linux/ftrace.h> > +#include <linux/thread_info.h> > +#include <linux/syscore_ops.h> > #include <asm/csr.h> > #include <asm/suspend.h> > +#include <asm/switch_to.h> > +#include <asm/vector.h> > > void suspend_save_csrs(struct suspend_context *context) > { > @@ -85,3 +90,43 @@ int cpu_suspend(unsigned long arg, > > return rc; > } > + > +static int riscv_cpu_suspend(void) > +{ > + struct task_struct *cur_task = get_current(); > + struct pt_regs *regs = task_pt_regs(cur_task); > + > + if (has_fpu()) { > + if (unlikely(regs->status & SR_SD)) > + fstate_save(cur_task, regs); > + } > + if (has_vector()) { > + if (unlikely(regs->status & SR_SD)) > + riscv_v_vstate_save(cur_task, regs); > + } > + > + return 0; > +} > + > +static void riscv_cpu_resume(void) > +{ > + struct task_struct *cur_task = get_current(); > + struct pt_regs *regs = task_pt_regs(cur_task); > + > + if (has_fpu()) > + fstate_restore(cur_task, regs); > + if (has_vector()) > + riscv_v_vstate_restore(cur_task, regs); > +} > + > +static struct syscore_ops riscv_cpu_syscore_ops = { > + .suspend = riscv_cpu_suspend, > + .resume = riscv_cpu_resume, > +}; > + > +static int __init riscv_cpu_suspend_init(void) > +{ > + register_syscore_ops(&riscv_cpu_syscore_ops); > + return 0; > +} > +arch_initcall(riscv_cpu_suspend_init); > -- > 2.34.1 >
Hi Anup On Tue, Aug 22, 2023 at 2:16 PM Anup Patel <anup@brainfault.org> wrote: > > On Tue, Aug 22, 2023 at 11:17 AM Nick Hu <nick.hu@sifive.com> wrote: > > > > If a task is the one who performs the suspend flow and it also do some > > floating or vector operations before the suspend, we might lose the FPU > > and vector states when it goes to the non-retention system suspend state. > > Add syscore ops to save and restore the FPU and vector states of the > > current task to fix the issue above. > > This only applies to non-retentive system suspend so why do we need > this before SBI system suspend is merged ? > > Regards, > Anup > > How about hibernation? Regards, Nick > > > > Co-developed-by: Andy Chiu <andy.chiu@sifive.com> > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > > Signed-off-by: Nick Hu <nick.hu@sifive.com> > > --- > > Changes in v2: > > a) Add Co-developed-by and adjust the order of signed-off > > b) Rephrase the commit message > > > > arch/riscv/kernel/suspend.c | 45 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 45 insertions(+) > > > > diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c > > index 3c89b8ec69c4..ff69ff8a1974 100644 > > --- a/arch/riscv/kernel/suspend.c > > +++ b/arch/riscv/kernel/suspend.c > > @@ -4,9 +4,14 @@ > > * Copyright (c) 2022 Ventana Micro Systems Inc. > > */ > > > > +#include <linux/cpu_pm.h> > > #include <linux/ftrace.h> > > +#include <linux/thread_info.h> > > +#include <linux/syscore_ops.h> > > #include <asm/csr.h> > > #include <asm/suspend.h> > > +#include <asm/switch_to.h> > > +#include <asm/vector.h> > > > > void suspend_save_csrs(struct suspend_context *context) > > { > > @@ -85,3 +90,43 @@ int cpu_suspend(unsigned long arg, > > > > return rc; > > } > > + > > +static int riscv_cpu_suspend(void) > > +{ > > + struct task_struct *cur_task = get_current(); > > + struct pt_regs *regs = task_pt_regs(cur_task); > > + > > + if (has_fpu()) { > > + if (unlikely(regs->status & SR_SD)) > > + fstate_save(cur_task, regs); > > + } > > + if (has_vector()) { > > + if (unlikely(regs->status & SR_SD)) > > + riscv_v_vstate_save(cur_task, regs); > > + } > > + > > + return 0; > > +} > > + > > +static void riscv_cpu_resume(void) > > +{ > > + struct task_struct *cur_task = get_current(); > > + struct pt_regs *regs = task_pt_regs(cur_task); > > + > > + if (has_fpu()) > > + fstate_restore(cur_task, regs); > > + if (has_vector()) > > + riscv_v_vstate_restore(cur_task, regs); > > +} > > + > > +static struct syscore_ops riscv_cpu_syscore_ops = { > > + .suspend = riscv_cpu_suspend, > > + .resume = riscv_cpu_resume, > > +}; > > + > > +static int __init riscv_cpu_suspend_init(void) > > +{ > > + register_syscore_ops(&riscv_cpu_syscore_ops); > > + return 0; > > +} > > +arch_initcall(riscv_cpu_suspend_init); > > -- > > 2.34.1 > >
On Tue, Aug 22, 2023 at 1:28 PM Nick Hu <nick.hu@sifive.com> wrote: > > Hi Anup > > > On Tue, Aug 22, 2023 at 2:16 PM Anup Patel <anup@brainfault.org> wrote: > > > > On Tue, Aug 22, 2023 at 11:17 AM Nick Hu <nick.hu@sifive.com> wrote: > > > > > > If a task is the one who performs the suspend flow and it also do some > > > floating or vector operations before the suspend, we might lose the FPU > > > and vector states when it goes to the non-retention system suspend state. > > > Add syscore ops to save and restore the FPU and vector states of the > > > current task to fix the issue above. > > > > This only applies to non-retentive system suspend so why do we need > > this before SBI system suspend is merged ? > > > > Regards, > > Anup > > > > > How about hibernation? If this is for hibernation then the commit description should say so. Adding syscore_ops would mean these callbacks will be called for both suspend-to-ram and hibernate cases. Other architectures don't have this save/restore for suspend-to-ram because it is generally called from idle task. Why not do the save/restore in save_processor_state() and restore_processor_state() just like arch/mips/power/cpu.c ? Regards, Anup > > Regards, > Nick > > > > > > Co-developed-by: Andy Chiu <andy.chiu@sifive.com> > > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > > > Signed-off-by: Nick Hu <nick.hu@sifive.com> > > > --- > > > Changes in v2: > > > a) Add Co-developed-by and adjust the order of signed-off > > > b) Rephrase the commit message > > > > > > arch/riscv/kernel/suspend.c | 45 +++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 45 insertions(+) > > > > > > diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c > > > index 3c89b8ec69c4..ff69ff8a1974 100644 > > > --- a/arch/riscv/kernel/suspend.c > > > +++ b/arch/riscv/kernel/suspend.c > > > @@ -4,9 +4,14 @@ > > > * Copyright (c) 2022 Ventana Micro Systems Inc. > > > */ > > > > > > +#include <linux/cpu_pm.h> > > > #include <linux/ftrace.h> > > > +#include <linux/thread_info.h> > > > +#include <linux/syscore_ops.h> > > > #include <asm/csr.h> > > > #include <asm/suspend.h> > > > +#include <asm/switch_to.h> > > > +#include <asm/vector.h> > > > > > > void suspend_save_csrs(struct suspend_context *context) > > > { > > > @@ -85,3 +90,43 @@ int cpu_suspend(unsigned long arg, > > > > > > return rc; > > > } > > > + > > > +static int riscv_cpu_suspend(void) > > > +{ > > > + struct task_struct *cur_task = get_current(); > > > + struct pt_regs *regs = task_pt_regs(cur_task); > > > + > > > + if (has_fpu()) { > > > + if (unlikely(regs->status & SR_SD)) > > > + fstate_save(cur_task, regs); > > > + } > > > + if (has_vector()) { > > > + if (unlikely(regs->status & SR_SD)) > > > + riscv_v_vstate_save(cur_task, regs); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static void riscv_cpu_resume(void) > > > +{ > > > + struct task_struct *cur_task = get_current(); > > > + struct pt_regs *regs = task_pt_regs(cur_task); > > > + > > > + if (has_fpu()) > > > + fstate_restore(cur_task, regs); > > > + if (has_vector()) > > > + riscv_v_vstate_restore(cur_task, regs); > > > +} > > > + > > > +static struct syscore_ops riscv_cpu_syscore_ops = { > > > + .suspend = riscv_cpu_suspend, > > > + .resume = riscv_cpu_resume, > > > +}; > > > + > > > +static int __init riscv_cpu_suspend_init(void) > > > +{ > > > + register_syscore_ops(&riscv_cpu_syscore_ops); > > > + return 0; > > > +} > > > +arch_initcall(riscv_cpu_suspend_init); > > > -- > > > 2.34.1 > > >
Hi Nick, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on v6.5-rc7 next-20230822] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Nick-Hu/riscv-suspend-Add-syscore-ops-for-suspend/20230822-135024 base: linus/master patch link: https://lore.kernel.org/r/20230822054739.33552-1-nick.hu%40sifive.com patch subject: [PATCH v2] riscv: suspend: Add syscore ops for suspend config: riscv-randconfig-r121-20230822 (https://download.01.org/0day-ci/archive/20230823/202308230107.1L1ahyas-lkp@intel.com/config) compiler: riscv64-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20230823/202308230107.1L1ahyas-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308230107.1L1ahyas-lkp@intel.com/ All warnings (new ones prefixed by >>): arch/riscv/kernel/suspend.c: In function 'riscv_cpu_resume': >> arch/riscv/kernel/suspend.c:114:25: warning: unused variable 'regs' [-Wunused-variable] 114 | struct pt_regs *regs = task_pt_regs(cur_task); | ^~~~ vim +/regs +114 arch/riscv/kernel/suspend.c 110 111 static void riscv_cpu_resume(void) 112 { 113 struct task_struct *cur_task = get_current(); > 114 struct pt_regs *regs = task_pt_regs(cur_task); 115 116 if (has_fpu()) 117 fstate_restore(cur_task, regs); 118 if (has_vector()) 119 riscv_v_vstate_restore(cur_task, regs); 120 } 121
Hi Anup Thanks for your feedbacks On Tue, Aug 22, 2023 at 5:17 PM Anup Patel <anup@brainfault.org> wrote: > > On Tue, Aug 22, 2023 at 1:28 PM Nick Hu <nick.hu@sifive.com> wrote: > > > > Hi Anup > > > > > > On Tue, Aug 22, 2023 at 2:16 PM Anup Patel <anup@brainfault.org> wrote: > > > > > > On Tue, Aug 22, 2023 at 11:17 AM Nick Hu <nick.hu@sifive.com> wrote: > > > > > > > > If a task is the one who performs the suspend flow and it also do some > > > > floating or vector operations before the suspend, we might lose the FPU > > > > and vector states when it goes to the non-retention system suspend state. > > > > Add syscore ops to save and restore the FPU and vector states of the > > > > current task to fix the issue above. > > > > > > This only applies to non-retentive system suspend so why do we need > > > this before SBI system suspend is merged ? > > > > > > Regards, > > > Anup > > > > > > > > How about hibernation? > > If this is for hibernation then the commit description should say so. > Actually this commit is for the suspend-to-ram. I ask about hibernation here because I think hibernation may also have the same issue too. If hibernation doesn't need it, I can defer this patch until we have system suspend. I send this patch because I believe we will support system suspend in the kernel soon or later and the hibernation might need it now. > Adding syscore_ops would mean these callbacks will be called for > both suspend-to-ram and hibernate cases. Other architectures don't > have this save/restore for suspend-to-ram because it is generally > called from idle task. > Do you think we don't have to consider the case in the patch? Regards, Nick > Why not do the save/restore in save_processor_state() and > restore_processor_state() just like arch/mips/power/cpu.c ? > > Regards, > Anup > > > > > > Regards, > > Nick > > > > > > > > Co-developed-by: Andy Chiu <andy.chiu@sifive.com> > > > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > > > > Signed-off-by: Nick Hu <nick.hu@sifive.com> > > > > --- > > > > Changes in v2: > > > > a) Add Co-developed-by and adjust the order of signed-off > > > > b) Rephrase the commit message > > > > > > > > arch/riscv/kernel/suspend.c | 45 +++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 45 insertions(+) > > > > > > > > diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c > > > > index 3c89b8ec69c4..ff69ff8a1974 100644 > > > > --- a/arch/riscv/kernel/suspend.c > > > > +++ b/arch/riscv/kernel/suspend.c > > > > @@ -4,9 +4,14 @@ > > > > * Copyright (c) 2022 Ventana Micro Systems Inc. > > > > */ > > > > > > > > +#include <linux/cpu_pm.h> > > > > #include <linux/ftrace.h> > > > > +#include <linux/thread_info.h> > > > > +#include <linux/syscore_ops.h> > > > > #include <asm/csr.h> > > > > #include <asm/suspend.h> > > > > +#include <asm/switch_to.h> > > > > +#include <asm/vector.h> > > > > > > > > void suspend_save_csrs(struct suspend_context *context) > > > > { > > > > @@ -85,3 +90,43 @@ int cpu_suspend(unsigned long arg, > > > > > > > > return rc; > > > > } > > > > + > > > > +static int riscv_cpu_suspend(void) > > > > +{ > > > > + struct task_struct *cur_task = get_current(); > > > > + struct pt_regs *regs = task_pt_regs(cur_task); > > > > + > > > > + if (has_fpu()) { > > > > + if (unlikely(regs->status & SR_SD)) > > > > + fstate_save(cur_task, regs); > > > > + } > > > > + if (has_vector()) { > > > > + if (unlikely(regs->status & SR_SD)) > > > > + riscv_v_vstate_save(cur_task, regs); > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static void riscv_cpu_resume(void) > > > > +{ > > > > + struct task_struct *cur_task = get_current(); > > > > + struct pt_regs *regs = task_pt_regs(cur_task); > > > > + > > > > + if (has_fpu()) > > > > + fstate_restore(cur_task, regs); > > > > + if (has_vector()) > > > > + riscv_v_vstate_restore(cur_task, regs); > > > > +} > > > > + > > > > +static struct syscore_ops riscv_cpu_syscore_ops = { > > > > + .suspend = riscv_cpu_suspend, > > > > + .resume = riscv_cpu_resume, > > > > +}; > > > > + > > > > +static int __init riscv_cpu_suspend_init(void) > > > > +{ > > > > + register_syscore_ops(&riscv_cpu_syscore_ops); > > > > + return 0; > > > > +} > > > > +arch_initcall(riscv_cpu_suspend_init); > > > > -- > > > > 2.34.1 > > > >
On Wed, Aug 23, 2023 at 8:38 AM Nick Hu <nick.hu@sifive.com> wrote: > > Hi Anup > > Thanks for your feedbacks > On Tue, Aug 22, 2023 at 5:17 PM Anup Patel <anup@brainfault.org> wrote: > > > > On Tue, Aug 22, 2023 at 1:28 PM Nick Hu <nick.hu@sifive.com> wrote: > > > > > > Hi Anup > > > > > > > > > On Tue, Aug 22, 2023 at 2:16 PM Anup Patel <anup@brainfault.org> wrote: > > > > > > > > On Tue, Aug 22, 2023 at 11:17 AM Nick Hu <nick.hu@sifive.com> wrote: > > > > > > > > > > If a task is the one who performs the suspend flow and it also do some > > > > > floating or vector operations before the suspend, we might lose the FPU > > > > > and vector states when it goes to the non-retention system suspend state. > > > > > Add syscore ops to save and restore the FPU and vector states of the > > > > > current task to fix the issue above. > > > > > > > > This only applies to non-retentive system suspend so why do we need > > > > this before SBI system suspend is merged ? > > > > > > > > Regards, > > > > Anup > > > > > > > > > > > How about hibernation? > > > > If this is for hibernation then the commit description should say so. > > > Actually this commit is for the suspend-to-ram. I ask about > hibernation here because I think hibernation may also have the same > issue too. > If hibernation doesn't need it, I can defer this patch until we have > system suspend. > I send this patch because I believe we will support system suspend in > the kernel soon or later and the hibernation might need it now. Okay, understood. > > > Adding syscore_ops would mean these callbacks will be called for > > both suspend-to-ram and hibernate cases. Other architectures don't > > have this save/restore for suspend-to-ram because it is generally > > called from idle task. > > > Do you think we don't have to consider the case in the patch? I was comparing other architectures but I did not find any explicit saving of FP state for suspend-to-ram. Although, I did find explicit FP save for hibernate. Functionally, it is fine to always save FP state for suspend-to-ram but I am wondering if this redundant work in suspend-to-ram path. Regards, Anup > > Regards, > Nick > > Why not do the save/restore in save_processor_state() and > > restore_processor_state() just like arch/mips/power/cpu.c ? > > > > Regards, > > Anup > > > > > > > > > > Regards, > > > Nick > > > > > > > > > > Co-developed-by: Andy Chiu <andy.chiu@sifive.com> > > > > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > > > > > Signed-off-by: Nick Hu <nick.hu@sifive.com> > > > > > --- > > > > > Changes in v2: > > > > > a) Add Co-developed-by and adjust the order of signed-off > > > > > b) Rephrase the commit message > > > > > > > > > > arch/riscv/kernel/suspend.c | 45 +++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 45 insertions(+) > > > > > > > > > > diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c > > > > > index 3c89b8ec69c4..ff69ff8a1974 100644 > > > > > --- a/arch/riscv/kernel/suspend.c > > > > > +++ b/arch/riscv/kernel/suspend.c > > > > > @@ -4,9 +4,14 @@ > > > > > * Copyright (c) 2022 Ventana Micro Systems Inc. > > > > > */ > > > > > > > > > > +#include <linux/cpu_pm.h> > > > > > #include <linux/ftrace.h> > > > > > +#include <linux/thread_info.h> > > > > > +#include <linux/syscore_ops.h> > > > > > #include <asm/csr.h> > > > > > #include <asm/suspend.h> > > > > > +#include <asm/switch_to.h> > > > > > +#include <asm/vector.h> > > > > > > > > > > void suspend_save_csrs(struct suspend_context *context) > > > > > { > > > > > @@ -85,3 +90,43 @@ int cpu_suspend(unsigned long arg, > > > > > > > > > > return rc; > > > > > } > > > > > + > > > > > +static int riscv_cpu_suspend(void) > > > > > +{ > > > > > + struct task_struct *cur_task = get_current(); > > > > > + struct pt_regs *regs = task_pt_regs(cur_task); > > > > > + > > > > > + if (has_fpu()) { > > > > > + if (unlikely(regs->status & SR_SD)) > > > > > + fstate_save(cur_task, regs); > > > > > + } > > > > > + if (has_vector()) { > > > > > + if (unlikely(regs->status & SR_SD)) > > > > > + riscv_v_vstate_save(cur_task, regs); > > > > > + } > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static void riscv_cpu_resume(void) > > > > > +{ > > > > > + struct task_struct *cur_task = get_current(); > > > > > + struct pt_regs *regs = task_pt_regs(cur_task); > > > > > + > > > > > + if (has_fpu()) > > > > > + fstate_restore(cur_task, regs); > > > > > + if (has_vector()) > > > > > + riscv_v_vstate_restore(cur_task, regs); > > > > > +} > > > > > + > > > > > +static struct syscore_ops riscv_cpu_syscore_ops = { > > > > > + .suspend = riscv_cpu_suspend, > > > > > + .resume = riscv_cpu_resume, > > > > > +}; > > > > > + > > > > > +static int __init riscv_cpu_suspend_init(void) > > > > > +{ > > > > > + register_syscore_ops(&riscv_cpu_syscore_ops); > > > > > + return 0; > > > > > +} > > > > > +arch_initcall(riscv_cpu_suspend_init); > > > > > -- > > > > > 2.34.1 > > > > >
Hi Anup On Wed, Aug 23, 2023 at 9:44 PM Anup Patel <anup@brainfault.org> wrote: > > On Wed, Aug 23, 2023 at 8:38 AM Nick Hu <nick.hu@sifive.com> wrote: > > > > Hi Anup > > > > Thanks for your feedbacks > > On Tue, Aug 22, 2023 at 5:17 PM Anup Patel <anup@brainfault.org> wrote: > > > > > > On Tue, Aug 22, 2023 at 1:28 PM Nick Hu <nick.hu@sifive.com> wrote: > > > > > > > > Hi Anup > > > > > > > > > > > > On Tue, Aug 22, 2023 at 2:16 PM Anup Patel <anup@brainfault.org> wrote: > > > > > > > > > > On Tue, Aug 22, 2023 at 11:17 AM Nick Hu <nick.hu@sifive.com> wrote: > > > > > > > > > > > > If a task is the one who performs the suspend flow and it also do some > > > > > > floating or vector operations before the suspend, we might lose the FPU > > > > > > and vector states when it goes to the non-retention system suspend state. > > > > > > Add syscore ops to save and restore the FPU and vector states of the > > > > > > current task to fix the issue above. > > > > > > > > > > This only applies to non-retentive system suspend so why do we need > > > > > this before SBI system suspend is merged ? > > > > > > > > > > Regards, > > > > > Anup > > > > > > > > > > > > > > How about hibernation? > > > > > > If this is for hibernation then the commit description should say so. > > > > > Actually this commit is for the suspend-to-ram. I ask about > > hibernation here because I think hibernation may also have the same > > issue too. > > If hibernation doesn't need it, I can defer this patch until we have > > system suspend. > > I send this patch because I believe we will support system suspend in > > the kernel soon or later and the hibernation might need it now. > > Okay, understood. > > > > > > Adding syscore_ops would mean these callbacks will be called for > > > both suspend-to-ram and hibernate cases. Other architectures don't > > > have this save/restore for suspend-to-ram because it is generally > > > called from idle task. > > > > > Do you think we don't have to consider the case in the patch? > > I was comparing other architectures but I did not find any explicit > saving of FP state for suspend-to-ram. Although, I did find explicit > FP save for hibernate. > > Functionally, it is fine to always save FP state for suspend-to-ram > but I am wondering if this redundant work in suspend-to-ram path. > Perhaps you are right. If no one would do the suspend-to-ram/hibernation during the FPU/VECTOR calculation in the same process, it may not be a practical case. Regards, Nick > Regards, > Anup > > > > > Regards, > > Nick > > > Why not do the save/restore in save_processor_state() and > > > restore_processor_state() just like arch/mips/power/cpu.c ? > > > > > > Regards, > > > Anup > > > > > > > > > > > > > > Regards, > > > > Nick > > > > > > > > > > > > Co-developed-by: Andy Chiu <andy.chiu@sifive.com> > > > > > > Signed-off-by: Andy Chiu <andy.chiu@sifive.com> > > > > > > Signed-off-by: Nick Hu <nick.hu@sifive.com> > > > > > > --- > > > > > > Changes in v2: > > > > > > a) Add Co-developed-by and adjust the order of signed-off > > > > > > b) Rephrase the commit message > > > > > > > > > > > > arch/riscv/kernel/suspend.c | 45 +++++++++++++++++++++++++++++++++++++ > > > > > > 1 file changed, 45 insertions(+) > > > > > > > > > > > > diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c > > > > > > index 3c89b8ec69c4..ff69ff8a1974 100644 > > > > > > --- a/arch/riscv/kernel/suspend.c > > > > > > +++ b/arch/riscv/kernel/suspend.c > > > > > > @@ -4,9 +4,14 @@ > > > > > > * Copyright (c) 2022 Ventana Micro Systems Inc. > > > > > > */ > > > > > > > > > > > > +#include <linux/cpu_pm.h> > > > > > > #include <linux/ftrace.h> > > > > > > +#include <linux/thread_info.h> > > > > > > +#include <linux/syscore_ops.h> > > > > > > #include <asm/csr.h> > > > > > > #include <asm/suspend.h> > > > > > > +#include <asm/switch_to.h> > > > > > > +#include <asm/vector.h> > > > > > > > > > > > > void suspend_save_csrs(struct suspend_context *context) > > > > > > { > > > > > > @@ -85,3 +90,43 @@ int cpu_suspend(unsigned long arg, > > > > > > > > > > > > return rc; > > > > > > } > > > > > > + > > > > > > +static int riscv_cpu_suspend(void) > > > > > > +{ > > > > > > + struct task_struct *cur_task = get_current(); > > > > > > + struct pt_regs *regs = task_pt_regs(cur_task); > > > > > > + > > > > > > + if (has_fpu()) { > > > > > > + if (unlikely(regs->status & SR_SD)) > > > > > > + fstate_save(cur_task, regs); > > > > > > + } > > > > > > + if (has_vector()) { > > > > > > + if (unlikely(regs->status & SR_SD)) > > > > > > + riscv_v_vstate_save(cur_task, regs); > > > > > > + } > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +static void riscv_cpu_resume(void) > > > > > > +{ > > > > > > + struct task_struct *cur_task = get_current(); > > > > > > + struct pt_regs *regs = task_pt_regs(cur_task); > > > > > > + > > > > > > + if (has_fpu()) > > > > > > + fstate_restore(cur_task, regs); > > > > > > + if (has_vector()) > > > > > > + riscv_v_vstate_restore(cur_task, regs); > > > > > > +} > > > > > > + > > > > > > +static struct syscore_ops riscv_cpu_syscore_ops = { > > > > > > + .suspend = riscv_cpu_suspend, > > > > > > + .resume = riscv_cpu_resume, > > > > > > +}; > > > > > > + > > > > > > +static int __init riscv_cpu_suspend_init(void) > > > > > > +{ > > > > > > + register_syscore_ops(&riscv_cpu_syscore_ops); > > > > > > + return 0; > > > > > > +} > > > > > > +arch_initcall(riscv_cpu_suspend_init); > > > > > > -- > > > > > > 2.34.1 > > > > > >
diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c index 3c89b8ec69c4..ff69ff8a1974 100644 --- a/arch/riscv/kernel/suspend.c +++ b/arch/riscv/kernel/suspend.c @@ -4,9 +4,14 @@ * Copyright (c) 2022 Ventana Micro Systems Inc. */ +#include <linux/cpu_pm.h> #include <linux/ftrace.h> +#include <linux/thread_info.h> +#include <linux/syscore_ops.h> #include <asm/csr.h> #include <asm/suspend.h> +#include <asm/switch_to.h> +#include <asm/vector.h> void suspend_save_csrs(struct suspend_context *context) { @@ -85,3 +90,43 @@ int cpu_suspend(unsigned long arg, return rc; } + +static int riscv_cpu_suspend(void) +{ + struct task_struct *cur_task = get_current(); + struct pt_regs *regs = task_pt_regs(cur_task); + + if (has_fpu()) { + if (unlikely(regs->status & SR_SD)) + fstate_save(cur_task, regs); + } + if (has_vector()) { + if (unlikely(regs->status & SR_SD)) + riscv_v_vstate_save(cur_task, regs); + } + + return 0; +} + +static void riscv_cpu_resume(void) +{ + struct task_struct *cur_task = get_current(); + struct pt_regs *regs = task_pt_regs(cur_task); + + if (has_fpu()) + fstate_restore(cur_task, regs); + if (has_vector()) + riscv_v_vstate_restore(cur_task, regs); +} + +static struct syscore_ops riscv_cpu_syscore_ops = { + .suspend = riscv_cpu_suspend, + .resume = riscv_cpu_resume, +}; + +static int __init riscv_cpu_suspend_init(void) +{ + register_syscore_ops(&riscv_cpu_syscore_ops); + return 0; +} +arch_initcall(riscv_cpu_suspend_init);