[PATCH v2] riscv: suspend: Add syscore ops for suspend

[PATCH v2] riscv: suspend: Add syscore ops for suspend

[Nick Hu]

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.

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: suspend: Add syscore ops for suspend

[Anup Patel]

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
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: suspend: Add syscore ops for suspend

[Nick Hu]

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
> >

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: suspend: Add syscore ops for suspend

[Anup Patel]

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
> > >

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: suspend: Add syscore ops for suspend

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: suspend: Add syscore ops for suspend

[Nick Hu]

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
> > > >

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: suspend: Add syscore ops for suspend

[Anup Patel]

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
> > > > >

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: suspend: Add syscore ops for suspend

[Nick Hu]

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
> > > > > >

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv