> -----Original Message----- > From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx] > Sent: Monday, May 23, 2011 2:16 PM > To: KY Srinivasan > Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxxxx; johnstul@xxxxxxxxxx; hch@xxxxxxxxxxxxx; 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 <ksrinivasan@xxxxxxxxxx>"); > > +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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
