> -----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
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
