On Fri, Apr 22, 2022 at 02:19:50PM +0000, Sebastian Ene wrote:
> This patch adds support for a virtual watchdog which relies on the
> per-cpu hrtimers to pet at regular intervals.
>
> Signed-off-by: Sebastian Ene <sebastianene@xxxxxxxxxx>
> ---
> drivers/misc/Kconfig | 8 ++
> drivers/misc/Makefile | 1 +
> drivers/misc/vm-wdt.c | 215 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 224 insertions(+)
> create mode 100644 drivers/misc/vm-wdt.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 2b9572a6d114..0e710149ff95 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -493,6 +493,14 @@ config OPEN_DICE
>
> If unsure, say N.
>
> +config VM_WATCHDOG
> + tristate "Virtual Machine Watchdog"
> + select LOCKUP_DETECTOR
> + help
> + Detect CPU locks on the virtual machine.
> + To compile this driver as a module, choose M here: the
> + module will be called vm-wdt.
> +
> source "drivers/misc/c2port/Kconfig"
> source "drivers/misc/eeprom/Kconfig"
> source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 2ec634354cf5..868e28d01b75 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o
> obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o
> obj-$(CONFIG_UID_SYS_STATS) += uid_sys_stats.o
> obj-$(CONFIG_OPEN_DICE) += open-dice.o
> +obj-$(CONFIG_VM_WATCHDOG) += vm-wdt.o
No tab?
> \ No newline at end of file
> diff --git a/drivers/misc/vm-wdt.c b/drivers/misc/vm-wdt.c
> new file mode 100644
> index 000000000000..ea4351754645
> --- /dev/null
> +++ b/drivers/misc/vm-wdt.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0+
I have to ask, do you really mean "+" here as this is not the overall
license of the kernel. It's not a normal license for your employer to
pick, so as long as you have legal approval, it's fine, but if not, you
need to get that.
> +//
> +// Virtual watchdog driver.
> +// Copyright (C) Google, 2022
> +
> +#define pr_fmt(fmt) "vm-watchdog: " fmt
It's a driver, you shouldn't need any pr_* calls.
> +
> +#include <linux/cpu.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/nmi.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/param.h>
> +#include <linux/percpu.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define DRV_NAME "vm_wdt"
KBUILD_MODNAME?
> +#define DRV_VERSION "1.0"
"versions" mean nothing once the code is in the kernel, please drop
this.
But why isn't this in the normal watchdog subdirectory? Why is this a
special driver?
> +
> +#define VMWDT_REG_STATUS (0x00)
> +#define VMWDT_REG_LOAD_CNT (0x04)
> +#define VMWDT_REG_CURRENT_CNT (0x08)
> +#define VMWDT_REG_CLOCK_FREQ_HZ (0x0C)
> +#define VMWDT_REG_LEN (0x10)
> +
> +#define VMWDT_DEFAULT_CLOCK_HZ (10)
> +#define VMWDT_DEFAULT_TIMEOT_SEC (8)
> +
> +struct vm_wdt_s {
> + void __iomem *membase;
> + u32 clock_freq;
> + u32 expiration_sec;
> + u32 ping_timeout_ms;
> + struct hrtimer per_cpu_hrtimer;
> + struct platform_device *dev;
> +};
> +
> +#define vmwdt_reg_write(wdt, reg, value) \
> + iowrite32((value), (wdt)->membase + (reg))
> +#define vmwdt_reg_read(wdt, reg) \
> + io32read((wdt)->membase + (reg))
> +
> +static struct platform_device *virt_dev;
> +
> +static enum hrtimer_restart vmwdt_timer_fn(struct hrtimer *hrtimer)
> +{
> + struct vm_wdt_s *cpu_wdt;
> + u32 ticks;
> +
> + cpu_wdt = container_of(hrtimer, struct vm_wdt_s, per_cpu_hrtimer);
> + ticks = cpu_wdt->clock_freq * cpu_wdt->expiration_sec;
> + vmwdt_reg_write(cpu_wdt, VMWDT_REG_LOAD_CNT, ticks);
> + hrtimer_forward_now(hrtimer, ms_to_ktime(cpu_wdt->ping_timeout_ms));
> +
> + return HRTIMER_RESTART;
> +}
> +
> +static void vmwdt_start(void *arg)
> +{
> + u32 ticks;
> + int cpu = smp_processor_id();
> + struct vm_wdt_s *cpu_wdt = arg;
> + struct hrtimer *hrtimer = &cpu_wdt->per_cpu_hrtimer;
> +
> + pr_info("cpu %u vmwdt start\n", cpu);
When drivers work properly, they are quiet.
Again, why not have this in drivers/watchdog/ and use the apis there
instead of creating a custom one for no reason?
thanks,
greg k-h
