Re: [PATCH v3] printk: Add boottime and real timestamps

[Mark Salyzyn]

On 08/03/2017 06:18 PM, Prarit Bhargava wrote:

> diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
> index cfc2465e8b77..6c73c305ad17 100644
> --- a/arch/arm/configs/aspeed_g4_defconfig
> +++ b/arch/arm/configs/aspeed_g4_defconfig
> @@ -162,7 +162,9 @@ CONFIG_JFFS2_FS_XATTR=y
>  CONFIG_UBIFS_FS=y
>  CONFIG_SQUASHFS=y
>  CONFIG_SQUASHFS_XZ=y
> -CONFIG_PRINTK_TIME=y
> +# CONFIG_PRINTK_TIME_DISABLE is not set
> +CONFIG_PRINTK_TIME_LOCAL=Y
> +CONFIG_PRINTK_TIME=1
>  CONFIG_DYNAMIC_DEBUG=y
>  CONFIG_STRIP_ASM_SYMS=y
>  CONFIG_DEBUG_FS=y

savedefconfig tells me otherwise:

-CONFIG_PRINTK_TIME=y
+CONFIG_PRINTK_TIME_LOCAL=y

is all that is needed, the rest is fluff and will cause a savedefconfig 
mismatch error.
> +static int printk_time_set(const char *val, const struct kernel_param *kp)
> +{
> +	char *param = strstrip((char *)val);
> +	int _printk_time;
> +
> +	if (strlen(param) != 1)
> +		return -EINVAL;
(see below) strlen is so restrictive, wouldn't it be nice(tm) to allow 
"boot", "monotonic" or "realtime" strings? Certainly KISS can handle the 
first letters for next to zero cost. (switch statement optimization)
> +
> +	switch (param[0]) {
> +	case '0':
> +	case 'n':
> +	case 'N':
I would wish for a few more 'convenience' cases:

case 'd': case 'D': /* Disable */
> +		_printk_time = PRINTK_TIME_DISABLE;
> +		break;
> +	case '1':
> +	case 'y':
> +	case 'Y':
> +		_printk_time = PRINTK_TIME_LOCAL;
> +		break;
> +	case '2':
case 'b': case 'B': /* Boottime */
> +		_printk_time = PRINTK_TIME_BOOT;
> +		break;
> +	case '3':
case 'm': case 'M': /* Monotonic */
> +		_printk_time = PRINTK_TIME_MONO;
> +		break;
> +	case '4':
case 'r': case 'R': /* Realtime */
> +		_printk_time = PRINTK_TIME_REAL;
> +		break;
> +	default:
> +		pr_warn("printk: invalid timestamp value\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * 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 %d(%s) ",
> +			printk_time_setting,
> +			printk_time_str[printk_time_setting]);
> +		return -EINVAL;
> +	}
> +
> +	printk_time = _printk_time;
> +	pr_info("printk: timestamp set to %d(%s).",
> +		printk_time, printk_time_str[printk_time]);
> +	return 0;
> +}
> . . .
>   /* time in seconds when suspend began for persistent clock */
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 98fe715522e8..a71ba5689814 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1,8 +1,46 @@
>   menu "printk and dmesg options"
>   
> +choice
> +	prompt "printk default clock"
> +	config PRINTK_TIME_DISABLE
> +	bool "Disabled"
> +	help
> +	 Selecting this option disables the time stamps of printk().
> +
> +	config PRINTK_TIME_LOCAL
> +	bool "Local Clock"
> +	help
> +	  Selecting this option causes the time stamps of printk() to be
> +	  stamped with the unadjusted hardware clock.
> +
> +	config PRINTK_TIME_BOOT
> +	bool "CLOCK_BOOTTIME"
> +	help
> +	  Selecting this option causes the time stamps of printk() to be
> +	  stamped with the adjusted boottime clock.
> +
> +	config PRINTK_TIME_MONO
> +	bool "CLOCK_MONOTONIC"
> +	help
> +	  Selecting this option causes the time stamps of printk() to be
> +	  stamped with the adjusted monotonic clock.
> +
> +	config PRINTK_TIME_REAL
> +	bool "CLOCK_REALTIME"
> +	help
> +	  Selecting this option causes the time stamps of printk() to be
> +	  stamped with the adjusted realtime clock.
> +
> +endchoice
This will ensure mutual exclusivity, no need to add # 
CONFIG_PRINTK_TIME_DISABLE is not set to configs.
> +
>   config PRINTK_TIME
> -	bool "Show timing information on printks"
> +	int "Show time stamp information on printks"
>   	depends on PRINTK
> +	default 0 if PRINTK_TIME_DISABLE
> +	default 1 if PRINTK_TIME_LOCAL
> +	default 2 if PRINTK_TIME_BOOT
> +	default 3 if PRINTK_TIME_MONO
> +	default 4 if PRINTK_TIME_REAL
>   	help
>   	  Selecting this option causes time stamps of the printk()
>   	  messages to be added to the output of the syslog() system
This "dummy" config will be automated by the select, so no need to add 
CONFIG_PRINTK_TIME=<x> to the defconfigs.

-- Mark