* [PATCH] ACPICA: Fix dispatcher timeout mechanism
@ 2018-10-15 22:18 Bart Van Assche
2018-10-15 22:44 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Bart Van Assche @ 2018-10-15 22:18 UTC (permalink / raw)
To: Rafael J . Wysocki
Cc: Bart Van Assche, Len Brown, Fengguang Wu, linux-acpi, stable
This patch avoids that the following warning is reported during hibernation:
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;
/* 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;
}
--
2.19.1.331.ge82ca0e54c-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ACPICA: Fix dispatcher timeout mechanism
2018-10-15 22:18 [PATCH] ACPICA: Fix dispatcher timeout mechanism Bart Van Assche
@ 2018-10-15 22:44 ` Rafael J. Wysocki
2018-10-15 23:01 ` Bart Van Assche
0 siblings, 1 reply; 5+ 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] 5+ messages in thread
* Re: [PATCH] ACPICA: Fix dispatcher timeout mechanism
2018-10-15 22:44 ` Rafael J. Wysocki
@ 2018-10-15 23:01 ` Bart Van Assche
2018-10-16 5:04 ` Greg KH
0 siblings, 1 reply; 5+ 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] 5+ 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; 5+ 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] 5+ 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; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2018-10-16 15:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-15 22:18 [PATCH] ACPICA: Fix dispatcher timeout mechanism Bart Van Assche
2018-10-15 22:44 ` 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.