[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.LFD.2.02.1105232010470.3078@ionos>
Date: Mon, 23 May 2011 20:15:58 +0200 (CEST)
From: Thomas Gleixner <tglx@...utronix.de>
To: "K. Y. Srinivasan" <kys@...rosoft.com>
cc: gregkh@...e.de, linux-kernel@...r.kernel.org,
devel@...uxdriverproject.org, johnstul@...ibm.com,
hch@...radead.org, Hank Janssen <hjanssen@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>
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
> +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 <ksrinivasan@...ell.com>");
> +MODULE_LICENSE("GPL");
Why do we need to build this as a module?
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists