Re: [PATCH] TCO Watchdog warning interrupt driver creation

[Darren Hart <dvhart-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]

On Fri, Dec 05, 2014 at 10:08:45AM +0000, Muller, Francois-nicolas wrote:
> This driver provides support for TCO watchdog warning interrupt.
> 

Hi Francois,

Cc: Rafael and linux-acpi per his request on ACPI drivers (even if they are for
platform/drivers/x86).

> This feature is useful to root cause watchdog expiration.

Would this not be better suited as a debug option to the iTCO_wdt.c driver under
drivers/watchdog?

Cc: Wim Van Sebroeck and linux-watchdog

> Upon first expiration of the TCO watchdog, a warning interrupt is fired
> to this driver, which calls panic() function and dumps debug information
> (registers and call stacks).

Nit: Newlines between paragraphs for legibility here and in comments please.

> This implies TCO watchdog driver has enabled the interrupt (SCI) and ACPI
> tables contain GPE mapping information.
> After the interrupt has been fired, TCO watchdog reloads automatically and
> upon second expiration it trigs a reset of the platform.

triggers

> 
> Change-Id: I39b615b59dd4336bf208454f08b3e9eac9eb2880

Please run through checkpatch.pl (Gerrit change-id's should be removed).

> Signed-off-by: Francois-Nicolas Muller <francois-nicolas.muller-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> drivers/platform/x86/Kconfig          |   13 +++++
> drivers/platform/x86/Makefile         |    1 +
> drivers/platform/x86/intel_warn_int.c |   98 +++++++++++++++++++++++++++++++++
> 3 files changed, 112 insertions(+)
> create mode 100644 drivers/platform/x86/intel_warn_int.c
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 5ae65c1..79cba16 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -827,4 +827,17 @@ config INTEL_BAYTRAIL_MBI
>                  Interface. This is a requirement for systems that need to configure
>                  the PUNIT for power management features such as RAPL.
> +config INTEL_WARN_INT
> +             tristate "TCO Watchdog warning interrupt"
> +             depends on ITCO_WDT
> +             ---help---
> +               This driver provides support for TCO watchdog warning interrupt.
> +               Upon first expiration of the TCO watchdog, a warning interrupt is
> +               fired and the driver calls panic() function to dump debug information

s/function//

> +               (registers and call stacks).

newline

> +               At the same time, the TCO watchdog reloads with 2.4 seconds timeout
> +               value and runs till the second expiration. At the second expiration of

s/till/until/

> +               the TCO watchdog, the platform resets (the dump is supposed to last less
> +               than 2.4 seconds).
> +
> endif # X86_PLATFORM_DEVICES
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 32caae3..2d47b4d 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -58,3 +58,4 @@ obj-$(CONFIG_PVPANIC)                               += pvpanic.o
> obj-$(CONFIG_INTEL_BAYTRAIL_MBI)                += intel_baytrail.o
> obj-$(CONFIG_INTEL_SOC_PMIC)        += dc_ti_cc.o
> obj-$(CONFIG_ACPI)                   += intel_em_config.o
> +obj-$(CONFIG_INTEL_WARN_INT)      += intel_warn_int.o
> diff --git a/drivers/platform/x86/intel_warn_int.c b/drivers/platform/x86/intel_warn_int.c
> new file mode 100644
> index 0000000..7ec8b73
> --- /dev/null
> +++ b/drivers/platform/x86/intel_warn_int.c
> @@ -0,0 +1,98 @@
> +/*
> + * intel_warn_int.c - This driver provides support for TCO watchdog warning
> + * interrupt.

Newlines between paragraphs in this section too.

> + * This feature is useful to root cause watchdog expiration.
> + * Upon first expiration of the TCO watchdog, a warning interrupt is fired
> + * to this driver, which calls panic() function and dumps debug information

s/function//

> + * (registers and call stacks).
> + * This implies TCO watchdog driver has enabled the interrupt (SCI) and ACPI

the ACPI...

> + * tables contain GPE mapping information.

the GPE...

> + * After the interrupt has been fired, TCO watchdog reloads automatically and
> + * upon second expiration it trigs a reset of the platform.

triggers

> + * Copyright (c) 2014, Intel Corporation.

All rights reserved.

> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *

extra * line, can remove.

> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/acpi.h>
> +#include <linux/nmi.h>
> +#include <acpi/actypes.h>
> +
> +#define DRV_NAME "warn_int"
> +#define TCO_CLASS DRV_NAME
> +
> +static const struct acpi_device_id tco_device_ids[] = {
> +             {"8086229C", 0},

What is this device ID? Is it specific to a debug ID for the WDT? If not, will
this eventually conflict with a non-debug driver for this WDT?

> +             {"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, tco_device_ids);
> +
> +static u32 warn_irq_handler(acpi_handle gpe_device, u32 gpe, void *context)
> +{
> +             pr_warn("[SHTDWN] %s, WATCHDOG TIMEOUT HANDLER!\n", __func__);

Indenting with 13 spaces? Please review Documentation/CodingStyle and update the
patch. Rather than point out all these sorts of errors, I'm goign to stop here -
please read the doc and correct throughout.

> +
> +             trigger_all_cpu_backtrace();
> +             panic("Kernel Watchdog");
> +
> +             /* This code should not be reached */
> +             return IRQ_HANDLED;
> +}
> +
> +static int acpi_tco_add(struct acpi_device *device)
> +{
> +             acpi_status status;
> +             unsigned long long tco_gpe;

Declare variables in decreasing line length order.

> +
> +             status = acpi_evaluate_integer(device->handle, "_GPE", NULL, &tco_gpe);
> +             if (ACPI_FAILURE(status))
> +                             return -EINVAL;
> +
> +             status = acpi_install_gpe_handler(NULL, tco_gpe,
> +                                                                              ACPI_GPE_EDGE_TRIGGERED,
> +                                                                              warn_irq_handler, NULL);
> +             if (ACPI_FAILURE(status))
> +                             return -ENODEV;
> +
> +             acpi_enable_gpe(NULL, tco_gpe);
> +
> +             pr_info("initialized. Interrupt=SCI GPE 0x%02llx", tco_gpe);
> +             return 0;
> +}
> +
> +static struct acpi_driver tco_driver = {
> +             .name = "warn_int",
> +             .class = TCO_CLASS,
> +             .ids = tco_device_ids,
> +             .ops = {
> +                             .add = acpi_tco_add,
> +             },
> +};
> +
> +static int __init warn_int_init(void)
> +{
> +             return acpi_bus_register_driver(&tco_driver);
> +}
> +
> +module_init(warn_int_init);
> +/* no module_exit, this driver shouldn't be unloaded */
> +
> +MODULE_AUTHOR("Francois-Nicolas Muller <francois-nicolas.muller-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>");
> +MODULE_DESCRIPTION("Intel TCO WatchDog Warning Interrupt");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> --
> 1.7.9.5
> 

-- 
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html