[PATCH 0/4] printk: some minor fixups

[PATCH 0/4] printk: some minor fixups

[Alex Elder]

This series makes some minor changes to kernel/printk/printk.c.
I've been poking around that file and will have a few more
substantive changes to propose soon, but I've been carrying
around these little things for a bit and thought I should
just get them out there.

					-Alex

This series, based on v3.16-rc4, is available here:
    http://git.linaro.org/landing-teams/working/broadcom/kernel.git
    Branch review/minor-printk-fixes

Alex Elder (4):
  printk: rename DEFAULT_MESSAGE_LOGLEVEL
  printk: fix some comments
  printk: use a clever macro
  printk: miscellaneous cleanups

 include/linux/printk.h |  2 +-
 kernel/printk/printk.c | 63 ++++++++++++++++++++++----------------------------
 2 files changed, 29 insertions(+), 36 deletions(-)

-- 
1.9.1

[PATCH 1/4] printk: rename DEFAULT_MESSAGE_LOGLEVEL

[Alex Elder]

This commit:
    a8fe19eb kernel/printk: use symbolic defines for console loglevels
makes consistent use of symbolic values for printk() log levels.

The naming scheme used is different from the one used for
DEFAULT_MESSAGE_LOGLEVEL though.  Change that symbol name to be
MESSAGE_LOGLEVEL_DEFAULT for consistency.

Note that we don't rename CONFIG_DEFAULT_MESSAGE_LOGLEVEL (to avoid
breaking existing config files that might reference it).

Signed-off-by: Alex Elder <elder@linaro.org>
---
 include/linux/printk.h | 2 +-
 kernel/printk/printk.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 319ff7e..3d1ccad 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -31,7 +31,7 @@ static inline const char *printk_skip_level(const char *buffer)
 }
 
 /* printk's without a loglevel use this.. */
-#define DEFAULT_MESSAGE_LOGLEVEL CONFIG_DEFAULT_MESSAGE_LOGLEVEL
+#define MESSAGE_LOGLEVEL_DEFAULT CONFIG_DEFAULT_MESSAGE_LOGLEVEL
 
 /* We show everything that is MORE important than this.. */
 #define CONSOLE_LOGLEVEL_SILENT  0 /* Mum's the word */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 13e839d..ac2b64e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -56,7 +56,7 @@
 
 int console_printk[4] = {
 	CONSOLE_LOGLEVEL_DEFAULT,	/* console_loglevel */
-	DEFAULT_MESSAGE_LOGLEVEL,	/* default_message_loglevel */
+	MESSAGE_LOGLEVEL_DEFAULT,	/* default_message_loglevel */
 	CONSOLE_LOGLEVEL_MIN,		/* minimum_console_loglevel */
 	CONSOLE_LOGLEVEL_DEFAULT,	/* default_console_loglevel */
 };
-- 
1.9.1

Re: [PATCH 1/4] printk: rename DEFAULT_MESSAGE_LOGLEVEL

[Borislav Petkov]

On Wed, Jul 09, 2014 at 08:04:13AM -0500, Alex Elder wrote:
> This commit:
>     a8fe19eb kernel/printk: use symbolic defines for console loglevels
> makes consistent use of symbolic values for printk() log levels.
> 
> The naming scheme used is different from the one used for
> DEFAULT_MESSAGE_LOGLEVEL though.  Change that symbol name to be
> MESSAGE_LOGLEVEL_DEFAULT for consistency.
> 
> Note that we don't rename CONFIG_DEFAULT_MESSAGE_LOGLEVEL (to avoid
> breaking existing config files that might reference it).
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  include/linux/printk.h | 2 +-
>  kernel/printk/printk.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 319ff7e..3d1ccad 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -31,7 +31,7 @@ static inline const char *printk_skip_level(const char *buffer)
>  }
>  
>  /* printk's without a loglevel use this.. */
> -#define DEFAULT_MESSAGE_LOGLEVEL CONFIG_DEFAULT_MESSAGE_LOGLEVEL
> +#define MESSAGE_LOGLEVEL_DEFAULT CONFIG_DEFAULT_MESSAGE_LOGLEVEL

Well, I can't say I like it - we have the config item
CONFIG_DEFAULT_MESSAGE_LOGLEVEL and DEFAULT_MESSAGE_LOGLEVEL resembles
it for a reason - it is the corresponding define coming from .config.

With this change you have:

CONFIG_DEFAULT_MESSAGE_LOGLEVEL
       MESSAGE_LOGLEVEL_DEFAULT

which is more confusing. To me at least. I can't see the resemblance at
a quick glance anymore.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 1/4] printk: rename DEFAULT_MESSAGE_LOGLEVEL

[Alex Elder]

On 07/09/2014 10:00 AM, Borislav Petkov wrote:
> On Wed, Jul 09, 2014 at 08:04:13AM -0500, Alex Elder wrote:
>> This commit:
>>      a8fe19eb kernel/printk: use symbolic defines for console loglevels
>> makes consistent use of symbolic values for printk() log levels.
>>
>> The naming scheme used is different from the one used for
>> DEFAULT_MESSAGE_LOGLEVEL though.  Change that symbol name to be
>> MESSAGE_LOGLEVEL_DEFAULT for consistency.
>>
>> Note that we don't rename CONFIG_DEFAULT_MESSAGE_LOGLEVEL (to avoid
>> breaking existing config files that might reference it).
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>   include/linux/printk.h | 2 +-
>>   kernel/printk/printk.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index 319ff7e..3d1ccad 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -31,7 +31,7 @@ static inline const char *printk_skip_level(const char *buffer)
>>   }
>>
>>   /* printk's without a loglevel use this.. */
>> -#define DEFAULT_MESSAGE_LOGLEVEL CONFIG_DEFAULT_MESSAGE_LOGLEVEL
>> +#define MESSAGE_LOGLEVEL_DEFAULT CONFIG_DEFAULT_MESSAGE_LOGLEVEL
>
> Well, I can't say I like it - we have the config item
> CONFIG_DEFAULT_MESSAGE_LOGLEVEL and DEFAULT_MESSAGE_LOGLEVEL resembles
> it for a reason - it is the corresponding define coming from .config.
>
> With this change you have:
>
> CONFIG_DEFAULT_MESSAGE_LOGLEVEL
>         MESSAGE_LOGLEVEL_DEFAULT
>
> which is more confusing. To me at least. I can't see the resemblance at
> a quick glance anymore.

Yes I realized this just sort of moved that sort of problem
to a different place.  The change was responding to the
inconsistency in naming in "printk.c".  I can control the
effects of that, but I can't predict who might be using
various config options, so I avoided doing that rename.

Was I being overly cautious on the config option name?
I could fix that too and have consistency everywhere.

					-Alex

Re: [PATCH 1/4] printk: rename DEFAULT_MESSAGE_LOGLEVEL

[Borislav Petkov]

On Wed, Jul 09, 2014 at 10:10:28AM -0500, Alex Elder wrote:
> Yes I realized this just sort of moved that sort of problem
> to a different place.  The change was responding to the
> inconsistency in naming in "printk.c".  I can control the
> effects of that, but I can't predict who might be using
> various config options, so I avoided doing that rename.
> 
> Was I being overly cautious on the config option name?
> I could fix that too and have consistency everywhere.

You mean turn it into CONFIG_MESSAGE_LOGLEVEL_DEFAULT?

I think you're free to do so because .config defines are not API anyway
and anyone who thinks so should get off the bad sh*t he's smoking.

:-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 1/4] printk: rename DEFAULT_MESSAGE_LOGLEVEL

[Alex Elder]

On 07/09/2014 10:13 AM, Borislav Petkov wrote:
> On Wed, Jul 09, 2014 at 10:10:28AM -0500, Alex Elder wrote:
>> Yes I realized this just sort of moved that sort of problem
>> to a different place.  The change was responding to the
>> inconsistency in naming in "printk.c".  I can control the
>> effects of that, but I can't predict who might be using
>> various config options, so I avoided doing that rename.
>>
>> Was I being overly cautious on the config option name?
>> I could fix that too and have consistency everywhere.
>
> You mean turn it into CONFIG_MESSAGE_LOGLEVEL_DEFAULT?

OK, I'll repost a little later with that change included.
Thanks.

					-Alex

Re: [PATCH 1/4] printk: rename DEFAULT_MESSAGE_LOGLEVEL

[Borislav Petkov]

On Wed, Jul 09, 2014 at 10:14:54AM -0500, Alex Elder wrote:
> OK, I'll repost a little later with that change included.

I'd wait a couple of days for the others to have a look too, if I were
you. Flooding people can be counterproductive. :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH 2/4] printk: fix some comments

[Alex Elder]

This patch fixes a few comments that don't accurately describe their
corresponding code.  It also fixes some minor typographical errors.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 kernel/printk/printk.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ac2b64e..646146c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -113,9 +113,9 @@ static int __down_trylock_console_sem(unsigned long ip)
  * This is used for debugging the mess that is the VT code by
  * keeping track if we have the console semaphore held. It's
  * definitely not the perfect debug tool (we don't know if _WE_
- * hold it are racing, but it helps tracking those weird code
- * path in the console code where we end up in places I want
- * locked without the console sempahore held
+ * hold it and are racing, but it helps tracking those weird code
+ * paths in the console code where we end up in places I want
+ * locked without the console sempahore held).
  */
 static int console_locked, console_suspended;
 
@@ -146,8 +146,8 @@ static int console_may_schedule;
  * the overall length of the record.
  *
  * The heads to the first and last entry in the buffer, as well as the
- * sequence numbers of these both entries are maintained when messages
- * are stored..
+ * sequence numbers of these entries are maintained when messages are
+ * stored.
  *
  * If the heads indicate available messages, the length in the header
  * tells the start next message. A length == 0 for the next message
@@ -344,7 +344,7 @@ static int log_make_free_space(u32 msg_size)
 	while (log_first_seq < log_next_seq) {
 		if (logbuf_has_space(msg_size, false))
 			return 0;
-		/* drop old messages until we have enough continuous space */
+		/* drop old messages until we have enough contiguous space */
 		log_first_idx = log_next(log_first_idx);
 		log_first_seq++;
 	}
@@ -1476,7 +1476,7 @@ static struct cont {
 	struct task_struct *owner;	/* task of first print*/
 	u64 ts_nsec;			/* time of first print */
 	u8 level;			/* log level of first message */
-	u8 facility;			/* log level of first message */
+	u8 facility;			/* log facility of first message */
 	enum log_flags flags;		/* prefix, newline flags */
 	bool flushed:1;			/* buffer sealed and committed */
 } cont;
@@ -1881,11 +1881,12 @@ static int __add_preferred_console(char *name, int idx, char *options,
 	return 0;
 }
 /*
- * Set up a list of consoles.  Called from init/main.c
+ * Set up a console.  Called via do_early_param() in init/main.c
+ * for each "console=" parameter in the boot command line.
  */
 static int __init console_setup(char *str)
 {
-	char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for index */
+	char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for "ttyS" */
 	char *s, *options, *brl_options = NULL;
 	int idx;
 
@@ -2045,8 +2046,8 @@ EXPORT_SYMBOL(console_lock);
 /**
  * console_trylock - try to lock the console system for exclusive use.
  *
- * Tried to acquire a lock which guarantees that the caller has
- * exclusive access to the console system and the console_drivers list.
+ * Try to acquire a lock which guarantees that the caller has exclusive
+ * access to the console system and the console_drivers list.
  *
  * returns 1 on success, and 0 on failure to acquire the lock.
  */
-- 
1.9.1

Re: [PATCH 2/4] printk: fix some comments

[Petr Mládek]

On Wed 2014-07-09 08:04:14, Alex Elder wrote:
> This patch fixes a few comments that don't accurately describe their
> corresponding code.  It also fixes some minor typographical errors.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>

Nice clean up. All changes make sense to me.

Reviewed-by: Petr Mladek <pmladek@suse.cz>

Best Regards,
Petr

> ---
>  kernel/printk/printk.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ac2b64e..646146c 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -113,9 +113,9 @@ static int __down_trylock_console_sem(unsigned long ip)
>   * This is used for debugging the mess that is the VT code by
>   * keeping track if we have the console semaphore held. It's
>   * definitely not the perfect debug tool (we don't know if _WE_
> - * hold it are racing, but it helps tracking those weird code
> - * path in the console code where we end up in places I want
> - * locked without the console sempahore held
> + * hold it and are racing, but it helps tracking those weird code
> + * paths in the console code where we end up in places I want
> + * locked without the console sempahore held).
>   */
>  static int console_locked, console_suspended;
>  
> @@ -146,8 +146,8 @@ static int console_may_schedule;
>   * the overall length of the record.
>   *
>   * The heads to the first and last entry in the buffer, as well as the
> - * sequence numbers of these both entries are maintained when messages
> - * are stored..
> + * sequence numbers of these entries are maintained when messages are
> + * stored.
>   *
>   * If the heads indicate available messages, the length in the header
>   * tells the start next message. A length == 0 for the next message
> @@ -344,7 +344,7 @@ static int log_make_free_space(u32 msg_size)
>  	while (log_first_seq < log_next_seq) {
>  		if (logbuf_has_space(msg_size, false))
>  			return 0;
> -		/* drop old messages until we have enough continuous space */
> +		/* drop old messages until we have enough contiguous space */
>  		log_first_idx = log_next(log_first_idx);
>  		log_first_seq++;
>  	}
> @@ -1476,7 +1476,7 @@ static struct cont {
>  	struct task_struct *owner;	/* task of first print*/
>  	u64 ts_nsec;			/* time of first print */
>  	u8 level;			/* log level of first message */
> -	u8 facility;			/* log level of first message */
> +	u8 facility;			/* log facility of first message */
>  	enum log_flags flags;		/* prefix, newline flags */
>  	bool flushed:1;			/* buffer sealed and committed */
>  } cont;
> @@ -1881,11 +1881,12 @@ static int __add_preferred_console(char *name, int idx, char *options,
>  	return 0;
>  }
>  /*
> - * Set up a list of consoles.  Called from init/main.c
> + * Set up a console.  Called via do_early_param() in init/main.c
> + * for each "console=" parameter in the boot command line.
>   */
>  static int __init console_setup(char *str)
>  {
> -	char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for index */
> +	char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for "ttyS" */
>  	char *s, *options, *brl_options = NULL;
>  	int idx;
>  
> @@ -2045,8 +2046,8 @@ EXPORT_SYMBOL(console_lock);
>  /**
>   * console_trylock - try to lock the console system for exclusive use.
>   *
> - * Tried to acquire a lock which guarantees that the caller has
> - * exclusive access to the console system and the console_drivers list.
> + * Try to acquire a lock which guarantees that the caller has exclusive
> + * access to the console system and the console_drivers list.
>   *
>   * returns 1 on success, and 0 on failure to acquire the lock.
>   */
> -- 
> 1.9.1
> 

[PATCH 3/4] printk: use a clever macro

[Alex Elder]

Use the IS_ENABLED() macro rather than #ifdef blocks to set certain
global values.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 kernel/printk/printk.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 646146c..6f75e8a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -453,11 +453,7 @@ static int log_store(int facility, int level,
 	return msg->text_len;
 }
 
-#ifdef CONFIG_SECURITY_DMESG_RESTRICT
-int dmesg_restrict = 1;
-#else
-int dmesg_restrict;
-#endif
+int dmesg_restrict = IS_ENABLED(CONFIG_SECURITY_DMESG_RESTRICT);
 
 static int syslog_action_restricted(int type)
 {
@@ -947,11 +943,7 @@ static inline void boot_delay_msec(int level)
 }
 #endif
 
-#if defined(CONFIG_PRINTK_TIME)
-static bool printk_time = 1;
-#else
-static bool printk_time;
-#endif
+static bool printk_time = IS_ENABLED(CONFIG_PRINTK_TIME);
 module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);
 
 static size_t print_time(u64 ts, char *buf)
-- 
1.9.1

Re: [PATCH 3/4] printk: use a clever macro

[Borislav Petkov]

On Wed, Jul 09, 2014 at 08:04:15AM -0500, Alex Elder wrote:
> Use the IS_ENABLED() macro rather than #ifdef blocks to set certain
> global values.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>

That's good - every patch which removes ifdeffery is a good patch. :)

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/4] printk: use a clever macro

[Petr Mládek]

On Wed 2014-07-09 08:04:15, Alex Elder wrote:
> Use the IS_ENABLED() macro rather than #ifdef blocks to set certain
> global values.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  kernel/printk/printk.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 646146c..6f75e8a 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -453,11 +453,7 @@ static int log_store(int facility, int level,
>  	return msg->text_len;
>  }
>  
> -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> -int dmesg_restrict = 1;
> -#else
> -int dmesg_restrict;
> -#endif
> +int dmesg_restrict = IS_ENABLED(CONFIG_SECURITY_DMESG_RESTRICT);

I wondered if IS_ENABLED() is guaranteed to set 1 when enabled. Well, it
does not matter because the variable is used as a boolean.

>  static int syslog_action_restricted(int type)
>  {
> @@ -947,11 +943,7 @@ static inline void boot_delay_msec(int level)
>  }
>  #endif
>  
> -#if defined(CONFIG_PRINTK_TIME)
> -static bool printk_time = 1;
> -#else
> -static bool printk_time;
> -#endif
> +static bool printk_time = IS_ENABLED(CONFIG_PRINTK_TIME);
>  module_param_named(time, printk_time, bool, S_IRUGO | S_IWUSR);
>  
>  static size_t print_time(u64 ts, char *buf)
> -- 
> 1.9.1

Nice clean up.

Reviewed-by: Petr Mladek <pmladek@suse.cz>

Best Regards,
Petr

Re: [PATCH 3/4] printk: use a clever macro

[Borislav Petkov]

On Wed, Jul 09, 2014 at 06:19:46PM +0200, Petr Mládek wrote:
> I wondered if IS_ENABLED() is guaranteed to set 1 when enabled.

See include/linux/kconfig.h

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/4] printk: use a clever macro

[Petr Mládek]

On Wed 2014-07-09 18:24:04, Borislav Petkov wrote:
> On Wed, Jul 09, 2014 at 06:19:46PM +0200, Petr Mládek wrote:
> > I wondered if IS_ENABLED() is guaranteed to set 1 when enabled.
> 
> See include/linux/kconfig.h

I know that it sets 1 now. I wondered about possible future
changes. Well, it is probably too paranoid. Anyway, any non-zero value
is fine.

Best Regards,
Petr

Re: [PATCH 3/4] printk: use a clever macro

[Borislav Petkov]

On Wed, Jul 09, 2014 at 06:32:05PM +0200, Petr Mládek wrote:
> I know that it sets 1 now. I wondered about possible future changes.
> Well, it is probably too paranoid. Anyway, any non-zero value is fine.

Yes, because they both are used only in a boolean context.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/4] printk: use a clever macro

[Geert Uytterhoeven]

Hi Alex,

On Wed, Jul 9, 2014 at 3:04 PM, Alex Elder <elder@linaro.org> wrote:
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -453,11 +453,7 @@ static int log_store(int facility, int level,
>         return msg->text_len;
>  }
>
> -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> -int dmesg_restrict = 1;
> -#else
> -int dmesg_restrict;
> -#endif
> +int dmesg_restrict = IS_ENABLED(CONFIG_SECURITY_DMESG_RESTRICT);

Doesn't this move dmesg_restrict from the bss to the data section
in case CONFIG_SECURITY_DMESG_RESTRICT is not enabled, due
to the explicit initialization to zero?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/4] printk: use a clever macro

[Alex Elder]

On 07/09/2014 12:58 PM, Geert Uytterhoeven wrote:
> Hi Alex,
> 
> On Wed, Jul 9, 2014 at 3:04 PM, Alex Elder <elder@linaro.org> wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -453,11 +453,7 @@ static int log_store(int facility, int level,
>>         return msg->text_len;
>>  }
>>
>> -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
>> -int dmesg_restrict = 1;
>> -#else
>> -int dmesg_restrict;
>> -#endif
>> +int dmesg_restrict = IS_ENABLED(CONFIG_SECURITY_DMESG_RESTRICT);
> 
> Doesn't this move dmesg_restrict from the bss to the data section
> in case CONFIG_SECURITY_DMESG_RESTRICT is not enabled, due
> to the explicit initialization to zero?

I honestly don't know.  Is that even a well-defined behavior?
Couldn't the compiler, noting an explicit 0 initialization,
put it into BSS anyway?

In any case, does this distinction matter?

					-Alex

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

[PATCH 4/4] printk: miscellaneous cleanups

[Alex Elder]

This patch contains some small cleanups to kernel/printk/printk.c.
None of them should cause any change in behavior.
  - When CONFIG_PRINTK is defined, parenthesize the value of LOG_LINE_MAX.
  - When CONFIG_PRINTK is *not* defined, there is an extra LOG_LINE_MAX
    definition; delete it.
  - Pull an assignment out of a conditional expression in console_setup().
  - Use isdigit() in console_setup() rather than open coding it.
  - In update_console_cmdline(), drop a NUL-termination assignment;
    the strlcpy() call that precedes it guarantees it's not needed.
  - Simplify some logic in printk_timed_ratelimit().

Signed-off-by: Alex Elder <elder@linaro.org>
---
 kernel/printk/printk.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 6f75e8a..909029e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -45,6 +45,7 @@
 #include <linux/poll.h>
 #include <linux/irq_work.h>
 #include <linux/utsname.h>
+#include <linux/ctype.h>
 
 #include <asm/uaccess.h>
 
@@ -257,7 +258,7 @@ static u64 clear_seq;
 static u32 clear_idx;
 
 #define PREFIX_MAX		32
-#define LOG_LINE_MAX		1024 - PREFIX_MAX
+#define LOG_LINE_MAX		(1024 - PREFIX_MAX)
 
 /* record buffer */
 #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
@@ -1794,7 +1795,7 @@ EXPORT_SYMBOL(printk);
 
 #define LOG_LINE_MAX		0
 #define PREFIX_MAX		0
-#define LOG_LINE_MAX 0
+
 static u64 syslog_seq;
 static u32 syslog_idx;
 static u64 console_seq;
@@ -1895,7 +1896,8 @@ static int __init console_setup(char *str)
 		strncpy(buf, str, sizeof(buf) - 1);
 	}
 	buf[sizeof(buf) - 1] = 0;
-	if ((options = strchr(str, ',')) != NULL)
+	options = strchr(str, ',');
+	if (options)
 		*(options++) = 0;
 #ifdef __sparc__
 	if (!strcmp(str, "ttya"))
@@ -1904,7 +1906,7 @@ static int __init console_setup(char *str)
 		strcpy(buf, "ttyS1");
 #endif
 	for (s = buf; *s; s++)
-		if ((*s >= '0' && *s <= '9') || *s == ',')
+		if (isdigit(*s) || *s == ',')
 			break;
 	idx = simple_strtoul(s, NULL, 10);
 	*s = 0;
@@ -1943,7 +1945,6 @@ int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, cha
 	     i++, c++)
 		if (strcmp(c->name, name) == 0 && c->index == idx) {
 			strlcpy(c->name, name_new, sizeof(c->name));
-			c->name[sizeof(c->name) - 1] = 0;
 			c->options = options;
 			c->index = idx_new;
 			return i;
@@ -2611,14 +2612,13 @@ EXPORT_SYMBOL(__printk_ratelimit);
 bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 			unsigned int interval_msecs)
 {
-	if (*caller_jiffies == 0
-			|| !time_in_range(jiffies, *caller_jiffies,
-					*caller_jiffies
-					+ msecs_to_jiffies(interval_msecs))) {
-		*caller_jiffies = jiffies;
-		return true;
-	}
-	return false;
+	unsigned long elapsed = jiffies - *caller_jiffies;
+
+	if (*caller_jiffies && elapsed <= msecs_to_jiffies(interval_msecs))
+		return false;
+
+	*caller_jiffies = jiffies;
+	return true;
 }
 EXPORT_SYMBOL(printk_timed_ratelimit);
 
-- 
1.9.1

Re: [PATCH 4/4] printk: miscellaneous cleanups

[Petr Mládek]

Sending once again as a correct reply. I am sorry for the
confusion. I think that it is high time for me to go home and sleep :-)

On Wed 2014-07-09 08:04:16, Alex Elder wrote:
> This patch contains some small cleanups to kernel/printk/printk.c.
> None of them should cause any change in behavior.
>   - When CONFIG_PRINTK is defined, parenthesize the value of LOG_LINE_MAX.
>   - When CONFIG_PRINTK is *not* defined, there is an extra LOG_LINE_MAX
>     definition; delete it.
>   - Pull an assignment out of a conditional expression in console_setup().
>   - Use isdigit() in console_setup() rather than open coding it.
>   - In update_console_cmdline(), drop a NUL-termination assignment;
>     the strlcpy() call that precedes it guarantees it's not needed.
>   - Simplify some logic in printk_timed_ratelimit().
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  kernel/printk/printk.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 6f75e8a..909029e 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -45,6 +45,7 @@
>  #include <linux/poll.h>
>  #include <linux/irq_work.h>
>  #include <linux/utsname.h>
> +#include <linux/ctype.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -257,7 +258,7 @@ static u64 clear_seq;
>  static u32 clear_idx;
>  
>  #define PREFIX_MAX		32
> -#define LOG_LINE_MAX		1024 - PREFIX_MAX
> +#define LOG_LINE_MAX		(1024 - PREFIX_MAX)
>  
>  /* record buffer */
>  #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> @@ -1794,7 +1795,7 @@ EXPORT_SYMBOL(printk);
>  
>  #define LOG_LINE_MAX		0
>  #define PREFIX_MAX		0
> -#define LOG_LINE_MAX 0
> +
>  static u64 syslog_seq;
>  static u32 syslog_idx;
>  static u64 console_seq;
> @@ -1895,7 +1896,8 @@ static int __init console_setup(char *str)
>  		strncpy(buf, str, sizeof(buf) - 1);
>  	}
>  	buf[sizeof(buf) - 1] = 0;
> -	if ((options = strchr(str, ',')) != NULL)
> +	options = strchr(str, ',');
> +	if (options)
>  		*(options++) = 0;
>  #ifdef __sparc__
>  	if (!strcmp(str, "ttya"))
> @@ -1904,7 +1906,7 @@ static int __init console_setup(char *str)
>  		strcpy(buf, "ttyS1");
>  #endif
>  	for (s = buf; *s; s++)
> -		if ((*s >= '0' && *s <= '9') || *s == ',')
> +		if (isdigit(*s) || *s == ',')
>  			break;
>  	idx = simple_strtoul(s, NULL, 10);
>  	*s = 0;
> @@ -1943,7 +1945,6 @@ int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, cha
>  	     i++, c++)
>  		if (strcmp(c->name, name) == 0 && c->index == idx) {
>  			strlcpy(c->name, name_new, sizeof(c->name));
> -			c->name[sizeof(c->name) - 1] = 0;
>  			c->options = options;
>  			c->index = idx_new;
>  			return i;
> @@ -2611,14 +2612,13 @@ EXPORT_SYMBOL(__printk_ratelimit);
>  bool printk_timed_ratelimit(unsigned long *caller_jiffies,
>  			unsigned int interval_msecs)
>  {
> -	if (*caller_jiffies == 0
> -			|| !time_in_range(jiffies, *caller_jiffies,
> -					*caller_jiffies
> -					+ msecs_to_jiffies(interval_msecs))) {
> -		*caller_jiffies = jiffies;
> -		return true;
> -	}
> -	return false;
> +	unsigned long elapsed = jiffies - *caller_jiffies;

I wondered if the deduction might be negative. Well, it should not be
if none manipulates *caller_jiffies in a strange way. If it happens,
the following condition will most likely fail and *caller_jiffies will
get reset to jiffies. It looks reasonable to me.

Reviewed-by: Petr Mladek <pmladek@suse.cz>

> +	if (*caller_jiffies && elapsed <= msecs_to_jiffies(interval_msecs))
> +		return false;
> +
> +	*caller_jiffies = jiffies;
> +	return true;
>  }
>  EXPORT_SYMBOL(printk_timed_ratelimit);

Best Regards,
Petr

Re: [PATCH 4/4] printk: miscellaneous cleanups

[Alex Elder]

On 07/09/2014 11:29 AM, Petr Mládek wrote:
> Sending once again as a correct reply. I am sorry for the
> confusion. I think that it is high time for me to go home and sleep :-)
>
> On Wed 2014-07-09 08:04:16, Alex Elder wrote:
>> This patch contains some small cleanups to kernel/printk/printk.c.
>> None of them should cause any change in behavior.
>>    - When CONFIG_PRINTK is defined, parenthesize the value of LOG_LINE_MAX.
>>    - When CONFIG_PRINTK is *not* defined, there is an extra LOG_LINE_MAX
>>      definition; delete it.
>>    - Pull an assignment out of a conditional expression in console_setup().
>>    - Use isdigit() in console_setup() rather than open coding it.
>>    - In update_console_cmdline(), drop a NUL-termination assignment;
>>      the strlcpy() call that precedes it guarantees it's not needed.
>>    - Simplify some logic in printk_timed_ratelimit().
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>   kernel/printk/printk.c | 26 +++++++++++++-------------
>>   1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index 6f75e8a..909029e 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c

. . .

>> @@ -2611,14 +2612,13 @@ EXPORT_SYMBOL(__printk_ratelimit);
>>   bool printk_timed_ratelimit(unsigned long *caller_jiffies,
>>   			unsigned int interval_msecs)
>>   {
>> -	if (*caller_jiffies == 0
>> -			|| !time_in_range(jiffies, *caller_jiffies,
>> -					*caller_jiffies
>> -					+ msecs_to_jiffies(interval_msecs))) {
>> -		*caller_jiffies = jiffies;
>> -		return true;
>> -	}
>> -	return false;
>> +	unsigned long elapsed = jiffies - *caller_jiffies;
>

Currently, all callers pass a 0 value in *caller_jiffies
initially (using a static/BSS variable), and a value updated
by a previous call to __printk_ratelimit() thereafter.

If a caller passed something different, yes, it's possible the
result would wrap to a high unsigned value (i.e., go negative).
However the logic used here involves the same subtraction
operation as was used before--though previously that was
buried inside the time_after() macro called by time_in_range().

					-Alex

> I wondered if the deduction might be negative. Well, it should not be
> if none manipulates *caller_jiffies in a strange way. If it happens,
> the following condition will most likely fail and *caller_jiffies will
> get reset to jiffies. It looks reasonable to me.
>
> Reviewed-by: Petr Mladek <pmladek@suse.cz>
>
>> +	if (*caller_jiffies && elapsed <= msecs_to_jiffies(interval_msecs))
>> +		return false;
>> +
>> +	*caller_jiffies = jiffies;
>> +	return true;
>>   }
>>   EXPORT_SYMBOL(printk_timed_ratelimit);
>
> Best Regards,
> Petr
>

[PATCH 4/4] printk: miscellaneous cleanups

[Alex Elder]

This patch contains some small cleanups to kernel/printk/printk.c.
None of them should cause any change in behavior.
  - When CONFIG_PRINTK is defined, parenthesize the value of LOG_LINE_MAX.
  - When CONFIG_PRINTK is *not* defined, there is an extra LOG_LINE_MAX
    definition; delete it.
  - Pull an assignment out of a conditional expression in console_setup().
  - Use isdigit() in console_setup() rather than open coding it.
  - In update_console_cmdline(), drop a NUL-termination assignment;
    the strlcpy() call that precedes it guarantees it's not needed.
  - Simplify some logic in printk_timed_ratelimit().

Signed-off-by: Alex Elder <elder@linaro.org>
---
 kernel/printk/printk.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 6f75e8a..909029e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -45,6 +45,7 @@
 #include <linux/poll.h>
 #include <linux/irq_work.h>
 #include <linux/utsname.h>
+#include <linux/ctype.h>
 
 #include <asm/uaccess.h>
 
@@ -257,7 +258,7 @@ static u64 clear_seq;
 static u32 clear_idx;
 
 #define PREFIX_MAX		32
-#define LOG_LINE_MAX		1024 - PREFIX_MAX
+#define LOG_LINE_MAX		(1024 - PREFIX_MAX)
 
 /* record buffer */
 #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
@@ -1794,7 +1795,7 @@ EXPORT_SYMBOL(printk);
 
 #define LOG_LINE_MAX		0
 #define PREFIX_MAX		0
-#define LOG_LINE_MAX 0
+
 static u64 syslog_seq;
 static u32 syslog_idx;
 static u64 console_seq;
@@ -1895,7 +1896,8 @@ static int __init console_setup(char *str)
 		strncpy(buf, str, sizeof(buf) - 1);
 	}
 	buf[sizeof(buf) - 1] = 0;
-	if ((options = strchr(str, ',')) != NULL)
+	options = strchr(str, ',');
+	if (options)
 		*(options++) = 0;
 #ifdef __sparc__
 	if (!strcmp(str, "ttya"))
@@ -1904,7 +1906,7 @@ static int __init console_setup(char *str)
 		strcpy(buf, "ttyS1");
 #endif
 	for (s = buf; *s; s++)
-		if ((*s >= '0' && *s <= '9') || *s == ',')
+		if (isdigit(*s) || *s == ',')
 			break;
 	idx = simple_strtoul(s, NULL, 10);
 	*s = 0;
@@ -1943,7 +1945,6 @@ int update_console_cmdline(char *name, int idx, char *name_new, int idx_new, cha
 	     i++, c++)
 		if (strcmp(c->name, name) == 0 && c->index == idx) {
 			strlcpy(c->name, name_new, sizeof(c->name));
-			c->name[sizeof(c->name) - 1] = 0;
 			c->options = options;
 			c->index = idx_new;
 			return i;
@@ -2611,14 +2612,13 @@ EXPORT_SYMBOL(__printk_ratelimit);
 bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 			unsigned int interval_msecs)
 {
-	if (*caller_jiffies == 0
-			|| !time_in_range(jiffies, *caller_jiffies,
-					*caller_jiffies
-					+ msecs_to_jiffies(interval_msecs))) {
-		*caller_jiffies = jiffies;
-		return true;
-	}
-	return false;
+	unsigned long elapsed = jiffies - *caller_jiffies;

I wondered if the deduction might be negative. Well, it should not be
if none manipulates *caller_jiffies a strange way. If it happens,
the following condition will most likely fail and *caller_jiffies will
get reset to jiffies. It looks reasonable to me.

Reviewed-by: Petr Mladek <pmladek@suse.cz>

+
+	if (*caller_jiffies && elapsed <= msecs_to_jiffies(interval_msecs))
+		return false;
+
+	*caller_jiffies = jiffies;
+	return true;
 }
 EXPORT_SYMBOL(printk_timed_ratelimit);

Best Regards,
Petr