From: Thomas Shao <huishao@microsoft.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
CC: "tglx@linutronix.de" <tglx@linutronix.de>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devel@linuxdriverproject.org" <devel@linuxdriverproject.org>,
        "olaf@aepfle.de" <olaf@aepfle.de>,
        "apw@canonical.com" <apw@canonical.com>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        KY Srinivasan <kys@microsoft.com>
Subject: RE: [PATCH 2/2] hyperv: Implement Time Synchronization using host
 time sample
Thread-Topic: [PATCH 2/2] hyperv: Implement Time Synchronization using host
 time sample
Thread-Index: AQHP55ZRTp/VO8CmfEuTJogFjyVUzJwvcjOAgAAW4JA=
Date: Tue, 14 Oct 2014 12:50:23 +0000
Message-ID: <CD2CABCB2C0A0D4682C5F8AD840141540BFD19@HKXPRD3002MB006.064d.mgd.msft.net>
References: <1413285078-7027-1-git-send-email-huishao@microsoft.com>
 <20141014111915.GV23154@mwanda>
In-Reply-To: <20141014111915.GV23154@mwanda>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org


> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Tuesday, October 14, 2014 7:19 PM
> To: Thomas Shao
> Cc: tglx@linutronix.de; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com; KY Srinivasan
> 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.
> 

I will correct the style of the comment.

> > +#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.
> 
I agree with you. Maybe it's a good idea to correct the surrounding code in 
another patch.

> > +
> > +/*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"?

Sounds good.

> 
> > +
> >  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.

I've run the scripts/checkpatch.pl, it didn't complain anything. I'll change to forcesync.

> 
> regards,
> dan carpenter

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