lkml.org 
[lkml]   [2017]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps
From
Date
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?
> +
> +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.

-- Mark

\
 
 \ /
  Last update: 2017-08-07 18:59    [W:0.272 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site