Two current [1] and three previous [2] systems locked during boot
because the cursor flash timer was set using an ops->cur_blink_jiffies
value of 0. Previous patches attempted to solve the problem by moving
variable initialization earlier in the setup sequence [2].
Use the normal cursor blink default interval of 200 ms if
ops->cur_blink_jiffies is not in the range specified in commit
bd63364caa8d. Since invalid values are not used, specific system
initialization timings should not cause lockups.
[1] https://bugs.launchpad.net/bugs/1574814
[2] see commits: 2a17d7e80f1d, f235f664a8af, a1e533ec07d5
Signed-off-by: Scot Doyle <redacted>
Cc: <redacted> [v4.2]
---
drivers/video/console/fbcon.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
From: Jeremy Kerr <jk@ozlabs.org> Date: 2016-05-19 07:25:56
Hi Scot,
Use the normal cursor blink default interval of 200 ms if
ops->cur_blink_jiffies is not in the range specified in commit
bd63364caa8d. Since invalid values are not used, specific system
initialization timings should not cause lockups.
This fixes an issue we're seeing with the ast driver on OpenPOWER
machines, thanks!
Acked-by: Jeremy Kerr <jk@ozlabs.org>
Cheers,
Jeremy
On Thu, May 19, 2016 at 12:21 PM, Scot Doyle [off-list ref] wrote:
Two current [1] and three previous [2] systems locked during boot
because the cursor flash timer was set using an ops->cur_blink_jiffies
value of 0. Previous patches attempted to solve the problem by moving
variable initialization earlier in the setup sequence [2].
Use the normal cursor blink default interval of 200 ms if
ops->cur_blink_jiffies is not in the range specified in commit
bd63364caa8d. Since invalid values are not used, specific system
initialization timings should not cause lockups.
[1] https://bugs.launchpad.net/bugs/1574814
[2] see commits: 2a17d7e80f1d, f235f664a8af, a1e533ec07d5
Signed-off-by: Scot Doyle <redacted>
Cc: <redacted> [v4.2]
From: Pavel Machek <hidden> Date: 2016-05-19 09:01:45
Hi!
Two current [1] and three previous [2] systems locked during boot
because the cursor flash timer was set using an ops->cur_blink_jiffies
value of 0. Previous patches attempted to solve the problem by moving
variable initialization earlier in the setup sequence [2].
Use the normal cursor blink default interval of 200 ms if
ops->cur_blink_jiffies is not in the range specified in commit
bd63364caa8d. Since invalid values are not used, specific system
initialization timings should not cause lockups.
[1] https://bugs.launchpad.net/bugs/1574814
[2] see commits: 2a17d7e80f1d, f235f664a8af, a1e533ec07d5
Two current [1] and three previous [2] systems locked during boot
because the cursor flash timer was set using an ops->cur_blink_jiffies
value of 0. Previous patches attempted to solve the problem by moving
variable initialization earlier in the setup sequence [2].
Use the normal cursor blink default interval of 200 ms if
ops->cur_blink_jiffies is not in the range specified in commit
bd63364caa8d. Since invalid values are not used, specific system
initialization timings should not cause lockups.
[1] https://bugs.launchpad.net/bugs/1574814
[2] see commits: 2a17d7e80f1d, f235f664a8af, a1e533ec07d5
@@ -788,6 +788,7 @@ __mod_timer(struct timer_list *timer, unsigned long expires,timer_stats_timer_set_start_info(timer);BUG_ON(!timer->function);+WARN_ONCE(expires=jiffies,"timer should expire in the future");base=lock_timer_base(timer,&flags);------
On Thu, May 19, 2016 at 10:22 PM, Scot Doyle [off-list ref] wrote:
On Thu, 19 May 2016, Pavel Machek wrote:
quoted
Hi!
quoted
Two current [1] and three previous [2] systems locked during boot
because the cursor flash timer was set using an ops->cur_blink_jiffies
value of 0. Previous patches attempted to solve the problem by moving
variable initialization earlier in the setup sequence [2].
Use the normal cursor blink default interval of 200 ms if
ops->cur_blink_jiffies is not in the range specified in commit
bd63364caa8d. Since invalid values are not used, specific system
initialization timings should not cause lockups.
[1] https://bugs.launchpad.net/bugs/1574814
[2] see commits: 2a17d7e80f1d, f235f664a8af, a1e533ec07d5
And actually... perhaps mod_timer should have some check for too low
timeouts..?
WARN_ON?
Pavel
Interesting idea. I applied this patch to a couple systems and
receive the same warning on both:
If 'jiffies' is passed to mod_timer() for same timer unusually OR
mod_timer() isn't called from the timer handler, it shoudln't cause
soft lockup.
In the case of fbcon, 'jiffies' is always passed to mod_timer() and
mod_timer() is called from the timer handler meantime, that is a
real lockup.
@@ -788,6 +788,7 @@ __mod_timer(struct timer_list *timer, unsigned long expires,timer_stats_timer_set_start_info(timer);BUG_ON(!timer->function);+WARN_ONCE(expires=jiffies,"timer should expire in the future");base=lock_timer_base(timer,&flags);------
From: David Daney <hidden> Date: 2016-05-19 16:13:34
On 05/18/2016 09:21 PM, Scot Doyle wrote:
Two current [1] and three previous [2] systems locked during boot
because the cursor flash timer was set using an ops->cur_blink_jiffies
value of 0. Previous patches attempted to solve the problem by moving
variable initialization earlier in the setup sequence [2].
Use the normal cursor blink default interval of 200 ms if
ops->cur_blink_jiffies is not in the range specified in commit
bd63364caa8d. Since invalid values are not used, specific system
initialization timings should not cause lockups.
This patch just papers over the problem that you yourself introduced in
commit bd63364caa8d ("vt: add cursor blink interval escape sequence").
As you know, I have a patch that fixes the problem at the source:
https://lkml.org/lkml/2016/5/17/455
I don't like the idea of silently ignoring bad values passed in from
other code (drivers/tty/vt/vt.c), and much less doing the check for bad
values each time the timer expires rather than just once, where the bad
value is first introduced.
I think it would be preferable to WARN() at the site the bad value is
introduced, so that we can easily find the real source of the problem.
Initialize cur_blink_jiffies to a sane default value, then if something
attempts to set it to a value that would cause soft lockup, WARN and
refuse to change it.
Also there is a stylistic issue...
Two current [1] and three previous [2] systems locked during boot
because the cursor flash timer was set using an ops->cur_blink_jiffies
value of 0. Previous patches attempted to solve the problem by moving
variable initialization earlier in the setup sequence [2].
Use the normal cursor blink default interval of 200 ms if
ops->cur_blink_jiffies is not in the range specified in commit
bd63364caa8d. Since invalid values are not used, specific system
initialization timings should not cause lockups.
This patch just papers over the problem that you yourself introduced in commit
bd63364caa8d ("vt: add cursor blink interval escape sequence").
As you know, I have a patch that fixes the problem at the source:
https://lkml.org/lkml/2016/5/17/455
I don't like the idea of silently ignoring bad values passed in from other
code (drivers/tty/vt/vt.c), and much less doing the check for bad values each
time the timer expires rather than just once, where the bad value is first
introduced.
I think it would be preferable to WARN() at the site the bad value is
introduced, so that we can easily find the real source of the problem.
Initialize cur_blink_jiffies to a sane default value, then if something
attempts to set it to a value that would cause soft lockup, WARN and refuse to
change it.
I agree this approach would be cleaner and am willing to give it a try
by submitting an alternative patch and ack'ing yours. Thanks for taking
the time to critique my proposal.
Two systems are locking on boot [1] because ops->cur_blink_jiffies
is set to zero from vc->vc_cur_blink_ms.
Ignore such invalid intervals and log a warning.
[1] https://bugs.launchpad.net/bugs/1574814
Suggested-by: David Daney <redacted>
Signed-off-by: Scot Doyle <redacted>
Cc: <redacted> [v4.2]
---
drivers/video/console/fbcon.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
From: David Daney <hidden> Date: 2016-05-19 22:50:20
On 05/19/2016 03:31 PM, Scot Doyle wrote:
Two systems are locking on boot [1] because ops->cur_blink_jiffies
is set to zero from vc->vc_cur_blink_ms.
Ignore such invalid intervals and log a warning.
[1] https://bugs.launchpad.net/bugs/1574814
Suggested-by: David Daney <redacted>
Signed-off-by: Scot Doyle <redacted>
Cc: <redacted> [v4.2]
This seems better. I didn't test it, but...
Acked-by: David Daney <redacted>
From: Jeremy Kerr <jk@ozlabs.org> Date: 2016-05-20 01:22:08
Hi Scot,
Two systems are locking on boot [1] because ops->cur_blink_jiffies
is set to zero from vc->vc_cur_blink_ms.
Ignore such invalid intervals and log a warning.
This prevents a lockup on AST BMC machines, but (as expected) generates
a warning against the fbcon driver, which is a significantly better
result.
Tested-by: Jeremy Kerr <jk@ozlabs.org>
[now to sort out the issue in the ast driver...]
Cheers,
Jeremy
On Fri, May 20, 2016 at 6:31 AM, Scot Doyle [off-list ref] wrote:
Two systems are locking on boot [1] because ops->cur_blink_jiffies
is set to zero from vc->vc_cur_blink_ms.
Ignore such invalid intervals and log a warning.
[1] https://bugs.launchpad.net/bugs/1574814
Suggested-by: David Daney <redacted>
Signed-off-by: Scot Doyle <redacted>
Cc: <redacted> [v4.2]
Not sure this one is needed for stable because it justs dumps
a warning, and not set a valid period to ops->cur_blink_jiffies.
So I guess other fix patch is still required for the soft lockup issue, right?
Thanks,
From: Jeremy Kerr <jk@ozlabs.org> Date: 2016-05-20 02:26:29
Hi Ming,
Not sure this one is needed for stable because it justs dumps
a warning, and not set a valid period to ops->cur_blink_jiffies.
So I guess other fix patch is still required for the soft lockup
issue, right?
The main thing is that we don't set cur_blink_jiffies to the < 50ms
value. As far as I can tell, it means we'll still get the original
default, set in fbcon_startup():
ops->cur_blink_jiffies = HZ / 5;
And so don't end up spinning on the timer expiry.
Cheers,
Jeremy
On Fri, May 20, 2016 at 10:26 AM, Jeremy Kerr [off-list ref] wrote:
Hi Ming,
quoted
Not sure this one is needed for stable because it justs dumps
a warning, and not set a valid period to ops->cur_blink_jiffies.
So I guess other fix patch is still required for the soft lockup
issue, right?
The main thing is that we don't set cur_blink_jiffies to the < 50ms
value. As far as I can tell, it means we'll still get the original
default, set in fbcon_startup():
ops->cur_blink_jiffies = HZ / 5;
And so don't end up spinning on the timer expiry.
Jeremy, your theory is correct, thanks for your clarification! And my test
just shows that this patch does fix the soft lockup too, so
Tested-by: Ming Lei [off-list ref]
Then looks there are two fix patches acked & tested:
- the patch in this thread
- another one "[PATCH] tty: vt: Fix soft lockup in fbcon cursor
blink timer."
https://lkml.org/lkml/2016/5/17/455
So which one will be pushed to linus?
Thanks,
Ming
From: Jeremy Kerr <jk@ozlabs.org> Date: 2016-05-20 05:04:43
Hi Ming,
Then looks there are two fix patches acked & tested:
- the patch in this thread
- another one "[PATCH] tty: vt: Fix soft lockup in fbcon cursor
blink timer."
https://lkml.org/lkml/2016/5/17/455
So which one will be pushed to linus?
Not that it's my call, but we may want both; the first as a safety
measure to prevent an invalid cur_blink_jiffies ever being set, and the
second one to actually fix the initialisation of vc_cur_blink_ms (and
address the warning introduced by the first).
I guess we could just go with the latter for stable...
Cheers,
Jeremy
Then looks there are two fix patches acked & tested:
- the patch in this thread
- another one "[PATCH] tty: vt: Fix soft lockup in fbcon cursor
blink timer."
https://lkml.org/lkml/2016/5/17/455
So which one will be pushed to linus?
Not that it's my call, but we may want both; the first as a safety
measure to prevent an invalid cur_blink_jiffies ever being set, and the
second one to actually fix the initialisation of vc_cur_blink_ms (and
address the warning introduced by the first).
Tomi / Greg,
I'd suggest
- applying "tty: vt: Fix soft lockup in fbcon cursor blink timer." to 4.7 and stable[4.2]
- applying "fbcon: warn on invalid cursor blink intervals" to 4.7
- ignoring "fbcon: use default if cursor blink interval is not valid"
Note: the patches don't depend on each other
I guess we could just go with the latter for stable...
Cheers,
Jeremy
Then looks there are two fix patches acked & tested:
- the patch in this thread
- another one "[PATCH] tty: vt: Fix soft lockup in fbcon cursor
blink timer."
https://lkml.org/lkml/2016/5/17/455
So which one will be pushed to linus?
Not that it's my call, but we may want both; the first as a safety
measure to prevent an invalid cur_blink_jiffies ever being set, and the
second one to actually fix the initialisation of vc_cur_blink_ms (and
address the warning introduced by the first).
Tomi / Greg,
I'd suggest
- applying "tty: vt: Fix soft lockup in fbcon cursor blink timer." to 4.7 and stable[4.2]
- applying "fbcon: warn on invalid cursor blink intervals" to 4.7
- ignoring "fbcon: use default if cursor blink interval is not valid"
Note: the patches don't depend on each other
"tty: vt: Fix soft lockup..." should be applied first in order to avoid
unnecessary reports due to the log warning in "fbcon: warn on invalid..."
quoted
I guess we could just go with the latter for stable...
Cheers,
Jeremy
From: Henrique de Moraes Holschuh <hmh@hmh.eng.br> Date: 2016-05-28 11:45:22
On Thu, 19 May 2016, Scot Doyle wrote:
Two systems are locking on boot [1] because ops->cur_blink_jiffies
is set to zero from vc->vc_cur_blink_ms.
Ignore such invalid intervals and log a warning.
[1] https://bugs.launchpad.net/bugs/1574814
Suggested-by: David Daney <redacted>
Signed-off-by: Scot Doyle <redacted>
Cc: <redacted> [v4.2]
FWIW:
Tested-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> on top of 4.4.11.
And nothing caused it to issue warnings here, so far (with the other
recommended patch applied first).
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
From: Henrique de Moraes Holschuh <hmh@hmh.eng.br> Date: 2016-05-28 11:48:54
On Fri, 20 May 2016, Scot Doyle wrote:
On Fri, 20 May 2016, Jeremy Kerr wrote:
quoted
quoted
Then looks there are two fix patches acked & tested:
- the patch in this thread
- another one "[PATCH] tty: vt: Fix soft lockup in fbcon cursor
blink timer."
https://lkml.org/lkml/2016/5/17/455
So which one will be pushed to linus?
Not that it's my call, but we may want both; the first as a safety
measure to prevent an invalid cur_blink_jiffies ever being set, and the
second one to actually fix the initialisation of vc_cur_blink_ms (and
address the warning introduced by the first).
Tomi / Greg,
I'd suggest
- applying "tty: vt: Fix soft lockup in fbcon cursor blink timer." to 4.7 and stable[4.2]
- applying "fbcon: warn on invalid cursor blink intervals" to 4.7
- ignoring "fbcon: use default if cursor blink interval is not valid"
Note: the patches don't depend on each other
I applied both recommended patches on top of 4.4.11 for testing, and they
made things a lot better here.
I suggest the second patch should be backported to stable too, might as well
fix this thing for good *and keep the door closed*.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
-----Original Message-----
From: Henrique de Moraes Holschuh [mailto:hmh@hmh.eng.br]
Sent: Saturday, May 28, 2016 4:49 AM
To: Scot Doyle <redacted>
Cc: Tomi Valkeinen <redacted>; Jean-Christophe Plagniol-
Villard [off-list ref]; Greg Kroah-Hartman
[off-list ref]; Jeremy Kerr [off-list ref]; Ming Lei
[off-list ref]; Daney, David [off-list ref];
Dann Frazier [off-list ref]; Peter Hurley
[off-list ref]; Pavel Machek [off-list ref]; Jonathan Liu
[off-list ref]; Alistair Popple [off-list ref]; Jean-Philippe
Brucker [off-list ref]; Chintakuntla, Radha
[off-list ref]; Jiri Slaby [off-list ref]; David
Airlie [off-list ref]; David Daney [off-list ref]; dri-
devel@lists.freedesktop.org; linux-fbdev@vger.kernel.org; Linux Kernel
Mailing List [off-list ref]; stable
[off-list ref]
Subject: Re: [PATCH] fbcon: warn on invalid cursor blink intervals
On Fri, 20 May 2016, Scot Doyle wrote:
quoted
On Fri, 20 May 2016, Jeremy Kerr wrote:
quoted
quoted
Then looks there are two fix patches acked & tested:
- the patch in this thread
- another one "[PATCH] tty: vt: Fix soft lockup in fbcon cursor
blink timer."
https://lkml.org/lkml/2016/5/17/455
So which one will be pushed to linus?
Not that it's my call, but we may want both; the first as a safety
measure to prevent an invalid cur_blink_jiffies ever being set, and the
second one to actually fix the initialisation of vc_cur_blink_ms (and
address the warning introduced by the first).
Tomi / Greg,
I'd suggest
- applying "tty: vt: Fix soft lockup in fbcon cursor blink timer." to 4.7 and
stable[4.2]
quoted
- applying "fbcon: warn on invalid cursor blink intervals" to 4.7
- ignoring "fbcon: use default if cursor blink interval is not valid"
Note: the patches don't depend on each other
I applied both recommended patches on top of 4.4.11 for testing, and they
made things a lot better here.
I suggest the second patch should be backported to stable too, might as well
fix this thing for good *and keep the door closed*.
Is this patch available on some tree so that I can point to ?
And hope it will make it to linux-next soon ?
quoted hunk
-- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh