If for any reason, host timesync messages were not processed by
the guest, hv_ptp_gettime() returns a stale value and the
caller (clock_gettime, PTP ioctl etc) has no means to know this
now. Return an error so that the caller knows about this.
Signed-off-by: Vineeth Pillai <[email protected]>
---
drivers/hv/hv_util.c | 46 +++++++++++++++++++++++++++++++++-----------
1 file changed, 35 insertions(+), 11 deletions(-)
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 92ee0fe4c919..1357861fd8ae 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -282,26 +282,52 @@ static struct {
spinlock_t lock;
} host_ts;
-static struct timespec64 hv_get_adj_host_time(void)
+static inline u64 reftime_to_ns(u64 reftime)
{
- struct timespec64 ts;
- u64 newtime, reftime;
+ return (reftime - WLTIMEDELTA) * 100;
+}
+
+/*
+ * Hard coded threshold for host timesync delay: 600 seconds
+ */
+const u64 HOST_TIMESYNC_DELAY_THRESH = 600 * NSEC_PER_SEC;
+
+static int hv_get_adj_host_time(struct timespec64 *ts)
+{
+ u64 newtime, reftime, timediff_adj;
unsigned long flags;
+ int ret = 0;
spin_lock_irqsave(&host_ts.lock, flags);
reftime = hv_read_reference_counter();
- newtime = host_ts.host_time + (reftime - host_ts.ref_time);
- ts = ns_to_timespec64((newtime - WLTIMEDELTA) * 100);
+
+ /*
+ * We need to let the caller know that last update from host
+ * is older than the max allowable threshold. clock_gettime()
+ * and PTP ioctl do not have a documented error that we could
+ * return for this specific case. Use ESTALE to report this.
+ */
+ timediff_adj = reftime - host_ts.ref_time;
+ if (timediff_adj * 100 > HOST_TIMESYNC_DELAY_THRESH) {
+ pr_warn("TIMESYNC IC: Stale time stamp, %llu nsecs old\n",
+ HOST_TIMESYNC_DELAY_THRESH);
+ ret = -ESTALE;
+ }
+
+ newtime = host_ts.host_time + timediff_adj;
+ *ts = ns_to_timespec64(reftime_to_ns(newtime));
spin_unlock_irqrestore(&host_ts.lock, flags);
- return ts;
+ return ret;
}
static void hv_set_host_time(struct work_struct *work)
{
- struct timespec64 ts = hv_get_adj_host_time();
- do_settimeofday64(&ts);
+ struct timespec64 ts;
+
+ if (!hv_get_adj_host_time(&ts))
+ do_settimeofday64(&ts);
}
/*
@@ -622,9 +648,7 @@ static int hv_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
static int hv_ptp_gettime(struct ptp_clock_info *info, struct timespec64 *ts)
{
- *ts = hv_get_adj_host_time();
-
- return 0;
+ return hv_get_adj_host_time(ts);
}
static struct ptp_clock_info ptp_hyperv_info = {
--
2.17.1
Hi Vineeth,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.9-rc1 next-20200819]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Vineeth-Pillai/hv_utils-return-error-if-host-timesysnc-update-is-stale/20200820-014742
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 18445bf405cb331117bc98427b1ba6f12418ad17
config: i386-randconfig-s001-20200818 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-183-gaa6ede3b-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> drivers/hv/hv_util.c:293:11: sparse: sparse: symbol 'HOST_TIMESYNC_DELAY_THRESH' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Signed-off-by: kernel test robot <[email protected]>
---
hv_util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 1357861fd8aee8..35d126e6f012db 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -290,7 +290,7 @@ static inline u64 reftime_to_ns(u64 reftime)
/*
* Hard coded threshold for host timesync delay: 600 seconds
*/
-const u64 HOST_TIMESYNC_DELAY_THRESH = 600 * NSEC_PER_SEC;
+static const u64 HOST_TIMESYNC_DELAY_THRESH = 600 * NSEC_PER_SEC;
static int hv_get_adj_host_time(struct timespec64 *ts)
{
Hi Vineeth,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.9-rc1 next-20200819]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Vineeth-Pillai/hv_utils-return-error-if-host-timesysnc-update-is-stale/20200820-014742
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 18445bf405cb331117bc98427b1ba6f12418ad17
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/hv/hv_util.c:293:44: warning: integer overflow in expression of type 'long int' results in '-1295421440' [-Woverflow]
293 | const u64 HOST_TIMESYNC_DELAY_THRESH = 600 * NSEC_PER_SEC;
| ^
# https://github.com/0day-ci/linux/commit/89495ac7e3c97f1a96ba77a67b532cdeaddc624c
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Vineeth-Pillai/hv_utils-return-error-if-host-timesysnc-update-is-stale/20200820-014742
git checkout 89495ac7e3c97f1a96ba77a67b532cdeaddc624c
vim +293 drivers/hv/hv_util.c
289
290 /*
291 * Hard coded threshold for host timesync delay: 600 seconds
292 */
> 293 const u64 HOST_TIMESYNC_DELAY_THRESH = 600 * NSEC_PER_SEC;
294
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
From: Vineeth Pillai <[email protected]> Sent: Wednesday, August 19, 2020 10:45 AM
>
> If for any reason, host timesync messages were not processed by
> the guest, hv_ptp_gettime() returns a stale value and the
> caller (clock_gettime, PTP ioctl etc) has no means to know this
> now. Return an error so that the caller knows about this.
>
> Signed-off-by: Vineeth Pillai <[email protected]>
> ---
> drivers/hv/hv_util.c | 46 +++++++++++++++++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 92ee0fe4c919..1357861fd8ae 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -282,26 +282,52 @@ static struct {
> spinlock_t lock;
> } host_ts;
>
> -static struct timespec64 hv_get_adj_host_time(void)
> +static inline u64 reftime_to_ns(u64 reftime)
> {
> - struct timespec64 ts;
> - u64 newtime, reftime;
> + return (reftime - WLTIMEDELTA) * 100;
> +}
> +
> +/*
> + * Hard coded threshold for host timesync delay: 600 seconds
> + */
> +const u64 HOST_TIMESYNC_DELAY_THRESH = 600 * NSEC_PER_SEC;
Kernel test robot has already complained that this should be static,
and about the potential overflow based on the types of the constants in
the right side expression. I didn't check the details, but I suspect the
complaint is when building in 32-bit mode. This code does get built in
32-bit mode and it's possible for run 32-bit Linux guests on Hyper-V.
> +
> +static int hv_get_adj_host_time(struct timespec64 *ts)
> +{
> + u64 newtime, reftime, timediff_adj;
> unsigned long flags;
> + int ret = 0;
>
> spin_lock_irqsave(&host_ts.lock, flags);
> reftime = hv_read_reference_counter();
> - newtime = host_ts.host_time + (reftime - host_ts.ref_time);
> - ts = ns_to_timespec64((newtime - WLTIMEDELTA) * 100);
> +
> + /*
> + * We need to let the caller know that last update from host
> + * is older than the max allowable threshold. clock_gettime()
> + * and PTP ioctl do not have a documented error that we could
> + * return for this specific case. Use ESTALE to report this.
> + */
> + timediff_adj = reftime - host_ts.ref_time;
> + if (timediff_adj * 100 > HOST_TIMESYNC_DELAY_THRESH) {
> + pr_warn("TIMESYNC IC: Stale time stamp, %llu nsecs old\n",
> + HOST_TIMESYNC_DELAY_THRESH);
Let's provide the timediff_adj in the message instead of the constant
threshold value so we know the degree of staleness. :-)
Also, I'm wondering if this should be pr_warn_once(). Presumably
chronyd or whoever is reading /dev/ptp0 will give up after getting
this error, but if not, it would be nice to avoid filling up the console
with these error messages.
> + ret = -ESTALE;
> + }
> +
> + newtime = host_ts.host_time + timediff_adj;
> + *ts = ns_to_timespec64(reftime_to_ns(newtime));
> spin_unlock_irqrestore(&host_ts.lock, flags);
>
> - return ts;
> + return ret;
> }
>
> static void hv_set_host_time(struct work_struct *work)
> {
> - struct timespec64 ts = hv_get_adj_host_time();
>
> - do_settimeofday64(&ts);
> + struct timespec64 ts;
> +
> + if (!hv_get_adj_host_time(&ts))
> + do_settimeofday64(&ts);
> }
>
> /*
> @@ -622,9 +648,7 @@ static int hv_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>
> static int hv_ptp_gettime(struct ptp_clock_info *info, struct timespec64 *ts)
> {
> - *ts = hv_get_adj_host_time();
> -
> - return 0;
> + return hv_get_adj_host_time(ts);
> }
>
> static struct ptp_clock_info ptp_hyperv_info = {
> --
> 2.17.1
Hi Michael,
> > +const u64 HOST_TIMESYNC_DELAY_THRESH = 600 * NSEC_PER_SEC;
>
> Kernel test robot has already complained that this should be static,
> and about the potential overflow based on the types of the constants in
> the right side expression. I didn't check the details, but I suspect the
> complaint is when building in 32-bit mode. This code does get built in
> 32-bit mode and it's possible for run 32-bit Linux guests on Hyper-V.
>
NSEC_PER_SEC is defined long and it caused the warning with i386 build.
Casting it to u64 would fix the issue. Will fix the static warning as well
in the next iteration.
> > + pr_warn("TIMESYNC IC: Stale time stamp, %llu nsecs old\n",
> > + HOST_TIMESYNC_DELAY_THRESH);
>
> Let's provide the timediff_adj in the message instead of the constant
> threshold value so we know the degree of staleness. :-)
>
> Also, I'm wondering if this should be pr_warn_once(). Presumably
> chronyd or whoever is reading /dev/ptp0 will give up after getting
> this error, but if not, it would be nice to avoid filling up the console
> with these error messages.
>
Makes sense, will fix this also.
Thanks,
Vineeth