Re: [PATCH] fbcon: use default if cursor blink interval is not valid

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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...


[1] https://bugs.launchpad.net/bugs/1574814
[2] see commits: 2a17d7e80f1d, f235f664a8af, a1e533ec07d5

Signed-off-by: Scot Doyle <lkml14@xxxxxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx> [v4.2]
---
  drivers/video/console/fbcon.c | 17 +++++++++++++----
  1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index 6e92917..da61d87 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -396,13 +396,23 @@ static void fb_flashcursor(struct work_struct *work)
  	console_unlock();
  }

+static int cursor_blink_jiffies(int candidate)
+{
+	if (candidate >= msecs_to_jiffies(50) &&
+	    candidate <= msecs_to_jiffies(USHRT_MAX))
+		return candidate;
+	else
+		return HZ / 5;

You use msecs_to_jiffies() is several places, then here open code the division. Please use msecs_to_jiffies(), that is it's intended job.


+}
+
  static void cursor_timer_handler(unsigned long dev_addr)
  {
  	struct fb_info *info = (struct fb_info *) dev_addr;
  	struct fbcon_ops *ops = info->fbcon_par;

  	queue_work(system_power_efficient_wq, &info->queue);
-	mod_timer(&ops->cursor_timer, jiffies + ops->cur_blink_jiffies);
+	mod_timer(&ops->cursor_timer, jiffies +
+	    cursor_blink_jiffies(ops->cur_blink_jiffies));
  }

  static void fbcon_add_cursor_timer(struct fb_info *info)
@@ -417,7 +427,8 @@ static void fbcon_add_cursor_timer(struct fb_info *info)

  		init_timer(&ops->cursor_timer);
  		ops->cursor_timer.function = cursor_timer_handler;
-		ops->cursor_timer.expires = jiffies + ops->cur_blink_jiffies;
+		ops->cursor_timer.expires = jiffies +
+		    cursor_blink_jiffies(ops->cur_blink_jiffies);
  		ops->cursor_timer.data = (unsigned long ) info;
  		add_timer(&ops->cursor_timer);
  		ops->flags |= FBCON_FLAGS_CURSOR_TIMER;
@@ -709,7 +720,6 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info,
  	}

  	if (!err) {
-		ops->cur_blink_jiffies = HZ / 5;
  		info->fbcon_par = ops;

  		if (vc)
@@ -957,7 +967,6 @@ static const char *fbcon_startup(void)
  	ops->currcon = -1;
  	ops->graphics = 1;
  	ops->cur_rotate = -1;
-	ops->cur_blink_jiffies = HZ / 5;
  	info->fbcon_par = ops;
  	p->con_rotate = initial_rotation;
  	set_blitting_type(vc, info);


--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]