* Re: [PATCH] ACPICA: Fix dispatcher timeout mechanism [not found] <20181015221848.256323-1-bvanassche@acm.org> @ 2018-10-15 22:44 ` Rafael J. Wysocki 2018-10-15 23:01 ` Bart Van Assche 0 siblings, 1 reply; 4+ messages in thread From: Rafael J. Wysocki @ 2018-10-15 22:44 UTC (permalink / raw) To: bvanassche Cc: Rafael J. Wysocki, Len Brown, kbuild test robot, ACPI Devel Maling List, Stable On Tue, Oct 16, 2018 at 12:19 AM Bart Van Assche <bvanassche@acm.org> wrote: > > This patch avoids that the following warning is reported during hibernation: Well, what exactly is the problem and why is the patch the right way to address it? > WARNING: CPU: 0 PID: 612 at kernel/time/timekeeping.c:751 ktime_get+0x116/0x120 > RIP: 0010:ktime_get+0x116/0x120 > Call Trace: > acpi_os_get_timer+0xe/0x30 > acpi_ds_exec_begin_control_op+0x175/0x1de > acpi_ds_exec_begin_op+0x2c7/0x39a > acpi_ps_create_op+0x573/0x5e4 > acpi_ps_parse_loop+0x349/0x1220 > acpi_ps_parse_aml+0x25b/0x6da > acpi_ps_execute_method+0x327/0x41b > acpi_ns_evaluate+0x4e9/0x6f5 > acpi_ut_evaluate_object+0xd9/0x2f2 > acpi_rs_get_method_data+0x8f/0x114 > acpi_walk_resources+0x122/0x1b6 > acpi_pci_link_get_current.isra.2+0x157/0x280 > acpi_pci_link_set+0x32f/0x4a0 > irqrouter_resume+0x58/0x80 > syscore_resume+0x84/0x380 > hibernation_snapshot+0x20c/0x4f0 > hibernate+0x22d/0x3a6 > state_store+0x99/0xa0 > kobj_attr_store+0x37/0x50 > sysfs_kf_write+0x87/0xa0 > kernfs_fop_write+0x1a5/0x240 > __vfs_write+0xd2/0x410 > vfs_write+0x101/0x250 > ksys_write+0xab/0x120 > __x64_sys_write+0x43/0x50 > do_syscall_64+0x71/0x220 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > Reported-by: Fengguang Wu <fengguang.wu@intel.com> > Fixes: 164a08cee135 ("ACPICA: Dispatcher: Introduce timeout mechanism for infinite loop detection") > References: https://lists.01.org/pipermail/lkp/2018-April/008406.html > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Len Brown <lenb@kernel.org> > Cc: Fengguang Wu <fengguang.wu@intel.com> > Cc: linux-acpi@vger.kernel.org > Cc: stable@vger.kernel.org > --- > drivers/acpi/acpica/aclocal.h | 2 +- > drivers/acpi/acpica/dscontrol.c | 9 ++++----- > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h > index 0f28a38a43ea..7b093bcbaef5 100644 > --- a/drivers/acpi/acpica/aclocal.h > +++ b/drivers/acpi/acpica/aclocal.h > @@ -588,7 +588,7 @@ struct acpi_control_state { > union acpi_parse_object *predicate_op; > u8 *aml_predicate_start; /* Start of if/while predicate */ > u8 *package_end; /* End of if/while block */ > - u64 loop_timeout; /* While() loop timeout */ > + unsigned long loop_timeout; /* While() loop timeout */ > }; > > /* > diff --git a/drivers/acpi/acpica/dscontrol.c b/drivers/acpi/acpica/dscontrol.c > index 0da96268deb5..9dbea4549484 100644 > --- a/drivers/acpi/acpica/dscontrol.c > +++ b/drivers/acpi/acpica/dscontrol.c > @@ -84,8 +84,8 @@ acpi_ds_exec_begin_control_op(struct acpi_walk_state *walk_state, > control_state->control.package_end = > walk_state->parser_state.pkg_end; > control_state->control.opcode = op->common.aml_opcode; > - control_state->control.loop_timeout = acpi_os_get_timer() + > - (u64)(acpi_gbl_max_loop_iterations * ACPI_100NSEC_PER_SEC); > + control_state->control.loop_timeout = jiffies + > + acpi_gbl_max_loop_iterations * HZ; jiffies is Linux-specific and so it should not be used in the ACPICA code. > > /* Push the control state on this walk's control stack */ > > @@ -179,9 +179,8 @@ acpi_ds_exec_end_control_op(struct acpi_walk_state *walk_state, > * written AML when the hardware does not respond within a while > * loop and the loop does not implement a timeout. > */ > - if (ACPI_TIME_AFTER(acpi_os_get_timer(), > - control_state->control. > - loop_timeout)) { > + if (time_after(jiffies, > + control_state->control.loop_timeout)) { > status = AE_AML_LOOP_TIMEOUT; > break; > } > -- ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ACPICA: Fix dispatcher timeout mechanism 2018-10-15 22:44 ` [PATCH] ACPICA: Fix dispatcher timeout mechanism Rafael J. Wysocki @ 2018-10-15 23:01 ` Bart Van Assche 2018-10-16 5:04 ` Greg KH 0 siblings, 1 reply; 4+ messages in thread From: Bart Van Assche @ 2018-10-15 23:01 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, Len Brown, kbuild test robot, ACPI Devel Maling List, Stable On Tue, 2018-10-16 at 00:44 +0200, Rafael J. Wysocki wrote: > On Tue, Oct 16, 2018 at 12:19 AM Bart Van Assche <bvanassche@acm.org> wrote: > > > > This patch avoids that the following warning is reported during hibernation: > > Well, what exactly is the problem and why is the patch the right way > to address it? It is not safe to call ktime_get() after having called timekeeping_suspend(). > > /* > > diff --git a/drivers/acpi/acpica/dscontrol.c b/drivers/acpi/acpica/dscontrol.c > > index 0da96268deb5..9dbea4549484 100644 > > --- a/drivers/acpi/acpica/dscontrol.c > > +++ b/drivers/acpi/acpica/dscontrol.c > > @@ -84,8 +84,8 @@ acpi_ds_exec_begin_control_op(struct acpi_walk_state *walk_state, > > control_state->control.package_end = > > walk_state->parser_state.pkg_end; > > control_state->control.opcode = op->common.aml_opcode; > > - control_state->control.loop_timeout = acpi_os_get_timer() + > > - (u64)(acpi_gbl_max_loop_iterations * ACPI_100NSEC_PER_SEC); > > + control_state->control.loop_timeout = jiffies + > > + acpi_gbl_max_loop_iterations * HZ; > > jiffies is Linux-specific and so it should not be used in the ACPICA code. Really? There is plenty of other ACPI code that uses the jiffies counter directly. Why wouldn't it be allowed to use the jiffies counter in this context since there is so much other ACPI code that uses that counter? $ git grep -nHw jiffies drivers/acpi drivers/acpi/acpi_pad.c:158: if (time_before(expire_time, jiffies)) { drivers/acpi/acpi_pad.c:159: last_jiffies = jiffies; drivers/acpi/acpi_pad.c:165: expire_time = jiffies + HZ * (100 - idle_pct) / 100; drivers/acpi/acpi_pad.c:184: if (time_before(expire_time, jiffies)) { drivers/acpi/apei/ghes.c:744: expire = jiffies + msecs_to_jiffies(g->notify.poll_interval); drivers/acpi/battery.c:29:#include <linux/jiffies.h> drivers/acpi/battery.c:567: time_before(jiffies, battery->update_time + drivers/acpi/battery.c:583: battery->update_time = jiffies; drivers/acpi/dock.c:30:#include <linux/jiffies.h> drivers/acpi/dock.c:365: ds->last_dock_time = jiffies; drivers/acpi/dock.c:389: time_before(jiffies, (ds->last_dock_time + HZ))) drivers/acpi/dock.c:621: dock_station->last_dock_time = jiffies - HZ; drivers/acpi/ec.c:304: ec->timestamp = jiffies; drivers/acpi/ec.c:313: ec->timestamp = jiffies; drivers/acpi/ec.c:320: ec->timestamp = jiffies; drivers/acpi/ec.c:752: } while (time_before(jiffies, timeout)); drivers/acpi/ec.c:762: unsigned long delay = jiffies + drivers/acpi/ec.c:770: } while (time_before(jiffies, delay)); drivers/acpi/ec.c:1350: ec->timestamp = jiffies; drivers/acpi/osl.c:41:#include <linux/jiffies.h> drivers/acpi/osl.c:1230: long jiffies; drivers/acpi/osl.c:1246: jiffies = MAX_SCHEDULE_TIMEOUT; drivers/acpi/osl.c:1248: jiffies = msecs_to_jiffies(timeout); drivers/acpi/osl.c:1250: ret = down_timeout(sem, jiffies); drivers/acpi/sbs.c:31:#include <linux/jiffies.h> drivers/acpi/sbs.c:377: time_before(jiffies, battery->update_time + drivers/acpi/sbs.c:391: battery->update_time = jiffies; drivers/acpi/thermal.c:36:#include <linux/jiffies.h> Bart. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ACPICA: Fix dispatcher timeout mechanism 2018-10-15 23:01 ` Bart Van Assche @ 2018-10-16 5:04 ` Greg KH 2018-10-16 15:03 ` Bart Van Assche 0 siblings, 1 reply; 4+ messages in thread From: Greg KH @ 2018-10-16 5:04 UTC (permalink / raw) To: Bart Van Assche Cc: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown, kbuild test robot, ACPI Devel Maling List, Stable On Mon, Oct 15, 2018 at 04:01:35PM -0700, Bart Van Assche wrote: > On Tue, 2018-10-16 at 00:44 +0200, Rafael J. Wysocki wrote: > > On Tue, Oct 16, 2018 at 12:19 AM Bart Van Assche <bvanassche@acm.org> wrote: > > > > > > This patch avoids that the following warning is reported during hibernation: > > > > Well, what exactly is the problem and why is the patch the right way > > to address it? > > It is not safe to call ktime_get() after having called timekeeping_suspend(). > > > > /* > > > diff --git a/drivers/acpi/acpica/dscontrol.c b/drivers/acpi/acpica/dscontrol.c > > > index 0da96268deb5..9dbea4549484 100644 > > > --- a/drivers/acpi/acpica/dscontrol.c > > > +++ b/drivers/acpi/acpica/dscontrol.c > > > @@ -84,8 +84,8 @@ acpi_ds_exec_begin_control_op(struct acpi_walk_state *walk_state, > > > control_state->control.package_end = > > > walk_state->parser_state.pkg_end; > > > control_state->control.opcode = op->common.aml_opcode; > > > - control_state->control.loop_timeout = acpi_os_get_timer() + > > > - (u64)(acpi_gbl_max_loop_iterations * ACPI_100NSEC_PER_SEC); > > > + control_state->control.loop_timeout = jiffies + > > > + acpi_gbl_max_loop_iterations * HZ; > > > > jiffies is Linux-specific and so it should not be used in the ACPICA code. > > Really? There is plenty of other ACPI code that uses the jiffies counter directly. > Why wouldn't it be allowed to use the jiffies counter in this context since there > is so much other ACPI code that uses that counter? acpica is different from acpi. It is the "core acpi code" that is shared across other operating systems. That is why jiffies does not work in it :( thanks, greg k-h ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ACPICA: Fix dispatcher timeout mechanism 2018-10-16 5:04 ` Greg KH @ 2018-10-16 15:03 ` Bart Van Assche 0 siblings, 0 replies; 4+ messages in thread From: Bart Van Assche @ 2018-10-16 15:03 UTC (permalink / raw) To: Greg KH Cc: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown, kbuild test robot, ACPI Devel Maling List, Stable On Tue, 2018-10-16 at 07:04 +0200, Greg KH wrote: > On Mon, Oct 15, 2018 at 04:01:35PM -0700, Bart Van Assche wrote: > > On Tue, 2018-10-16 at 00:44 +0200, Rafael J. Wysocki wrote: > > > On Tue, Oct 16, 2018 at 12:19 AM Bart Van Assche <bvanassche@acm.org> wrote: > > > > > > > > This patch avoids that the following warning is reported during hibernation: > > > > > > Well, what exactly is the problem and why is the patch the right way > > > to address it? > > > > It is not safe to call ktime_get() after having called timekeeping_suspend(). > > > > > > /* > > > > diff --git a/drivers/acpi/acpica/dscontrol.c b/drivers/acpi/acpica/dscontrol.c > > > > index 0da96268deb5..9dbea4549484 100644 > > > > --- a/drivers/acpi/acpica/dscontrol.c > > > > +++ b/drivers/acpi/acpica/dscontrol.c > > > > @@ -84,8 +84,8 @@ acpi_ds_exec_begin_control_op(struct acpi_walk_state *walk_state, > > > > control_state->control.package_end = > > > > walk_state->parser_state.pkg_end; > > > > control_state->control.opcode = op->common.aml_opcode; > > > > - control_state->control.loop_timeout = acpi_os_get_timer() + > > > > - (u64)(acpi_gbl_max_loop_iterations * ACPI_100NSEC_PER_SEC); > > > > + control_state->control.loop_timeout = jiffies + > > > > + acpi_gbl_max_loop_iterations * HZ; > > > > > > jiffies is Linux-specific and so it should not be used in the ACPICA code. > > > > Really? There is plenty of other ACPI code that uses the jiffies counter directly. > > Why wouldn't it be allowed to use the jiffies counter in this context since there > > is so much other ACPI code that uses that counter? > > acpica is different from acpi. It is the "core acpi code" that is > shared across other operating systems. That is why jiffies does not > work in it :( Thanks Greg for the feedback. I wasn't aware of the difference between ACPICA and other ACPI code. Bart. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-10-16 22:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20181015221848.256323-1-bvanassche@acm.org>
2018-10-15 22:44 ` [PATCH] ACPICA: Fix dispatcher timeout mechanism Rafael J. Wysocki
2018-10-15 23:01 ` Bart Van Assche
2018-10-16 5:04 ` Greg KH
2018-10-16 15:03 ` Bart Van Assche
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox