Clocksource: Move the Hyper-V clocksource driver out of staging
Subsystems (2)
PATCH 1/1 Clocksource: Move the Hyper-V clocksource driver out of staging
Move the Hyper-V clocksource driver out of staging.
--- 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
drivers/clocksource/Makefile +1 -0
@@ -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
drivers/clocksource/hv_timesource.c new file +102 -0
102 lines not on this page · raw message
drivers/staging/hv/Makefile +1 -1
@@ -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
drivers/staging/hv/hv_timesource.c deleted +0 -102
102 lines not on this page · raw message
Signature · 1 line
1.7.4.1
drivers/clocksource/hv_timesource.c line 35
Quoted 12 lines · K. Y. Srinivasan
drivers/clocksource/hv_timesource.c new file +12
-0+ +#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
Quoted 37 lines
drivers/clocksource/hv_timesource.c new file +37
-0+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" ), + }, + },
drivers/clocksource/hv_timesource.c line 83
Quoted 10 lines
drivers/clocksource/hv_timesource.c new file +10
-0+ { }, +}; +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 ?
drivers/clocksource/hv_timesource.c line 90
Quoted 6 lines
drivers/clocksource/hv_timesource.c new file +6
-0+ +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 ?
drivers/clocksource/hv_timesource.c line 93
Quoted 2 lines
drivers/clocksource/hv_timesource.c new file +2
-0+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.
drivers/clocksource/hv_timesource.c line 96
Quoted 2 lines
drivers/clocksource/hv_timesource.c new file +2
-0+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.
drivers/clocksource/hv_timesource.c line 102
Quoted 6 lines
drivers/clocksource/hv_timesource.c new file +6
-0+} + +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
Quoted 13 lines
-----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 stagingOn Mon, 23 May 2011, K. Y. Srinivasan wrote:
Quoted 14 lines
drivers/clocksource/hv_timesource.c line 35
Quoted 12 lines
drivers/clocksource/hv_timesource.c new file +12
-0+ +#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 22Please do not use hard coded conversion factors. See below
I will fix this.
Quoted 35 lines
Quoted 24 lines
drivers/clocksource/hv_timesource.c new file +23
-0+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) +*/ drivers/clocksource/hv_timesource.c
Quoted 10 lines
drivers/clocksource/hv_timesource.c new file +10
-0+ .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, "MicrosoftCorporation"),
Quoted 16 lines
Quoted 4 lines
drivers/clocksource/hv_timesource.c new file +4
-0+ DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"), + DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"), + }, + },drivers/clocksource/hv_timesource.c line 83
Quoted 10 lines
drivers/clocksource/hv_timesource.c new file +10
-0+ { }, +}; +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_idtable is unused. What's the purpose ?
Both the PCI and DMI tables were defined to ensure autoloading
Quoted 10 lines
drivers/clocksource/hv_timesource.c
Quoted 6 lines
drivers/clocksource/hv_timesource.c new file +5
-0+ +static int __init init_hv_clocksource (void ) +{ + if ((x86_hyper != &x86_hyper_ms_hyperv) || + !(ms_hyperv.features &
HV_X64_MSR_TIME_REF_COUNT_AVAILABLE))Review
Quoted 1 line
(quoted diff) +1
-0+ return -ENODEV;Isn't this a sufficient indicator ?
This ensures we are running on Hyper-V and for that this check is sufficient.
Quoted 7 lines
drivers/clocksource/hv_timesource.c line 93
Quoted 3 lines
drivers/clocksource/hv_timesource.c new file +2
-0+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_hypervthen 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.
Quoted 6 lines
drivers/clocksource/hv_timesource.c line 96
Quoted 3 lines
drivers/clocksource/hv_timesource.c new file +2
-0+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.
Quoted 9 lines
drivers/clocksource/hv_timesource.c line 102
Quoted 7 lines
drivers/clocksource/hv_timesource.c new file +6
-0+} + +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
Quoted 10 lines · KY Srinivasan
Quoted 6 lines
drivers/clocksource/hv_timesource.c line 102
Quoted 4 lines
drivers/clocksource/hv_timesource.c new file +4
-0+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
Quoted 80 lines · K. Y. Srinivasan
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.cdrivers/clocksource/Makefile +1
-0@@ -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.odrivers/clocksource/hv_timesource.c new file +51
-0@@ -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*/
drivers/clocksource/hv_timesource.c line 61
Quoted 10 lines
drivers/clocksource/hv_timesource.c new file +10
-0+ .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.
drivers/clocksource/hv_timesource.c line 102
Quoted 4 lines
drivers/clocksource/hv_timesource.c new file +4
-0+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
Quoted 111 lines
-----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 stagingQuoted 8 lines · K. Y. Srinivasan
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/clocksource/Makefile obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC) += cs5535-
Quoted 11 lines
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.cdrivers/clocksource/Makefile +0
-0@@ -6,3 +6,4 @@ obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC) += cs5535-clockevt.o
Review
Quoted 5 lines
drivers/clocksource/Makefile +1
-0obj-$(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(quoted diff) +0
-0
b/drivers/clocksource/hv_timesource.cQuoted 9 lines
new file mode 100644
index 0000000..0efb049drivers/clocksource/hv_timesource.c new file +4
-0@@ -0,0 +1,102 @@ +/* +* A clocksource for Linux running on HyperV. +* +* drivers/clocksource/hv_timesource.c line 14
Quoted 10 lines
drivers/clocksource/hv_timesource.c new file +10
-0+* 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
Quoted 37 lines
drivers/clocksource/hv_timesource.c new file +37
-0+* 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*/ drivers/clocksource/hv_timesource.c line 61
Quoted 10 lines
drivers/clocksource/hv_timesource.c new file +10
-0+ .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.
Quoted 1 line
Otherwise it looks pretty straightforward.
Will do; thanks.
Quoted 8 lines
drivers/clocksource/hv_timesource.c line 102
Quoted 6 lines
drivers/clocksource/hv_timesource.c new file +4
-0+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�
Quoted 1 line
+#include <
linux/version.h>
This one shouldn't be needed.
drivers/clocksource/hv_timesource.c line 32
Quoted 2 lines
drivers/clocksource/hv_timesource.c new file +2
-0+#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?
Quoted 12 lines
-----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
Quoted 3 lines
Quoted 1 line
+#include <
linux/version.h>This one shouldn't be needed.
Correct; I will fix this.
Quoted 6 lines
drivers/clocksource/hv_timesource.c line 32
Quoted 3 lines
drivers/clocksource/hv_timesource.c new file +2
-0+#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.
Quoted 3 lines
Shouldn't the
Kconfigentry also move fromdrivers/stagingtoarch/x86towards 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