Re: [PATCH v4] printk: Add monotonic, boottime, and realtime timestamps

[Mark Salyzyn <salyzyn@android.com>]

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