From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754380AbcESQNe (ORCPT ); Thu, 19 May 2016 12:13:34 -0400 Received: from mail-bl2on0066.outbound.protection.outlook.com ([65.55.169.66]:63936 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751760AbcESQNd (ORCPT ); Thu, 19 May 2016 12:13:33 -0400 Authentication-Results: scotdoyle.com; dkim=none (message not signed) header.d=none;scotdoyle.com; dmarc=none action=none header.from=caviumnetworks.com; Message-ID: <573DE2D0.1050402@caviumnetworks.com> Date: Thu, 19 May 2016 08:59:12 -0700 From: David Daney User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Scot Doyle CC: Tomi Valkeinen , Jean-Christophe Plagniol-Villard , David Daney , Ming Lei , Dann Frazier , Peter Hurley , Pavel Machek , Jonathan Liu , Alistair Popple , Jean-Philippe Brucker , "Chintakuntla, Radha" , Greg Kroah-Hartman , Jiri Slaby , David Airlie , , , , , Subject: Re: [PATCH] fbcon: use default if cursor blink interval is not valid References: <1463510464-28124-1-git-send-email-ddaney.cavm@gmail.com> <20160517204912.GA29719@amd> In-Reply-To: Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [50.233.148.158] X-ClientProxiedBy: CO2PR07CA0026.namprd07.prod.outlook.com (10.141.194.164) To BN4PR07MB2130.namprd07.prod.outlook.com (10.164.63.12) X-MS-Office365-Filtering-Correlation-Id: 1fe06cf1-4dbe-40f9-417f-08d37ffe8a28 X-Microsoft-Exchange-Diagnostics: 1;BN4PR07MB2130;2:qpp/f75ZBQpkotpB3kj5zqbzCXmgdnkKUYdADDDocUyl2aY+ZyqXuieTInOl7Duk5746JmtG9AcCuFmA/aVLpo8jImPL/8h5CFyLPX8RDojJvIfSraOcMD2ijbeltv7sUpgM6eHawsxg0DUg7KHo/MIF1glXpEKUKq9zlMeaIr4QH2DNrLc9KkgrFr+7KcDz;3:jytjM6nG4FDbHBsyBNAFk0D3KtmPkp2UIbjNJ0/nhI8BOssGek7PbmOxxrNmBFvmfcM6jjVPW56reNoUWJHuvjRKmKWdTjsRMujH0SzutCGHYCfBVFhFQpCeDNqQptdZ;25:sFbXWLa4h58cHHF4kPPKEtQ7PzH4B+tcvtCqBU0jC3C3dOpL/VZQzUKith7xviWMMoXQ6d2h1h8vrWthkSFpQ4Ku3+4Z1YxMMOBqkR6t90uYs7ABXT6CtZvYpXSWyzvIQ9fuqLyBxUV+vMAMETubvHTf99lRCjnFKgoeoOHN05FuH7FnuFQnCHSa/GnzO+DbqRDg6IL/41z9yEzhWiFCPiwSwjvXe/lIaKod5ZTd6yetH6FbE+aQXaxBbv9c/ulg6kghz04YtByAS18DVk6wJGRWeNNr9NlyTmnDgG0ZaHHNZfOh6LvQW2YoaRBZEJJ8zYSEmNrQzgqGpR9ZPd2vBJyl7qtsmdFtI3753SGcY6bzakikQLrLBnhR4hB0osHPTeKKlAgLdBkMiCbfj2xZwPvyfQQTv2vlOs3yM3yGOH0= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BN4PR07MB2130; X-Microsoft-Exchange-Diagnostics: 1;BN4PR07MB2130;20:UUamAUHqo8G6R4SU4sSRkZwZO3Bn0qoJQEB+7cRpCW+4+NQEbZvQbhurfEEvdl5b0owK0DP4F18rYrHxT0VcyGfh4u52l69Pal5L/KzZrYEUK2Z8AcWJbYWJFMbjlDNVuhm9ZlUe1fZhBTQF7KraYVWYcEJU+NWT3I8VR+uyEycte67QCxV+LfsRLUSjUwYAJAwnqmUlNEE3C0BPcXzO0KIOK+wFm3SJe2YvNB9mobZYBkytzj7eCCj0qLOmMgSq6Z0YKAnVYemvf2eiz2cPG22IWdcUel1B5I0Og/f9oLtXFqYRp8AhbIuEv20B4Q1x3uqztVs4V5qyTTTEUpKH+OIJKFdxDBz+w4xjXJL8/dyYF+H8FwPlF4p+18kyAbwyXWfcuYuvg10TEVXQUeSstUxq/a4Hp6JD5ywlhETMAgphdvx4fLVqUmJUQog7g6oNpSJ/0S12uoHuWLl1OMx5mxLAdrkWbFt1bCxck90CteakCXMAAkqxRtkvI1hzArK2MWG+336122xuIXZEiu/ca99MGMEL+3dtE2Ielb7WTkCOAQheIH+y5CAsJEq2loXJ00KxKO3OMxfd0XljKPXCJ9iZ4FtNhdAktJbh80UbLw4= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046);SRVR:BN4PR07MB2130;BCL:0;PCL:0;RULEID:;SRVR:BN4PR07MB2130; X-Microsoft-Exchange-Diagnostics: 1;BN4PR07MB2130;4:jFDMUwdriX0BfCId4NscHKu754INVHHwJWLwh5Uj957/h5Av2HhvNKSgbRPKRuGKshSAw0fZO1XkDJX8h0keYujhX/w3ok83YB1B2GVcf2XjGELjQejOxBVjpSWMFKrXqDlnB/G5RMYc2KTKqKXXj9f6FAm7bKqMVdQ4n/1TpuH2FA2DknNSXNu7YjIPGDsnZPiLFdDYvEB8TBbbpH13BYUqDmvVLRAD/hR4WNBlswMrwTgyYgM9gA68rZcDHIszMQsrmMtxPcCUZNrTvbfeNPzxQNts5VfTbItIVdDHmguMmYRMTjPE6ltwQfr//tTIsus+z471iWWJvWnhkAAYV7VhI598V64Fr6/02ZPU/77f1a71+SVR/yixBcDyqsZz X-Forefront-PRVS: 094700CA91 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4630300001)(6009001)(24454002)(288314003)(377454003)(53416004)(8676002)(54356999)(87266999)(64126003)(19580395003)(76176999)(19580405001)(23756003)(4001350100001)(65806001)(66066001)(65956001)(80316001)(189998001)(2906002)(15975445007)(50466002)(93886004)(230700001)(33656002)(83506001)(81166006)(50986999)(77096005)(65816999)(59896002)(5004730100002)(110136002)(2950100001)(5008740100001)(586003)(92566002)(42186005)(3846002)(6116002)(36756003)(47776003)(4326007);DIR:OUT;SFP:1101;SCL:1;SRVR:BN4PR07MB2130;H:dl.caveonetworks.com;FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?iso-8859-1?Q?1;BN4PR07MB2130;23:e2BukMgJO+lLDM2mwRf1LiM6a8lwn/n4rLgfOt3?= =?iso-8859-1?Q?ifVswNdlCq3vlaBRpdveKc0EO1z0P1eWvct3uIwcUl8zPkoM/Udyo27G+t?= =?iso-8859-1?Q?BhfYA2wgLVWjRTjoFrjTK3l9inWgA+O7HUTKJ9nsBoc6jArY4geWdO5OHG?= =?iso-8859-1?Q?kmGTG9REoOZiPgreFKRO53wKSKRd0LrC/KkMkCk+wxxxWnKPQOpX//Zkbp?= =?iso-8859-1?Q?F4qm4q/gXK78mz2VGhHJaMtsFCZ9uInpOpgkdk/ShwIRDl4X8chfUaK9of?= =?iso-8859-1?Q?LQqe53y9DmMYzAr0OEP0xExznVtN4ef5XgEvhL7iLqkERCBGKuA/W/Bx9o?= =?iso-8859-1?Q?H3QojU6f4VCH+6Jy1oEM7ZseGXoETU347b/2kVbPUdM4AuaF8ZUkcUp+/i?= =?iso-8859-1?Q?VMIk5rQ0O3R5OpBe7gwpkB6MYvqeEuJAHWLr7lb2GG6B+ps85RtQeUo3a0?= =?iso-8859-1?Q?Pfq4RgIPu3QDqbztb1ZnUP5SuCK4mOlv3+oilfAVOZYcBWNe727Enf/1Hl?= =?iso-8859-1?Q?oJR4+uuBNZ0th6l1myte3Oshz3f1buEH1GUwRYdilJPQjOIQ9XHPesGLa5?= =?iso-8859-1?Q?kJNZsyLaJHAKdHDyisNqjOJfjfpMy/9RvQc7wajp340604DiTQK5QULbmE?= =?iso-8859-1?Q?VzaOCqNbWaiDtUw9q9Grx7n0nLI67N2VBWOdTP1hJExUTqcZGktsbMy9K7?= =?iso-8859-1?Q?51tsHB7ecI6bz8Ih9lsKmGrUbV5VEotGdR7BhL7///uIFJ7Zl/1CxVJrDi?= =?iso-8859-1?Q?vHENhE23883LnuaSnOPcA+Gy5Ay5i2i1+tG4J0Y9p9DqXuTm58ASpBqBhm?= =?iso-8859-1?Q?VlFoGzcxx23vNKUwXJyeohXiUD3sNvMlyqfrZn5RLkJgmk1fQ2JlGt0cba?= =?iso-8859-1?Q?9kWagoAq85wMS+LBnt2bVXImvuKYuSVdk0IXB86Ya908jJiUlNBPlK84CW?= =?iso-8859-1?Q?afYZFUydnh8glTLoBKYGt2cBqgF64Wg61xVJzhAPLJbqomtQ+lxqxrEitZ?= =?iso-8859-1?Q?EshkRoQAVHUUqtbQP2s0SK1z9E/evzWynX5QCSzZyb9ftysYHEN0+ltO9R?= =?iso-8859-1?Q?bjqfJhbd2RB7iahI/bClpOtMX4KCH7Cb47AZL46El+cv1ymS0xddZHKpnA?= =?iso-8859-1?Q?4UosM4eC68jHy74zlqfoBi6lGfLwZKkDdWacWx6N+pOwN/Au8GGpkXm06J?= =?iso-8859-1?Q?CK6g8V0ky5DiOANG+QV/QETpyuWBHtmPg=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;BN4PR07MB2130;5:8PXGY3ppNT7Z9eYD4yc+yy9GHF5S0DDla1cys/7zAijd3EIqA04G4/GlV16ht+5g64sDPsjhiR0TMGKqL7i6KTBP502OETN3SuwozySJgRggeHxWvhTm6FZJ8/ATXNooVUg4laWJDw7HhxieeQUyHg==;24:NuWMODtliuY4GK/ymvSeWZOimvZ46PaO8NW27L5MewBp4ASK4BPUst1BrAiKpVgrBdXptXeBXO6XdX7Phvu2m7HjSr8S1jwWm6kAv7mjUds=;7:L+OBv9nNRptnM8u6MSbgnnJbKAvBVEti5SOMsBS/kwrbh9l2p3zrIxZqCJmitmkwP24N0knWvwvCxoSRywpefTFsKRg+RVDVI61n5/1/jgla3kwAeBJbJ98Kd5AbVwJN+X0W2Y8r0Rc9APfhjOdTssBCEvFrY3t1TIdNMsW4R6Q5Vs1aWuRnHNFTIsa1wyMo SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: caviumnetworks.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 May 2016 15:59:18.2496 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN4PR07MB2130 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Cc: [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); >