linux-kernel - Re: [PATCH 3/4] timekeeping: Ignore the bogus sleep time if pm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161121081755.GB14231@gmail.com>
Date:   Mon, 21 Nov 2016 09:17:55 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     John Stultz <john.stultz@...aro.org>
Cc:     lkml <linux-kernel@...r.kernel.org>, Chen Yu <yu.c.chen@...el.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Xunlei Pang <xlpang@...hat.com>,
        Ingo Molnar <mingo@...hat.com>, Len Brown <lenb@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>, Pavel Machek <pavel@....cz>,
        Thomas Gleixner <tglx@...utronix.de>,
        Prarit Bhargava <prarit@...hat.com>,
        Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH 3/4] timekeeping: Ignore the bogus sleep time if pm_trace
 is enabled


* John Stultz <john.stultz@...aro.org> wrote:

> +static int pm_trace_notify(struct notifier_block *nb,
> +				unsigned long mode, void *_unused)
> +{
> +	switch (mode) {
> +	case PM_POST_HIBERNATION:
> +	case PM_POST_SUSPEND:
> +		if (pm_trace_rtc_abused) {
> +			pm_trace_rtc_abused = false;
> +			pr_warn("Possible incorrect RTC due to pm_trace,"
> +				"please use ntp-date or rdate to reset.\n");

Please don't break user-visible strings just to pacify checkpatch!

The bogus linebreak above hides a type in the user string:

  Possible incorrect RTC due to pm_trace,please use ntp-date or rdate to reset.

(There's a missing space after the comma.)

Best practice is to preserve the continuous nature of the user string in the code. 

In addition to that, please quote suggested command names, i.e. something like:

  Possible incorrect RTC due to pm_trace, please use 'ntp-date' or 'rdate' to reset it.

> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -191,6 +191,13 @@ static inline void cmos_write_bank2(unsigned char val, unsigned char addr)
>  
>  static int cmos_read_time(struct device *dev, struct rtc_time *t)
>  {
> +	/*
> +	 * If pmtrace abused the RTC for storage tell the caller that it is
> +	 * unusable.
> +	 */
> +	if (!pm_trace_rtc_valid())
> +		return -EIO;

Please standardize the spelling of 'pm_trace', as there's 3 variants present in 
this patch alone:

  'pm_trace'
  'pm trace'
  'pmtrace'

(Not to mention pm-trace.h - but that's a pre-existing inconsistency unrelated to 
your patch.)

Thanks,

	Ingo