Date: Mon, 21 Nov 2016 09:17:55 +0100
From: Ingo Molnar <mingo@kernel.org>
To: John Stultz <john.stultz@linaro.org>
Cc: lkml <linux-kernel@vger.kernel.org>, Chen Yu <yu.c.chen@intel.com>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Xunlei Pang <xlpang@redhat.com>, Ingo Molnar <mingo@redhat.com>,
        Len Brown <lenb@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>,
        Pavel Machek <pavel@ucw.cz>, Thomas Gleixner <tglx@linutronix.de>,
        Prarit Bhargava <prarit@redhat.com>,
        Richard Cochran <richardcochran@gmail.com>
Subject: Re: [PATCH 3/4] timekeeping: Ignore the bogus sleep time if pm_trace
 is enabled
Message-ID: <20161121081755.GB14231@gmail.com>
References: <1479531014-25264-1-git-send-email-john.stultz@linaro.org>
 <1479531014-25264-4-git-send-email-john.stultz@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1479531014-25264-4-git-send-email-john.stultz@linaro.org>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1555
Lines: 54


* John Stultz <john.stultz@linaro.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