From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4BF66C433FE for ; Sat, 23 Apr 2022 06:50:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233647AbiDWGxH (ORCPT ); Sat, 23 Apr 2022 02:53:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43312 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233589AbiDWGxG (ORCPT ); Sat, 23 Apr 2022 02:53:06 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1DED87F20C; Fri, 22 Apr 2022 23:50:09 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 0478F60B4D; Sat, 23 Apr 2022 06:50:09 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CCCF8C385A5; Sat, 23 Apr 2022 06:50:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1650696608; bh=N74KVf0Xg1tp9U6s88Q84deTqBztJDb0FzAXhMjhEW4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=AXwHADvT9bmFHX0p2wyDgl0QCoHo4RZi3t+kVmn6d1P6Ky6rW4PALGL+H++PrHako j2WeC/kzVQ2RX2z4KJJ925UrIusHekMufSMTKJOAJecBywBcsJE+tWglxJHXWqk09Q PrKTaQJngGHIqMcsw90Gm7OiZyfq4nuObagEbcmA= Date: Sat, 23 Apr 2022 08:50:03 +0200 From: Greg Kroah-Hartman To: Sebastian Ene Cc: Rob Herring , Arnd Bergmann , Dragan Cvetic , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, maz@kernel.org, will@kernel.org, qperret@google.com Subject: Re: [PATCH v2 2/2] misc: Add a mechanism to detect stalls on guest vCPUs Message-ID: References: <20220422141949.3456505-1-sebastianene@google.com> <20220422141949.3456505-3-sebastianene@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220422141949.3456505-3-sebastianene@google.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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