Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210129082726.19406-1-ruifeng.zhang0110@gmail.com>
 <YBPNAoitmgnTxiqQ@kroah.com> <CAG7+-3NGUgryomKB=_W=p9nADPJVnns_x9oWpAFCEei7RiyrOA@mail.gmail.com>
In-Reply-To: <CAG7+-3NGUgryomKB=_W=p9nADPJVnns_x9oWpAFCEei7RiyrOA@mail.gmail.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 4 Feb 2021 14:38:32 +0100
Message-ID: <CAJZ5v0gh1y14mipnwJKE+-7FKzasMMTTzZY_dTpuepHGjyK-xg@mail.gmail.com>
Subject: Re: [PATCH] RFC syscore: add suspend type to syscore
To:     Ruifeng Zhang <ruifeng.zhang0110@gmail.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>, ruifeng.zhang1@unisoc.com,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Chunyan Zhang <chunyan.zhang@unisoc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Thu, Feb 4, 2021 at 10:07 AM Ruifeng Zhang
<ruifeng.zhang0110@gmail.com> wrote:
>
> Greg KH <gregkh@linuxfoundation.org> 于2021年1月29日周五 下午4:53写道：
> >
> > On Fri, Jan 29, 2021 at 04:27:26PM +0800, Ruifeng Zhang wrote:
> > > From: Ruifeng Zhang <ruifeng.zhang1@unisoc.com>
> > >
> > > Suspend type contains s2ram and s2idle, but syscore is only
> > > available for S2RAM.
> >
> > Who else needs this?
> In the s2idle suspend and resume, some vendors want to do some
> things, for example the vendor implemented the watchdog driver.

Do that in the platform operations then.

Adding the syscore stuff to the suspend-to-idle flow is not an option, sorry.

> The GKI requires that no modification of the kernel source is allowed,
> so an syscore_s2idle is added for use.

The GKI rules are not a mainline kernel concern.

> The reason device_suspend was not chosen was that I wanted it to
> monitor for longer periods, such as between device_suspend and
> syscore_suspend.
> >
> > > S2idle requires a similar feature, so a new parameter
> > > "enum suspend_type" is added to distinguish it.
> >
> > Who requires this export?
> >
> > I don't see a user of this new code/api in this patch, so why would it
> > be accepted?
> >
> > Also, you are doing many different things in the same patch, please
> > break this up into a patch series where you only do one logical change
> > at a time.
> I think it's only one things in patch
> 0001-RFC-syscore-add-suspend-type-to-syscore.patch,
> add a new s2ildle type for syscore.
> >
> > thanks,
> >
> > greg k-h
>
> From 1abd09045639dafdbf713514d4f1323b572dd2ec Mon Sep 17 00:00:00 2001
> From: Ruifeng Zhang <ruifeng.zhang1@unisoc.com>
> Date: Thu, 4 Feb 2021 13:29:56 +0800
> Subject: [PATCH 2/2] RFC time: add syscore suspend ops to s2idle
>
> Some vendors need do more things when s2idle.
>
> The required GKI does not allow modification of the
> kernel source code, so provide the syscore operation
> interface.

No, this absolutely is a bad idea.

> Signed-off-by: Ruifeng Zhang <ruifeng.zhang1@unisoc.com>
> ---
>  kernel/time/tick-common.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
> index 9d3a22510bab..8c4509250456 100644
> --- a/kernel/time/tick-common.c
> +++ b/kernel/time/tick-common.c
> @@ -11,6 +11,7 @@
>  #include <linux/err.h>
>  #include <linux/hrtimer.h>
>  #include <linux/interrupt.h>
> +#include <linux/list.h>
>  #include <linux/nmi.h>
>  #include <linux/percpu.h>
>  #include <linux/profile.h>
> @@ -528,6 +529,7 @@ void tick_freeze(void)
>         trace_suspend_resume(TPS("timekeeping_freeze"),
>                      smp_processor_id(), true);
>         system_state = SYSTEM_SUSPEND;
> +       syscore_suspend(SUSPEND_S2IDLE);
>         sched_clock_suspend();
>         timekeeping_suspend();
>     } else {
> @@ -553,6 +555,7 @@ void tick_unfreeze(void)
>     if (tick_freeze_depth == num_online_cpus()) {
>         timekeeping_resume();
>         sched_clock_resume();
> +       syscore_resume(SUSPEND_S2IDLE);
>         system_state = SYSTEM_RUNNING;
>         trace_suspend_resume(TPS("timekeeping_freeze"),
>                      smp_processor_id(), false);
> --
> 2.17.1
>
> Greg KH <gregkh@linuxfoundation.org> 于2021年1月29日周五 下午4:53写道：
> >
> > On Fri, Jan 29, 2021 at 04:27:26PM +0800, Ruifeng Zhang wrote:
> > > From: Ruifeng Zhang <ruifeng.zhang1@unisoc.com>
> > >
> > > Suspend type contains s2ram and s2idle, but syscore is only
> > > available for S2RAM.
> >
> > Who else needs this?
> >
> > > S2idle requires a similar feature, so a new parameter
> > > "enum suspend_type" is added to distinguish it.
> >
> > Who requires this export?
> >
> > I don't see a user of this new code/api in this patch, so why would it
> > be accepted?
> >
> > Also, you are doing many different things in the same patch, please
> > break this up into a patch series where you only do one logical change
> > at a time.
> >
> > thanks,
> >
> > greg k-h