Move the Hyper-V clocksource driver out of staging.
Signed-off-by: K. Y. Srinivasan <[email protected]>
Signed-off-by: Hank Janssen <[email protected]>
Signed-off-by: Haiyang Zhang <[email protected]>
---
drivers/clocksource/Makefile | 1 +
drivers/clocksource/hv_timesource.c | 102 +++++++++++++++++++++++++++++++++++
drivers/staging/hv/Makefile | 2 +-
drivers/staging/hv/hv_timesource.c | 102 -----------------------------------
4 files changed, 104 insertions(+), 103 deletions(-)
create mode 100644 drivers/clocksource/hv_timesource.c
delete mode 100644 drivers/staging/hv/hv_timesource.c
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index be61ece..ea44327 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC) += cs5535-clockevt.o
obj-$(CONFIG_SH_TIMER_CMT) += sh_cmt.o
obj-$(CONFIG_SH_TIMER_MTU2) += sh_mtu2.o
obj-$(CONFIG_SH_TIMER_TMU) += sh_tmu.o
+obj-$(CONFIG_HYPERV) += hv_timesource.o
diff --git a/drivers/clocksource/hv_timesource.c b/drivers/clocksource/hv_timesource.c
new file mode 100644
index 0000000..0efb049
--- /dev/null
+++ b/drivers/clocksource/hv_timesource.c
@@ -0,0 +1,102 @@
+/*
+ * A clocksource for Linux running on HyperV.
+ *
+ *
+ * Copyright (C) 2010, Novell, Inc.
+ * Author : K. Y. Srinivasan <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/version.h>
+#include <linux/clocksource.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/dmi.h>
+#include <asm/hyperv.h>
+#include <asm/mshyperv.h>
+#include <asm/hypervisor.h>
+
+#define HV_CLOCK_SHIFT 22
+
+static cycle_t read_hv_clock(struct clocksource *arg)
+{
+ cycle_t current_tick;
+ /*
+ * Read the partition counter to get the current tick count. This count
+ * is set to 0 when the partition is created and is incremented in
+ * 100 nanosecond units.
+ */
+ rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
+ return current_tick;
+}
+
+static struct clocksource hyperv_cs = {
+ .name = "hyperv_clocksource",
+ .rating = 400, /* use this when running on Hyperv*/
+ .read = read_hv_clock,
+ .mask = CLOCKSOURCE_MASK(64),
+ /*
+ * The time ref counter in HyperV is in 100ns units.
+ * The definition of mult is:
+ * mult/2^shift = ns/cyc = 100
+ * mult = (100 << shift)
+ */
+ .mult = (100 << HV_CLOCK_SHIFT),
+ .shift = HV_CLOCK_SHIFT,
+};
+
+static const struct dmi_system_id __initconst
+hv_timesource_dmi_table[] __maybe_unused = {
+ {
+ .ident = "Hyper-V",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
+ DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
+ },
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
+
+static const struct pci_device_id __initconst
+hv_timesource_pci_table[] __maybe_unused = {
+ { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
+ { 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
+
+
+static int __init init_hv_clocksource(void)
+{
+ if ((x86_hyper != &x86_hyper_ms_hyperv) ||
+ !(ms_hyperv.features & HV_X64_MSR_TIME_REF_COUNT_AVAILABLE))
+ return -ENODEV;
+
+ if (!dmi_check_system(hv_timesource_dmi_table))
+ return -ENODEV;
+
+ pr_info("Registering HyperV clock source\n");
+ return clocksource_register(&hyperv_cs);
+}
+
+module_init(init_hv_clocksource);
+MODULE_DESCRIPTION("HyperV based clocksource");
+MODULE_AUTHOR("K. Y. Srinivasan <[email protected]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/staging/hv/Makefile b/drivers/staging/hv/Makefile
index 3004674..14e3c6a 100644
--- a/drivers/staging/hv/Makefile
+++ b/drivers/staging/hv/Makefile
@@ -1,4 +1,4 @@
-obj-$(CONFIG_HYPERV) += hv_vmbus.o hv_timesource.o
+obj-$(CONFIG_HYPERV) += hv_vmbus.o
obj-$(CONFIG_HYPERV_STORAGE) += hv_storvsc.o
obj-$(CONFIG_HYPERV_BLOCK) += hv_blkvsc.o
obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
diff --git a/drivers/staging/hv/hv_timesource.c b/drivers/staging/hv/hv_timesource.c
deleted file mode 100644
index 0efb049..0000000
--- a/drivers/staging/hv/hv_timesource.c
+++ /dev/null
@@ -1,102 +0,0 @@
-/*
- * A clocksource for Linux running on HyperV.
- *
- *
- * Copyright (C) 2010, Novell, Inc.
- * Author : K. Y. Srinivasan <[email protected]>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
- * NON INFRINGEMENT. See the GNU General Public License for more
- * details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
- *
- */
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include <linux/version.h>
-#include <linux/clocksource.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/pci.h>
-#include <linux/dmi.h>
-#include <asm/hyperv.h>
-#include <asm/mshyperv.h>
-#include <asm/hypervisor.h>
-
-#define HV_CLOCK_SHIFT 22
-
-static cycle_t read_hv_clock(struct clocksource *arg)
-{
- cycle_t current_tick;
- /*
- * Read the partition counter to get the current tick count. This count
- * is set to 0 when the partition is created and is incremented in
- * 100 nanosecond units.
- */
- rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
- return current_tick;
-}
-
-static struct clocksource hyperv_cs = {
- .name = "hyperv_clocksource",
- .rating = 400, /* use this when running on Hyperv*/
- .read = read_hv_clock,
- .mask = CLOCKSOURCE_MASK(64),
- /*
- * The time ref counter in HyperV is in 100ns units.
- * The definition of mult is:
- * mult/2^shift = ns/cyc = 100
- * mult = (100 << shift)
- */
- .mult = (100 << HV_CLOCK_SHIFT),
- .shift = HV_CLOCK_SHIFT,
-};
-
-static const struct dmi_system_id __initconst
-hv_timesource_dmi_table[] __maybe_unused = {
- {
- .ident = "Hyper-V",
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
- DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
- DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
- },
- },
- { },
-};
-MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
-
-static const struct pci_device_id __initconst
-hv_timesource_pci_table[] __maybe_unused = {
- { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
- { 0 }
-};
-MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
-
-
-static int __init init_hv_clocksource(void)
-{
- if ((x86_hyper != &x86_hyper_ms_hyperv) ||
- !(ms_hyperv.features & HV_X64_MSR_TIME_REF_COUNT_AVAILABLE))
- return -ENODEV;
-
- if (!dmi_check_system(hv_timesource_dmi_table))
- return -ENODEV;
-
- pr_info("Registering HyperV clock source\n");
- return clocksource_register(&hyperv_cs);
-}
-
-module_init(init_hv_clocksource);
-MODULE_DESCRIPTION("HyperV based clocksource");
-MODULE_AUTHOR("K. Y. Srinivasan <[email protected]>");
-MODULE_LICENSE("GPL");
--
1.7.4.1
On Mon, 23 May 2011, K. Y. Srinivasan wrote:
> +
> +#include <linux/version.h>
> +#include <linux/clocksource.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/dmi.h>
> +#include <asm/hyperv.h>
> +#include <asm/mshyperv.h>
> +#include <asm/hypervisor.h>
> +
> +#define HV_CLOCK_SHIFT 22
Please do not use hard coded conversion factors. See below
> +static cycle_t read_hv_clock(struct clocksource *arg)
> +{
> + cycle_t current_tick;
> + /*
> + * Read the partition counter to get the current tick count. This count
> + * is set to 0 when the partition is created and is incremented in
> + * 100 nanosecond units.
> + */
> + rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
> + return current_tick;
> +}
> +
> +static struct clocksource hyperv_cs = {
> + .name = "hyperv_clocksource",
> + .rating = 400, /* use this when running on Hyperv*/
> + .read = read_hv_clock,
> + .mask = CLOCKSOURCE_MASK(64),
> + /*
> + * The time ref counter in HyperV is in 100ns units.
> + * The definition of mult is:
> + * mult/2^shift = ns/cyc = 100
> + * mult = (100 << shift)
> + */
> + .mult = (100 << HV_CLOCK_SHIFT),
> + .shift = HV_CLOCK_SHIFT,
> +};
> +
> +static const struct dmi_system_id __initconst
> +hv_timesource_dmi_table[] __maybe_unused = {
> + {
> + .ident = "Hyper-V",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
> + DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
> + },
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
> +
> +static const struct pci_device_id __initconst
> +hv_timesource_pci_table[] __maybe_unused = {
> + { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
> + { 0 }
> +};
> +MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
This pci_id table is unused. What's the purpose ?
> +
> +static int __init init_hv_clocksource(void)
> +{
> + if ((x86_hyper != &x86_hyper_ms_hyperv) ||
> + !(ms_hyperv.features & HV_X64_MSR_TIME_REF_COUNT_AVAILABLE))
> + return -ENODEV;
Isn't this a sufficient indicator ?
> + if (!dmi_check_system(hv_timesource_dmi_table))
> + return -ENODEV;
Do we really need this additional check? I'd say if x86_hyper ==
x86_hyper_ms_hyperv then failing the DMI check would be more than
silly.
> + pr_info("Registering HyperV clock source\n");
> + return clocksource_register(&hyperv_cs);
Please use clocksource_register_hz/khz and get rid of the hard coded
constants.
> +}
> +
> +module_init(init_hv_clocksource);
> +MODULE_DESCRIPTION("HyperV based clocksource");
> +MODULE_AUTHOR("K. Y. Srinivasan <[email protected]>");
> +MODULE_LICENSE("GPL");
Why do we need to build this as a module?
Thanks,
tglx
On Mon, 2011-05-23 at 10:12 -0700, K. Y. Srinivasan wrote:
> Move the Hyper-V clocksource driver out of staging.
>
> Signed-off-by: K. Y. Srinivasan <[email protected]>
> Signed-off-by: Hank Janssen <[email protected]>
> Signed-off-by: Haiyang Zhang <[email protected]>
> ---
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/hv_timesource.c | 102 +++++++++++++++++++++++++++++++++++
> drivers/staging/hv/Makefile | 2 +-
> drivers/staging/hv/hv_timesource.c | 102 -----------------------------------
> 4 files changed, 104 insertions(+), 103 deletions(-)
> create mode 100644 drivers/clocksource/hv_timesource.c
> delete mode 100644 drivers/staging/hv/hv_timesource.c
>
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index be61ece..ea44327 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC) += cs5535-clockevt.o
> obj-$(CONFIG_SH_TIMER_CMT) += sh_cmt.o
> obj-$(CONFIG_SH_TIMER_MTU2) += sh_mtu2.o
> obj-$(CONFIG_SH_TIMER_TMU) += sh_tmu.o
> +obj-$(CONFIG_HYPERV) += hv_timesource.o
> diff --git a/drivers/clocksource/hv_timesource.c b/drivers/clocksource/hv_timesource.c
> new file mode 100644
> index 0000000..0efb049
> --- /dev/null
> +++ b/drivers/clocksource/hv_timesource.c
> @@ -0,0 +1,102 @@
> +/*
> + * A clocksource for Linux running on HyperV.
> + *
> + *
> + * Copyright (C) 2010, Novell, Inc.
> + * Author : K. Y. Srinivasan <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT. See the GNU General Public License for more
> + * details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/version.h>
> +#include <linux/clocksource.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/dmi.h>
> +#include <asm/hyperv.h>
> +#include <asm/mshyperv.h>
> +#include <asm/hypervisor.h>
> +
> +#define HV_CLOCK_SHIFT 22
> +
> +static cycle_t read_hv_clock(struct clocksource *arg)
> +{
> + cycle_t current_tick;
> + /*
> + * Read the partition counter to get the current tick count. This count
> + * is set to 0 when the partition is created and is incremented in
> + * 100 nanosecond units.
> + */
> + rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
> + return current_tick;
> +}
> +
> +static struct clocksource hyperv_cs = {
> + .name = "hyperv_clocksource",
> + .rating = 400, /* use this when running on Hyperv*/
> + .read = read_hv_clock,
> + .mask = CLOCKSOURCE_MASK(64),
> + /*
> + * The time ref counter in HyperV is in 100ns units.
> + * The definition of mult is:
> + * mult/2^shift = ns/cyc = 100
> + * mult = (100 << shift)
> + */
> + .mult = (100 << HV_CLOCK_SHIFT),
> + .shift = HV_CLOCK_SHIFT,
The mult/shift assignments can be dropped. Please use
clocksource_register_hz/khz() which will assign mult/shift for you.
Otherwise it looks pretty straightforward.
> +module_init(init_hv_clocksource);
> +MODULE_DESCRIPTION("HyperV based clocksource");
> +MODULE_AUTHOR("K. Y. Srinivasan <[email protected]>");
> +MODULE_LICENSE("GPL");
One other nit: Should this email address be updated to your current one?
thanks
-john
> +#include <linux/version.h>
This one shouldn't be needed.
> +#include <asm/hyperv.h>
> +#include <asm/mshyperv.h>
not really a review for this driver, but what's the purpose if having
these two headers?
Shouldn't the Kconfig entry also move from drivers/staging to
arch/x86 towards the other clocksources?
> -----Original Message-----
> From: john stultz [mailto:[email protected]]
> Sent: Monday, May 23, 2011 2:40 PM
> To: KY Srinivasan
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Hank
> Janssen; Haiyang Zhang
> Subject: Re: [PATCH 1/1] Clocksource: Move the Hyper-V clocksource driver out
> of staging
>
> On Mon, 2011-05-23 at 10:12 -0700, K. Y. Srinivasan wrote:
> > Move the Hyper-V clocksource driver out of staging.
> >
> > Signed-off-by: K. Y. Srinivasan <[email protected]>
> > Signed-off-by: Hank Janssen <[email protected]>
> > Signed-off-by: Haiyang Zhang <[email protected]>
> > ---
> > drivers/clocksource/Makefile | 1 +
> > drivers/clocksource/hv_timesource.c | 102
> +++++++++++++++++++++++++++++++++++
> > drivers/staging/hv/Makefile | 2 +-
> > drivers/staging/hv/hv_timesource.c | 102 -----------------------------------
> > 4 files changed, 104 insertions(+), 103 deletions(-)
> > create mode 100644 drivers/clocksource/hv_timesource.c
> > delete mode 100644 drivers/staging/hv/hv_timesource.c
> >
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index be61ece..ea44327 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -6,3 +6,4 @@ obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC) += cs5535-
> clockevt.o
> > obj-$(CONFIG_SH_TIMER_CMT) += sh_cmt.o
> > obj-$(CONFIG_SH_TIMER_MTU2) += sh_mtu2.o
> > obj-$(CONFIG_SH_TIMER_TMU) += sh_tmu.o
> > +obj-$(CONFIG_HYPERV) += hv_timesource.o
> > diff --git a/drivers/clocksource/hv_timesource.c
> b/drivers/clocksource/hv_timesource.c
> > new file mode 100644
> > index 0000000..0efb049
> > --- /dev/null
> > +++ b/drivers/clocksource/hv_timesource.c
> > @@ -0,0 +1,102 @@
> > +/*
> > + * A clocksource for Linux running on HyperV.
> > + *
> > + *
> > + * Copyright (C) 2010, Novell, Inc.
> > + * Author : K. Y. Srinivasan <[email protected]>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE
> or
> > + * NON INFRINGEMENT. See the GNU General Public License for more
> > + * details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> > + *
> > + */
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/version.h>
> > +#include <linux/clocksource.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/dmi.h>
> > +#include <asm/hyperv.h>
> > +#include <asm/mshyperv.h>
> > +#include <asm/hypervisor.h>
> > +
> > +#define HV_CLOCK_SHIFT 22
> > +
> > +static cycle_t read_hv_clock(struct clocksource *arg)
> > +{
> > + cycle_t current_tick;
> > + /*
> > + * Read the partition counter to get the current tick count. This count
> > + * is set to 0 when the partition is created and is incremented in
> > + * 100 nanosecond units.
> > + */
> > + rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
> > + return current_tick;
> > +}
> > +
> > +static struct clocksource hyperv_cs = {
> > + .name = "hyperv_clocksource",
> > + .rating = 400, /* use this when running on Hyperv*/
> > + .read = read_hv_clock,
> > + .mask = CLOCKSOURCE_MASK(64),
> > + /*
> > + * The time ref counter in HyperV is in 100ns units.
> > + * The definition of mult is:
> > + * mult/2^shift = ns/cyc = 100
> > + * mult = (100 << shift)
> > + */
> > + .mult = (100 << HV_CLOCK_SHIFT),
> > + .shift = HV_CLOCK_SHIFT,
>
> The mult/shift assignments can be dropped. Please use
> clocksource_register_hz/khz() which will assign mult/shift for you.
>
> Otherwise it looks pretty straightforward.
Will do; thanks.
>
>
> > +module_init(init_hv_clocksource);
> > +MODULE_DESCRIPTION("HyperV based clocksource");
> > +MODULE_AUTHOR("K. Y. Srinivasan <[email protected]>");
> > +MODULE_LICENSE("GPL");
>
> One other nit: Should this email address be updated to your current one?
I will update it.
Regards,
K. Y
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> -----Original Message-----
> From: Thomas Gleixner [mailto:[email protected]]
> Sent: Monday, May 23, 2011 2:16 PM
> To: KY Srinivasan
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Hank
> Janssen; Haiyang Zhang
> Subject: Re: [PATCH 1/1] Clocksource: Move the Hyper-V clocksource driver out
> of staging
>
> On Mon, 23 May 2011, K. Y. Srinivasan wrote:
> > +
> > +#include <linux/version.h>
> > +#include <linux/clocksource.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/dmi.h>
> > +#include <asm/hyperv.h>
> > +#include <asm/mshyperv.h>
> > +#include <asm/hypervisor.h>
> > +
> > +#define HV_CLOCK_SHIFT 22
>
> Please do not use hard coded conversion factors. See below
I will fix this.
>
> > +static cycle_t read_hv_clock(struct clocksource *arg)
> > +{
> > + cycle_t current_tick;
> > + /*
> > + * Read the partition counter to get the current tick count. This count
> > + * is set to 0 when the partition is created and is incremented in
> > + * 100 nanosecond units.
> > + */
> > + rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
> > + return current_tick;
> > +}
> > +
> > +static struct clocksource hyperv_cs = {
> > + .name = "hyperv_clocksource",
> > + .rating = 400, /* use this when running on Hyperv*/
> > + .read = read_hv_clock,
> > + .mask = CLOCKSOURCE_MASK(64),
> > + /*
> > + * The time ref counter in HyperV is in 100ns units.
> > + * The definition of mult is:
> > + * mult/2^shift = ns/cyc = 100
> > + * mult = (100 << shift)
> > + */
> > + .mult = (100 << HV_CLOCK_SHIFT),
> > + .shift = HV_CLOCK_SHIFT,
> > +};
> > +
> > +static const struct dmi_system_id __initconst
> > +hv_timesource_dmi_table[] __maybe_unused = {
> > + {
> > + .ident = "Hyper-V",
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "Microsoft
> Corporation"),
> > + DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
> > + DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
> > + },
> > + },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
> > +
> > +static const struct pci_device_id __initconst
> > +hv_timesource_pci_table[] __maybe_unused = {
> > + { PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
> > + { 0 }
> > +};
> > +MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
>
> This pci_id table is unused. What's the purpose ?
Both the PCI and DMI tables were defined to ensure autoloading
>
> > +
> > +static int __init init_hv_clocksource(void)
> > +{
> > + if ((x86_hyper != &x86_hyper_ms_hyperv) ||
> > + !(ms_hyperv.features &
> HV_X64_MSR_TIME_REF_COUNT_AVAILABLE))
> > + return -ENODEV;
>
> Isn't this a sufficient indicator ?
This ensures we are running on Hyper-V and for that this check is sufficient.
>
> > + if (!dmi_check_system(hv_timesource_dmi_table))
> > + return -ENODEV;
>
> Do we really need this additional check? I'd say if x86_hyper ==
> x86_hyper_ms_hyperv then failing the DMI check would be more than
> silly.
Both the PCI and DMI tables were introduced by Greg to support autoloading.
I will see if I can clean this up.
>
> > + pr_info("Registering HyperV clock source\n");
> > + return clocksource_register(&hyperv_cs);
>
> Please use clocksource_register_hz/khz and get rid of the hard coded
> constants.
Will do.
>
> > +}
> > +
> > +module_init(init_hv_clocksource);
> > +MODULE_DESCRIPTION("HyperV based clocksource");
> > +MODULE_AUTHOR("K. Y. Srinivasan <[email protected]>");
> > +MODULE_LICENSE("GPL");
>
> Why do we need to build this as a module?
No particular reason. This is the way, I had it in the staging directory.
Do you want me to build this as part of the kernel? I would then not have
to worry about auto-loading issues.
Regards,
K. Y
> -----Original Message-----
> From: Christoph Hellwig [mailto:[email protected]]
> Sent: Monday, May 23, 2011 2:43 PM
> To: KY Srinivasan
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Hank Janssen; Haiyang Zhang
> Subject: Re: [PATCH 1/1] Clocksource: Move the Hyper-V clocksource driver out
> of staging
>
> > +#include <linux/version.h>
>
> This one shouldn't be needed.
Correct; I will fix this.
>
> > +#include <asm/hyperv.h>
> > +#include <asm/mshyperv.h>
>
> not really a review for this driver, but what's the purpose if having
> these two headers?
These header files have the defines for checking if we are running on
Hyper-V.
>
> Shouldn't the Kconfig entry also move from drivers/staging to
> arch/x86 towards the other clocksources?
Currently this driver is built if HYPERV is defined. I agree with you
that I should have Kconfig entry in arch/x86. Since the rest of the
Hyper-V drivers are still in staging, I am thinking that perhaps we should
have a different config switch - HYPERV_CLKSRC. I will go ahead and
spin a new version of this patch with this change.
Regards,
K. Y
On Mon, 23 May 2011, KY Srinivasan wrote:
> > > +module_init(init_hv_clocksource);
> > > +MODULE_DESCRIPTION("HyperV based clocksource");
> > > +MODULE_AUTHOR("K. Y. Srinivasan <[email protected]>");
> > > +MODULE_LICENSE("GPL");
> >
> > Why do we need to build this as a module?
>
> No particular reason. This is the way, I had it in the staging directory.
> Do you want me to build this as part of the kernel? I would then not have
> to worry about auto-loading issues.
If we get rid of the DMI/PCI stuff then the whole module code is
larger than the real code. So no point in building it modular, just
make it depend on CONFIG_WHATEVER_MEANS_HYPERV
Thanks,
tglx