From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751950AbdHGSHP (ORCPT ); Mon, 7 Aug 2017 14:07:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55396 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751800AbdHGSHN (ORCPT ); Mon, 7 Aug 2017 14:07:13 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 1BF38C047B78 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=prarit@redhat.com Subject: Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps To: Mark Salyzyn , linux-kernel@vger.kernel.org References: <1502121162-27981-1-git-send-email-prarit@redhat.com> Cc: Jonathan Corbet , Petr Mladek , Sergey Senozhatsky , Steven Rostedt , John Stultz , Thomas Gleixner , Stephen Boyd , Andrew Morton , Greg Kroah-Hartman , "Paul E. McKenney" , Christoffer Dall , Deepa Dinamani , Ingo Molnar , Joel Fernandes , Kees Cook , Peter Zijlstra , Geert Uytterhoeven , "Luis R. Rodriguez" , Nicholas Piggin , "Jason A. Donenfeld" , Olof Johansson , Josh Poimboeuf , linux-doc@vger.kernel.org From: Prarit Bhargava Message-ID: <999cbd46-976b-4513-ef47-b6ca761954bc@redhat.com> Date: Mon, 7 Aug 2017 14:07:09 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Mon, 07 Aug 2017 18:07:13 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/07/2017 12:58 PM, Mark Salyzyn wrote: > On 08/07/2017 08:52 AM, Prarit Bhargava wrote: >> diff --git a/arch/arm/configs/aspeed_g4_defconfig >> b/arch/arm/configs/aspeed_g4_defconfig >> index cfc2465e8b77..5f3c50914e92 100644 >> --- a/arch/arm/configs/aspeed_g4_defconfig >> +++ b/arch/arm/configs/aspeed_g4_defconfig >> @@ -162,7 +162,7 @@ CONFIG_JFFS2_FS_XATTR=y >> CONFIG_UBIFS_FS=y >> CONFIG_SQUASHFS=y >> CONFIG_SQUASHFS_XZ=y >> -CONFIG_PRINTK_TIME=y >> +CONFIG_PRINTK_TIME_LOCAL=y >> CONFIG_DYNAMIC_DEBUG=y >> CONFIG_STRIP_ASM_SYMS=y >> CONFIG_DEBUG_FS=y > Many have had misgivings, let me try another pass at this. > > We (royal we) should really look into adjusting configuration parsing to allow > an easy transition from boolean to selection ... I am sure this is not the first > time bistate/tristate was moved to a number. > > An idea? Maybe look into a way to deal with this to use something _other_ than > CONFIG_PRINTK_TIME to hold the selection, and keep a (hidden/legacy?) > CONFIG_PRINTK_TIME that when selected sets CONFIG_PRINTK_TIME_LOCAL, and switch > to _not_ CONFIG_PRINTK_TIME_DISABLE as the internal mechanical replacement for > it. I do not know how disruptive this will be, but is worth it if the codebase > supports it, and legacy config retained? I looked for one but couldn't find one. The kernel is a big place, though, and perhaps it already exists :/. >> + >> +static int printk_time_set(const char *val, const struct kernel_param *kp) >> +{ >> + char *param = strstrip((char *)val); >> + int _printk_time = -1; >> + int stamp; >> + >> + if (strlen(param) == 1) { >> + /* Preserve legacy boolean settings */ >> + if (!strcmp("0", param) || !strcmp("n", param) || > if strlen(param) == 1, then param[0] == '0' etc works fine and is KISS. >> + /* >> + * Only allow enabling and disabling of the current printk_time >> + * setting. Changing it from one setting to another confuses >> + * userspace. >> + */ >> + if (printk_time_setting == PRINTK_TIME_DISABLE) { >> + printk_time_setting = _printk_time; >> + } else if ((printk_time_setting != _printk_time) && >> + (_printk_time != 0)) { >> + pr_warn("printk: timestamp can only be set to 0(disabled) or %s\n", >> + printk_time_str[printk_time_setting]); >> + return -EINVAL; >> + } > I agree with the restriction in the general case. However (as hinted at > before() #ifdef CONFIG_PRINTK_TIME_RESTRICT (default y, or #ifndef > CONFIG_PRINTK_TIME_DEBUG default n) around this will allow us users to choose if > we are confused or not. I can see being able to change it on the fly as an > option. Especially since we have /sys/module/printk/parameters/time. Yeah, but I think that should be a later enhancement. P. > > -- Mark