linux-kernel - Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host time sample

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: <20141014111915.GV23154@mwanda>
Date:	Tue, 14 Oct 2014 14:19:15 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Thomas Shao <huishao@...rosoft.com>
Cc:	tglx@...utronix.de, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org, devel@...uxdriverproject.org,
	olaf@...fle.de, apw@...onical.com, jasowang@...hat.com,
	kys@...rosoft.com
Subject: Re: [PATCH 2/2] hyperv: Implement Time Synchronization using host
 time sample

I had a couple small style nits.

On Tue, Oct 14, 2014 at 04:11:18AM -0700, Thomas Shao wrote:
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 3b9c9ef..1d8390c 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -51,11 +51,30 @@
>  #define HB_WS2008_MAJOR	1
>  #define HB_WS2008_VERSION	(HB_WS2008_MAJOR << 16 | HB_MINOR)
>  
> +#define  TIMESAMPLE_INTERVAL 5000000000L  /* 5s in nanosecond */

If you wanted you could say:

#define  TIMESAMPLE_INTERVAL  (5 * NSEC_PER_SEC)

> +
> +/*host sends time sample for every 5s.So the max polling interval
> + *is 128*5 = 640s.
> +*/

The comment still has problems throughout.  Read
Documentation/CodingStyle.

> +#define  TIME_ADJ_MAX_INTERVAL 128 /*Max polling interval */
> +
>  static int sd_srv_version;
>  static int ts_srv_version;
>  static int hb_srv_version;
>  static int util_fw_version;
>  
> +/*host sends time sample for every 5s.So the initial polling interval
> + *is 5s.
> +*/
> +static s32 adj_interval = 1;

Prefer mundane types instead there is a reason.  This should be int
because it's not specified in a hardware spec.  I know you are being
consistent with the surrounding code, but the surrounding code is bad so
don't emulate it.

> +
> +/*The host_time_sync module parameter is used to control the time
> +  sync between host and guest.
> +*/
> +static bool host_time_sync;
> +module_param(host_time_sync, bool, (S_IRUGO | S_IWUSR));
> +MODULE_PARM_DESC(host_time_sync, "If the guest sync time with host");

Maybe say: "Synchronize time with the host"?

> +
>  static void shutdown_onchannelcallback(void *context);
>  static struct hv_util_service util_shutdown = {
>  	.util_cb = shutdown_onchannelcallback,
> @@ -163,15 +182,61 @@ static void shutdown_onchannelcallback(void *context)
>  /*
>   * Set guest time to host UTC time.
>   */
> -static inline void do_adj_guesttime(u64 hosttime)
> +static inline void do_adj_guesttime(u64 hosttime, bool forceSync)

I'm surprise checkpatch.pl does't complain about this CamelCase.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/