| Message ID | 20200422051529.30757-1-zhang.lyra@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | PM: sleep: call devfreq_suspend/resume and cpufreq_suspend/resume in pairs. | expand |
On Wed, Apr 22, 2020 at 7:15 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote: > > From: Vincent Wang <vincent.wang@unisoc.com> > > If dpm_prepare() fails in dpm_suspend_start(), dpm_suspend() can't be > called. That's correct. > And then, devfreq_suspend() and cpufreq_suspend() will not be > called in the suspend flow. Right. > But in the resiume flow, devfreq_resume() and cpufreq_resume() will > be called. Right, and they are expected to cope with the situation. > This patch will ensure that devfreq_suspend/devfreq_resume and > cpufreq_suspend/cpufreq_resume are called in pairs. So why is it better to do this than to make devfreq_resume() meet the expectations? > Signed-off-by: Vincent Wang <vincent.wang@unisoc.com> > Signed-off-by: Samer Xie <samer.xie@unisoc.com> > Signed-off-by: Chunyan Zhang <zhang.lyra@gmail.com> > --- > drivers/base/power/main.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index fdd508a78ffd..eb3d987d43e0 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -1866,9 +1866,6 @@ int dpm_suspend(pm_message_t state) > trace_suspend_resume(TPS("dpm_suspend"), state.event, true); > might_sleep(); > > - devfreq_suspend(); > - cpufreq_suspend(); > - > mutex_lock(&dpm_list_mtx); > pm_transition = state; > async_error = 0; > @@ -1988,6 +1985,9 @@ int dpm_prepare(pm_message_t state) > trace_suspend_resume(TPS("dpm_prepare"), state.event, true); > might_sleep(); > > + devfreq_suspend(); > + cpufreq_suspend(); > + > /* > * Give a chance for the known devices to complete their probes, before > * disable probing of devices. This sync point is important at least > -- > 2.20.1 >
Hi Rafael, (Behalf Of Vincent Wang) Thanks for your comments, please see my answers below. On Wed, 22 Apr 2020 at 17:05, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Apr 22, 2020 at 7:15 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote: > > > > From: Vincent Wang <vincent.wang@unisoc.com> > > > > If dpm_prepare() fails in dpm_suspend_start(), dpm_suspend() can't be > > called. > > That's correct. > > > And then, devfreq_suspend() and cpufreq_suspend() will not be > > called in the suspend flow. > > Right. > > > But in the resiume flow, devfreq_resume() and cpufreq_resume() will > > be called. > > Right, and they are expected to cope with the situation. > > > This patch will ensure that devfreq_suspend/devfreq_resume and > > cpufreq_suspend/cpufreq_resume are called in pairs. > > So why is it better to do this than to make devfreq_resume() meet the > expectations? Yes，we found an issue with cpufreq schedutil governor on kernel4.14, and I think the issue should haven't been changed on the latest version of kernel. In the function dpm_suspend_start(), dpm_suspend() would not be exceuted if return error from device_prepare() [1]. So cpufreq_cpufreq() will not be called, then cpufreq_remove_update_util_hook() will not be called either, and so cpufreq_update_util_data will not be NULL. In the dpm resume flow, sugov_start() will be called, in which sg_cpu.update_util will be set to 0. And since cpufreq_update_util_data is not NULL, data->func will not be set and is still NULL which actually is sg_cpu.update_util. void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, void (*func)(struct update_util_data *data, u64 time, unsigned int flags)) { [...] if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu))) return; data->func = func; [...] } When cpufreq_update_util() is called by scheduler, there will be a NULL pointer issue. [1] https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/base/power/main.c#L2052 > > > Signed-off-by: Vincent Wang <vincent.wang@unisoc.com> > > Signed-off-by: Samer Xie <samer.xie@unisoc.com> > > Signed-off-by: Chunyan Zhang <zhang.lyra@gmail.com> > > --- > > drivers/base/power/main.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > > index fdd508a78ffd..eb3d987d43e0 100644 > > --- a/drivers/base/power/main.c > > +++ b/drivers/base/power/main.c > > @@ -1866,9 +1866,6 @@ int dpm_suspend(pm_message_t state) > > trace_suspend_resume(TPS("dpm_suspend"), state.event, true); > > might_sleep(); > > > > - devfreq_suspend(); > > - cpufreq_suspend(); > > - > > mutex_lock(&dpm_list_mtx); > > pm_transition = state; > > async_error = 0; > > @@ -1988,6 +1985,9 @@ int dpm_prepare(pm_message_t state) > > trace_suspend_resume(TPS("dpm_prepare"), state.event, true); > > might_sleep(); > > > > + devfreq_suspend(); > > + cpufreq_suspend(); > > + > > /* > > * Give a chance for the known devices to complete their probes, before > > * disable probing of devices. This sync point is important at least > > -- > > 2.20.1 > >
On Wed, Apr 22, 2020 at 1:19 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote: > > Hi Rafael, > > (Behalf Of Vincent Wang) > > Thanks for your comments, please see my answers below. > > On Wed, 22 Apr 2020 at 17:05, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Wed, Apr 22, 2020 at 7:15 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote: > > > > > > From: Vincent Wang <vincent.wang@unisoc.com> > > > > > > If dpm_prepare() fails in dpm_suspend_start(), dpm_suspend() can't be > > > called. > > > > That's correct. > > > > > And then, devfreq_suspend() and cpufreq_suspend() will not be > > > called in the suspend flow. > > > > Right. > > > > > But in the resiume flow, devfreq_resume() and cpufreq_resume() will > > > be called. > > > > Right, and they are expected to cope with the situation. > > > > > This patch will ensure that devfreq_suspend/devfreq_resume and > > > cpufreq_suspend/cpufreq_resume are called in pairs. > > > > So why is it better to do this than to make devfreq_resume() meet the > > expectations? > > Yes，we found an issue with cpufreq schedutil governor on kernel4.14, > and I think the issue should haven't been changed on the latest > version of kernel. > > In the function dpm_suspend_start(), dpm_suspend() would not be > exceuted if return error from device_prepare() [1]. So > cpufreq_cpufreq() will not be called, I guess you mean cpufreq_suspend(). That should be OK . > then cpufreq_remove_update_util_hook() will not be called either, and so > cpufreq_update_util_data will not be NULL. > > In the dpm resume flow, sugov_start() will be called, in which > sg_cpu.update_util will be set to 0. Which code patch does this? Surely not cpufreq_resume(), because that checks cpufreq_suspended which cannot be set if cpufreq_suspend() has not been called (because it is the only place setting cpufreq_suspended).
On Wed, 22 Apr 2020 at 22:21, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Apr 22, 2020 at 1:19 PM Chunyan Zhang <zhang.lyra@gmail.com> wrote: > > > > Hi Rafael, > > > > (Behalf Of Vincent Wang) > > > > Thanks for your comments, please see my answers below. > > > > On Wed, 22 Apr 2020 at 17:05, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Wed, Apr 22, 2020 at 7:15 AM Chunyan Zhang <zhang.lyra@gmail.com> wrote: > > > > > > > > From: Vincent Wang <vincent.wang@unisoc.com> > > > > > > > > If dpm_prepare() fails in dpm_suspend_start(), dpm_suspend() can't be > > > > called. > > > > > > That's correct. > > > > > > > And then, devfreq_suspend() and cpufreq_suspend() will not be > > > > called in the suspend flow. > > > > > > Right. > > > > > > > But in the resiume flow, devfreq_resume() and cpufreq_resume() will > > > > be called. > > > > > > Right, and they are expected to cope with the situation. > > > > > > > This patch will ensure that devfreq_suspend/devfreq_resume and > > > > cpufreq_suspend/cpufreq_resume are called in pairs. > > > > > > So why is it better to do this than to make devfreq_resume() meet the > > > expectations? > > > > Yes，we found an issue with cpufreq schedutil governor on kernel4.14, > > and I think the issue should haven't been changed on the latest > > version of kernel. > > > > In the function dpm_suspend_start(), dpm_suspend() would not be > > exceuted if return error from device_prepare() [1]. So > > cpufreq_cpufreq() will not be called, > > I guess you mean cpufreq_suspend(). > > That should be OK . > > > then cpufreq_remove_update_util_hook() will not be called either, and so > > cpufreq_update_util_data will not be NULL. > > > > In the dpm resume flow, sugov_start() will be called, in which > > sg_cpu.update_util will be set to 0. > > Which code patch does this? > > Surely not cpufreq_resume(), because that checks cpufreq_suspended which > cannot be set if cpufreq_suspend() has not been called (because it is the only > place setting cpufreq_suspended). Right, I saw that, then there's no issue indeed. I just need to backport the patch which added checking of cpufreq_suspended to cpufreq_resume. Thanks for your review!
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index fdd508a78ffd..eb3d987d43e0 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -1866,9 +1866,6 @@ int dpm_suspend(pm_message_t state) trace_suspend_resume(TPS("dpm_suspend"), state.event, true); might_sleep(); - devfreq_suspend(); - cpufreq_suspend(); - mutex_lock(&dpm_list_mtx); pm_transition = state; async_error = 0; @@ -1988,6 +1985,9 @@ int dpm_prepare(pm_message_t state) trace_suspend_resume(TPS("dpm_prepare"), state.event, true); might_sleep(); + devfreq_suspend(); + cpufreq_suspend(); + /* * Give a chance for the known devices to complete their probes, before * disable probing of devices. This sync point is important at least