public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: John Stultz <john.stultz@linaro.org>
To: George Spelvin <linux@horizon.com>
Cc: linux-kernel@vger.kernel.org, Alexander Gordeev <lasaine@lvk.cs.msu.su>
Subject: Re: [PATCH] ntp: make is_error_status() use its argument
Date: Mon, 12 May 2014 10:54:29 -0700	[thread overview]
Message-ID: <53710AD5.504@linaro.org> (raw)
In-Reply-To: <20140512133548.31421.qmail@ns.horizon.com>

On 05/12/2014 06:35 AM, George Spelvin wrote:
> It's an inline function always called with the global time_status as an
> argument, so there's zero functional difference, but the non-CONFIG_SMP
> version uses the passed-in argument, while the CONFIG_SMP one ignores
> its argument and uses the global.

CONFIG_NTP_PPS not CONFIG_SMP, right?

Good catch though, looks like the check code was re-factored out, but
someone forgot to use the local variable.

Adding Alexander since he submitted the pps logic.

If there's no objections, I'll queue this (with a more verbose commit
message) for 3.16

thanks
-john


>     
> Make it use the argument always; shorter variable names are good.
>
> Signed-off-by: George Spelvin <linux@horizon.com>
> ---
> While poking about in the code, I came across this rather odd bit.
> It looked worth fixing, on general principles.
>
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index 419a52cecd..1a2aad3fff 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -165,21 +165,21 @@ static inline void pps_set_freq(s64 freq)
>  
>  static inline int is_error_status(int status)
>  {
> -	return (time_status & (STA_UNSYNC|STA_CLOCKERR))
> +	return (status & (STA_UNSYNC|STA_CLOCKERR))
>  		/* PPS signal lost when either PPS time or
>  		 * PPS frequency synchronization requested
>  		 */
> -		|| ((time_status & (STA_PPSFREQ|STA_PPSTIME))
> -			&& !(time_status & STA_PPSSIGNAL))
> +		|| ((status & (STA_PPSFREQ|STA_PPSTIME))
> +			&& !(status & STA_PPSSIGNAL))
>  		/* PPS jitter exceeded when
>  		 * PPS time synchronization requested */
> -		|| ((time_status & (STA_PPSTIME|STA_PPSJITTER))
> +		|| ((status & (STA_PPSTIME|STA_PPSJITTER))
>  			== (STA_PPSTIME|STA_PPSJITTER))
>  		/* PPS wander exceeded or calibration error when
>  		 * PPS frequency synchronization requested
>  		 */
> -		|| ((time_status & STA_PPSFREQ)
> -			&& (time_status & (STA_PPSWANDER|STA_PPSERROR)));
> +		|| ((status & STA_PPSFREQ)
> +			&& (status & (STA_PPSWANDER|STA_PPSERROR)));
>  }
>  
>  static inline void pps_fill_timex(struct timex *txc)


  reply	other threads:[~2014-05-12 17:54 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-05-12 13:35 [PATCH] ntp: make is_error_status() use its argument George Spelvin
2014-05-12 17:54 ` John Stultz [this message]
2014-05-12 20:10   ` Alexander Gordeev
2014-05-13  3:01   ` George Spelvin
2014-05-13  3:15     ` John Stultz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=53710AD5.504@linaro.org \
    --to=john.stultz@linaro.org \
    --cc=lasaine@lvk.cs.msu.su \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@horizon.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox